feat: add --json-schema flag to xrd convert#142
Conversation
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
9e0abf8 to
a92acf9
Compare
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds shared CRD-to-JSON-Schema generation, refactors ChangesJSON Schema generation for xrd convert and xpkg get-crds
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant convertCmd.Run
participant CRDsToJSONSchemas
participant writeOutputs
participant OutputDir
participant OutputFile
participant Stdout
User->>convertCmd.Run: select jsonschema format
convertCmd.Run->>CRDsToJSONSchemas: convert derived CRDs
CRDsToJSONSchemas-->>convertCmd.Run: schema output entries
convertCmd.Run->>writeOutputs: write output entries
alt OutputDir
writeOutputs->>OutputDir: write one schema per file
else OutputFile
writeOutputs->>OutputFile: write concatenated payload
else stdout
writeOutputs->>Stdout: write concatenated payload
end
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
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: 2
🧹 Nitpick comments (1)
cmd/crossplane/xrd/convert_test.go (1)
109-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage of the three output modes — thank you for adding this!
One coverage suggestion that ties into the
writeOutputsconcern: every case here usesminimalXRD(...Namespaced, nil), which yields a single schema, so theOutputFile/Stdoutconcatenation path never sees more than one output. Adding a legacy-XRD-with-Claim case (whichTestToCRDsalready shows produces two CRDs) would exercise the multi-schema behavior and lock in whatever decision we land on for that path. Would you be open to adding it?🤖 Prompt for 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. In `@cmd/crossplane/xrd/convert_test.go` around lines 109 - 189, TestConvertJSONSchema only covers a single-schema case, so writeOutputs is not validated when multiple JSON Schemas are produced. Update TestConvertJSONSchema to add a legacy XRD with Claim-enabled case, reusing the existing toCRDs and toJSONSchemaOutputs flow, so the OutputFile and Stdout paths are exercised with more than one schema. Keep the existing minimalXRD coverage, but add a case that mirrors the multi-CRD behavior already demonstrated in TestToCRDs and assert the expected multi-output behavior through convertCmd.writeOutputs.
🤖 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/crossplane/xrd/convert.go`:
- Around line 116-146: `toJSONSchemaOutputs` can return multiple `convertOutput`
entries for a single XRD, but the concatenating branch in `writeOutputs` appears
to assume only one JSON schema output and may merge or overwrite them
incorrectly. Update `writeOutputs` to iterate over all outputs returned by
`toJSONSchemaOutputs` (and keep the `toCRDOutputs` behavior consistent) so every
schema emitted by `CRDsToJSONSchemas` is preserved, using the existing
`toJSONSchemaOutputs` and `writeOutputs` flow as the main places to adjust.
- Around line 163-179: The JSON schema output path in convert.go currently
concatenates multiple schemas into one invalid JSON document when c.OutputFile
or stdout is used. Update the conversion flow around the outputs buffer/write
logic to either reject multi-schema output unless c.OutputDir is set, or
otherwise enforce a single-schema constraint in --json-schema mode. Use the
existing Convert/c.OutputFile/stdout handling to add the guard before writing,
and make sure the multi-schema case is covered explicitly rather than silently
emitting invalid JSON.
---
Nitpick comments:
In `@cmd/crossplane/xrd/convert_test.go`:
- Around line 109-189: TestConvertJSONSchema only covers a single-schema case,
so writeOutputs is not validated when multiple JSON Schemas are produced. Update
TestConvertJSONSchema to add a legacy XRD with Claim-enabled case, reusing the
existing toCRDs and toJSONSchemaOutputs flow, so the OutputFile and Stdout paths
are exercised with more than one schema. Keep the existing minimalXRD coverage,
but add a case that mirrors the multi-CRD behavior already demonstrated in
TestToCRDs and assert the expected multi-output behavior through
convertCmd.writeOutputs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9de61f10-3569-431a-8696-519bc03b87fd
📒 Files selected for processing (5)
cmd/crossplane/xpkg/get_crds.gocmd/crossplane/xrd/convert.gocmd/crossplane/xrd/convert_test.gocmd/crossplane/xrd/help/convert.mdinternal/schemas/generator/json.go
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/crossplane/xrd/convert_test.go (1)
129-132: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPlease create the parent directory for the
-ocase.Thanks for covering the single-file path. One catch:
writeOutputs()does notMkdirAll()forOutputFile, so/out/schema.jsonwill fail onafero.NewMemMapFs()before the JSON assertions run. Could we either pre-create/outin the test or write to a top-level file instead?Suggested tweak
- cmd: convertCmd{JSONSchema: true, OutputFile: "/out/schema.json"}, - wantFile: "/out/schema.json", + cmd: convertCmd{JSONSchema: true, OutputFile: "/schema.json"}, + wantFile: "/schema.json",🤖 Prompt for 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. In `@cmd/crossplane/xrd/convert_test.go` around lines 129 - 132, The OutputFile test case in convert_test.go is using a nested path without creating its parent directory, so the `writeOutputs()` path for `convertCmd` will fail before the JSON checks run. Update the `OutputFile` scenario to either pre-create the parent directory for `/out/schema.json` in the test setup or switch `wantFile` to a top-level file path, keeping the coverage focused on `convertCmd.writeOutputs()` behavior.
🧹 Nitpick comments (1)
cmd/crossplane/xrd/convert_test.go (1)
216-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCould we make the rejection checks table-driven and assert the actual error?
Thanks for adding coverage for the multi-schema guardrails. Right now
err == nilonly proves a failure happened; it will not catch regressions in the actionableuse --output-dirguidance. Folding these intoargs/wantErrcases and comparing withcmp.Diff(..., cmpopts.EquateErrors())would match the repo’s test convention and lock down the CLI UX. As per path instructions,**/*_test.go: “Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, use cmp.Diff with cmpopts.EquateErrors() for error testing.”🤖 Prompt for 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. In `@cmd/crossplane/xrd/convert_test.go` around lines 216 - 226, The multi-schema rejection assertions in convertCmd.writeOutputs tests are still ad hoc and only check for nil errors; convert these cases in convert_test.go to the repo’s table-driven args/wantErr style, using cmp.Diff with cmpopts.EquateErrors() to assert the exact error message and preserve the actionable “use --output-dir” guidance. Keep the coverage around writeOutputs and the JSONSchema / OutputFile scenarios, but fold them into a single PascalCase test with structured cases instead of separate err == nil checks.Source: Path instructions
🤖 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.
Outside diff comments:
In `@cmd/crossplane/xrd/convert_test.go`:
- Around line 129-132: The OutputFile test case in convert_test.go is using a
nested path without creating its parent directory, so the `writeOutputs()` path
for `convertCmd` will fail before the JSON checks run. Update the `OutputFile`
scenario to either pre-create the parent directory for `/out/schema.json` in the
test setup or switch `wantFile` to a top-level file path, keeping the coverage
focused on `convertCmd.writeOutputs()` behavior.
---
Nitpick comments:
In `@cmd/crossplane/xrd/convert_test.go`:
- Around line 216-226: The multi-schema rejection assertions in
convertCmd.writeOutputs tests are still ad hoc and only check for nil errors;
convert these cases in convert_test.go to the repo’s table-driven args/wantErr
style, using cmp.Diff with cmpopts.EquateErrors() to assert the exact error
message and preserve the actionable “use --output-dir” guidance. Keep the
coverage around writeOutputs and the JSONSchema / OutputFile scenarios, but fold
them into a single PascalCase test with structured cases instead of separate err
== nil checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 387c002c-1e2f-4226-a32f-e7b669528125
📒 Files selected for processing (2)
cmd/crossplane/xrd/convert.gocmd/crossplane/xrd/convert_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/crossplane/xrd/convert.go
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
One thought on the flag, but otherwise this lgtm. Thanks for the update!
| OutputDir string `help:"A directory to write the generated CRDs to. Each CRD gets a separate file named after the CRD." placeholder:"DIR" predictor:"directory" type:"path" xor:"output"` | ||
|
|
||
| // Format flags. | ||
| JSONSchema bool `help:"Write JSON Schema files instead of CRDs. Useful for YAML language server integration." name:"json-schema"` |
There was a problem hiding this comment.
I would lean toward having a --format flag that takes crd or jsonschema as an option, with crd as the default. This makes the default more obvious/explicit and also allows for more formats to be added in the future.
There was a problem hiding this comment.
Absolutely! Changed
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/crossplane/xrd/convert.go (2)
179-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd context to the stdout error message.
To align completely with the CLI error message path instructions, consider specifying that the output was destined for
stdout. This small addition gives users a clearer picture of what failed.💡 Proposed change
- if _, err := k.Stdout.Write(buf.Bytes()); err != nil { - return errors.Wrap(err, "cannot write output") - } + if _, err := k.Stdout.Write(buf.Bytes()); err != nil { + return errors.Wrap(err, "cannot write output to stdout") + }🤖 Prompt for 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. In `@cmd/crossplane/xrd/convert.go` around lines 179 - 181, Update the error context in the k.Stdout.Write call within the conversion flow to explicitly identify stdout as the failed output destination, while preserving the existing wrapped error handling.Source: Path instructions
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider generalizing the output flag help text.
Now that this command supports both CRD and JSON Schema formats, the help text for
OutputFileandOutputDirmight be slightly misleading as they still explicitly mention "CRD YAML" and "generated CRDs".Would it make sense to update these descriptions to be format-agnostic? For example, replacing "generated CRD YAML" with "generated output" and "generated CRDs" with "generated output files".
🤖 Prompt for 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. In `@cmd/crossplane/xrd/convert.go` around lines 59 - 60, Update the help text for the OutputFile and OutputDir flags to be format-agnostic, replacing references to CRD YAML and generated CRDs with generated output and generated output files while preserving the existing path and file-naming descriptions.
🤖 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.
Nitpick comments:
In `@cmd/crossplane/xrd/convert.go`:
- Around line 179-181: Update the error context in the k.Stdout.Write call
within the conversion flow to explicitly identify stdout as the failed output
destination, while preserving the existing wrapped error handling.
- Around line 59-60: Update the help text for the OutputFile and OutputDir flags
to be format-agnostic, replacing references to CRD YAML and generated CRDs with
generated output and generated output files while preserving the existing path
and file-naming descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14f4db5c-0bad-4fd4-bc60-192215f35d0c
📒 Files selected for processing (3)
cmd/crossplane/xrd/convert.gocmd/crossplane/xrd/convert_test.gocmd/crossplane/xrd/help/convert.md
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/crossplane/xrd/help/convert.md
- cmd/crossplane/xrd/convert_test.go
2 participants
Description of your changes
Add
--json-schemaflag toxrd convertsubcommand, similarly as whatxpkg get-crdssubcommand already provides.Refactor the common logic to avoid duplication.
Fixes #141
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.