feat(ledger): add schemas commands (insert/get/list)#165
Conversation
Wrap the ledger v2 schema endpoints in fctl. insert accepts a local file or URL containing JSON or YAML; get renders json or yaml.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a new ChangesLedger schemas CLI commands
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ledger/schemas/insert.go`:
- Around line 90-96: The Render method for InsertController always prints
"Schema inserted!" regardless of the API result; check the stored result
(c.store.Success set from response.StatusCode == 201) inside
InsertController.Render and only print the success message when c.store.Success
is true, otherwise print a failure/error message (e.g., "Failed to insert
schema") and return a non-nil error or appropriate non-zero exit indication;
update Render to reference c.store.Success rather than unconditionally calling
pterm.Success.WithWriter(...).Printfln.
- Around line 124-130: The HTTP request in readSource uses cmd.Context() and
http.DefaultClient.Do(req) with no deadline, which can hang; wrap the existing
ctx with a timeout (e.g., ctx, cancel := context.WithTimeout(ctx,
10*time.Second); defer cancel()) and use that timed context when creating the
request (http.NewRequestWithContext) or instantiate a local http.Client with a
Timeout and use client.Do(req); update references in readSource and add the time
import.
In `@cmd/ledger/schemas/list.go`:
- Around line 46-47: Validate the page-size flag value locally before making the
API call: after reading the int flag (c.pageSizeFlag) in the command handler
(the function that builds/runs the list schema command where fctl.WithIntFlag is
used), check that the parsed page size is > 0 and return a clear error (e.g.
"invalid --page-size: must be > 0") if not; apply the same check in the other
handler code path referenced around the second occurrence (lines 68-71) so both
places reject non-positive values before issuing any API request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fe94cdd-cb7d-433a-a7df-13a89bb0a0b9
📒 Files selected for processing (5)
cmd/ledger/root.gocmd/ledger/schemas/get.gocmd/ledger/schemas/insert.gocmd/ledger/schemas/list.gocmd/ledger/schemas/root.go
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
J'ai laissé quelques commentaires inline. Le point principal est la pagination de schemas list: l'endpoint est cursor-paginé mais la commande ne permet pas de récupérer les pages suivantes.
| return nil, err | ||
| } | ||
|
|
||
| response, err := stackClient.Ledger.V2.ListSchemas(cmd.Context(), operations.V2ListSchemasRequest{ |
There was a problem hiding this comment.
This endpoint is cursor-paginated (V2ListSchemasRequest.Cursor, then V2SchemasCursor.HasMore/Next/Previous in the response), but the command only exposes --page-size and only stores Data. Once a ledger has more than 15 schema versions, or more than the requested page size, the remaining pages are unreachable from fctl. Please add a --cursor flag, pass it into the request, and render the cursor metadata like the other list commands do.
|
|
||
| response, err := stackClient.Ledger.V2.ListSchemas(cmd.Context(), operations.V2ListSchemasRequest{ | ||
| Ledger: fctl.GetString(cmd, internal.LedgerFlag), | ||
| PageSize: pointer.For(int64(fctl.GetInt(cmd, c.pageSizeFlag))), |
There was a problem hiding this comment.
--page-size is read with GetInt without validation, so --page-size 0 or a negative value is forwarded to the API. Recent paginated commands use fctl.WithPageSizeFlag() plus fctl.GetPageSize() to reject these values in the CLI; doing the same here would avoid an invalid API call.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| resp, err := http.DefaultClient.Do(req) |
There was a problem hiding this comment.
This request bypasses the HTTP client configured by fctl, so global transport flags such as --insecure-tls do not apply to https:// schema sources. Since URLs are an official input for this command, please use a client built from the command flags, or inject it from Run, instead of using http.DefaultClient.
- insert: error out unless the API returns 204 (correct success code; was 201 and masked by an unconditional success message) - insert: fetch URL sources via fctl's configured HTTP client (honors --insecure-tls) with a request timeout - list: use WithCursorFlag/WithPageSizeFlag, follow cursor pagination, validate page size, and render cursor metadata
Review feedback addressed (commit faa6293)All five points from @flemzord and CodeRabbit are fixed and verified live against staging.
Pagination — full testCreated a dedicated ledger with 12 schemas (
Server-side observation (not an fctl issue): the first page honors page-size, but pages fetched via the cursor return all remaining rows rather than
|
2 participants
Summary
InsertSchema,GetSchema,ListSchemas) in fctl under a newfctl ledger schemascommand group.insert <version> <source>—sourceis a local file path or anhttp(s)://URL, and the content can be JSON or YAML (auto-detected, no flag). YAML is normalized to JSON before decoding into the SDK struct so itsjsontags are honored.get <version> [--format json|yaml]— defaults to colorized JSON;--format yamlemits YAML for easy round-tripping (get --format yaml→ edit →insert).list [--page-size N]— table of Version, Created at, and chart/queries/transactions counts.--ledgerflag (consistent withtransactions/accounts). No new dependencies (yaml.v3was already required).Test plan
go build,go vet ./...,gofmt -l,golangci-lint run --fix(0 issues),go mod tidy(no diff)kpuqzlfmhzqx, stackdrru, ledgermy-ledger):insertfrom local JSON, local YAML, and a URL returning YAMLlistshows all inserted versionsgetrenders both JSON (default) and--format yaml, round-tripping the stored schema