feat(xr): Convert a Claim to XR via crossplane xr generate#13
Conversation
Introduce a Claim to XR converter: `crossplane xr generate claim.yaml`. Similarly to other converter functions/tools, it takes a Claim either on stdin or as file argument and generates an equivalent XR YAML. The parent `crossplane xr` is marked as maturity "alpha", which applies to the subtree as well. This new tool can be used together with the render command as shown in the example below: ```bash crossplane render <(crossplane xr generate claim.yaml) composition.yaml functions.yaml ``` The library function ConvertClaimToXR can be used by downstream tools (eg crossplane-diff) that rely on Claim to XR conversion. The tool (and as a result the converter function, via the Options struct) is configurable and provides the following flags: - `--direct`: omit spec.claimRef and the equivalent labels. - `--name`: specify an explicit XR name, skipping either the random suffix in the non-direct mode or the Claim name fallback in direct mode. - `--kind`: override the derived "X<Kind>" default. - `--gen-uid`: populate metadata.uid with a fresh random UUID. Signed-off-by: Theo Chatzimichos <tampakrap@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces a new ChangesXR Generate Command
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ 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: 3
🧹 Nitpick comments (1)
cmd/crossplane/xr/generate.go (1)
116-116: 💤 Low valueMake error message consistent with codebase style.
For consistency with other error messages in the codebase (which use lowercase and "cannot"), consider changing "Unable to marshal back to yaml" to "cannot marshal XR to YAML".
♻️ Suggested change
- if err != nil { - return errors.Wrap(err, "Unable to marshal back to yaml") - } + if err != nil { + return errors.Wrap(err, "cannot marshal XR to YAML") + }🤖 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/xr/generate.go` at line 116, Change the error string passed to errors.Wrap in the return statement that currently reads errors.Wrap(err, "Unable to marshal back to yaml") so it follows project style; replace the message with "cannot marshal XR to YAML" (i.e., use errors.Wrap(err, "cannot marshal XR to YAML")) in the function in generate.go where the XR marshaling failure is handled.
🤖 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/xr/generate.go`:
- Around line 136-143: Update the user-facing error message constants
(errNoAPIVersion, errNoKind, errNoSpecSection and optionally errEmptyClaimYAML
and errNilInput) to be user-friendly and actionable: replace terse developer
phrases with full-sentence guidance that names the missing field and suggests a
next step (for example: indicate the Claim YAML must include an
apiVersion/kind/spec and ask the user to check their Claim definition or
validate their YAML). Locate these constants by name in the file
(errNoAPIVersion, errNoKind, errNoSpecSection, errEmptyClaimYAML, errNilInput)
and update their string values accordingly so CLI output gives clear, helpful
instructions to the user.
- Around line 244-257: When building the XR in non-Direct mode (the block
guarded by if !opts.Direct), validate that claim.GetNamespace() is non-empty
before setting labels[labelClaimNamespace] or calling
xrPaved.SetValue("spec.claimRef", ...); if the namespace is empty return a
wrapped error (e.g. "claim namespace required in non-Direct mode") so we don't
create an XR with an empty claimRef namespace; perform this check early in the
if !opts.Direct block (before using claim.GetNamespace()) and only proceed to
set labels and call xrPaved.SetValue when the namespace is present.
- Line 100: Replace the generic errors.Wrap(err, "Unmarshalling Error") call
with a user-friendly, specific CLI message such as errors.Wrap(err, "cannot
parse Claim YAML") or errors.Wrap(err, "invalid Claim YAML format"); update the
string in the existing errors.Wrap(err, "...") expression so the returned error
refers to the Claim YAML parsing problem (keep the wrapped internal err for
diagnostics but avoid exposing raw internal details in the top-level message).
---
Nitpick comments:
In `@cmd/crossplane/xr/generate.go`:
- Line 116: Change the error string passed to errors.Wrap in the return
statement that currently reads errors.Wrap(err, "Unable to marshal back to
yaml") so it follows project style; replace the message with "cannot marshal XR
to YAML" (i.e., use errors.Wrap(err, "cannot marshal XR to YAML")) in the
function in generate.go where the XR marshaling failure is handled.
🪄 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: 2c84bb47-bee6-4223-abcf-f9d69ec8d963
📒 Files selected for processing (4)
cmd/crossplane/main.gocmd/crossplane/xr/generate.gocmd/crossplane/xr/generate_test.gocmd/crossplane/xr/xr.go
|
|
||
| claim := &unstructured.Unstructured{} | ||
| if err := yaml.Unmarshal(claimData, claim); err != nil { | ||
| return errors.Wrap(err, "Unmarshalling Error") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Improve error message specificity.
The error message "Unmarshalling Error" doesn't tell the user what went wrong with their Claim YAML. Consider making it more specific and helpful, such as "cannot parse Claim YAML" or "invalid Claim YAML format".
As per coding guidelines, CLI error messages must be especially user-friendly and avoid internal error details.
💬 Suggested improvement
- if err := yaml.Unmarshal(claimData, claim); err != nil {
- return errors.Wrap(err, "Unmarshalling Error")
- }
+ if err := yaml.Unmarshal(claimData, claim); err != nil {
+ return errors.Wrap(err, "cannot parse Claim YAML")
+ }🤖 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/xr/generate.go` at line 100, Replace the generic
errors.Wrap(err, "Unmarshalling Error") call with a user-friendly, specific CLI
message such as errors.Wrap(err, "cannot parse Claim YAML") or errors.Wrap(err,
"invalid Claim YAML format"); update the string in the existing errors.Wrap(err,
"...") expression so the returned error refers to the Claim YAML parsing problem
(keep the wrapped internal err for diagnostics but avoid exposing raw internal
details in the top-level message).
There was a problem hiding this comment.
I agree a bit with coderabbit on adding a little detail here, but more importantly by convention error messages should start with a lower-case letter.
| return errors.Wrap(err, "Unmarshalling Error") | |
| return errors.Wrap(err, "cannot unmarshal claim") |
There was a problem hiding this comment.
@adamwg, great point! The lowercase convention for error messages is indeed a standard Go idiom — the Go wiki's CodeReviewComments specifically states that error strings should not be capitalized. Your suggestion "cannot unmarshal claim" is both idiomatic and descriptive, making it a clear improvement over the original "Unmarshalling Error".
✏️ Learnings added
Learnt from: adamwg
Repo: crossplane/cli PR: 13
File: cmd/crossplane/xr/generate.go:100-100
Timestamp: 2026-05-20T21:52:31.054Z
Learning: In crossplane/cli Go code, error messages passed to errors.Wrap() and errors.New() must start with a lowercase letter, following standard Go conventions (per Go wiki CodeReviewComments). For example, use `errors.Wrap(err, "cannot unmarshal claim")` not `errors.Wrap(err, "Unmarshalling Error")`. This applies to all Go files in the repository.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: internal/project/build.go:361-364
Timestamp: 2026-05-12T22:54:33.936Z
Learning: When reviewing Go code targeting Go 1.26+ (per go.mod and/or CI build settings), do not flag `new(expr)` as a compile error. In Go 1.26, the built-in `new` accepts an expression (not only a type): `new(expr)` allocates a variable of the type of `expr`, initializes it to the value of `expr`, and returns a pointer to it (`*T`). This means usages like `new(someFunc())` or `new(someStringConstant)` are valid syntax and should be allowed under Go 1.26 or later.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: internal/project/build.go:361-364
Timestamp: 2026-05-12T22:54:33.936Z
Learning: When reviewing Go code, allow the Go 1.26+ syntax `new(expr)` where `expr` is any expression (not just a type). In Go 1.26, `new(expr)` allocates a value of the type of `expr`, initializes it to the value of `expr`, and returns a `*T` pointer to it (e.g., `new(someFunc())`, `new(someStringConstant)`, `new(pkg.Const)`). If the repository targets Go 1.26 or later (e.g., module `go` directive >= 1.26 and/or build/CI uses Go >= 1.26), do not flag these usages as compilation errors.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: cmd/crossplane/dependency/dependency.go:21-25
Timestamp: 2026-05-13T18:59:33.289Z
Learning: In crossplane/cli, experimental/beta CLI command gating is done via the kong command registration struct tag `maturity:"beta"` (or similar) placed on the *parent/top-level* command fields (e.g., top-level `Cmd` fields in `cmd/crossplane/main.go`). Subcommands registered under a parent are already considered gated by that parent maturity. During code review, do not request/introduce programmatic feature flags or add a wrapper function to conditionally register commands; treat the `maturity` tag as the correct and sufficient mechanism. Instead, focus review attention on whether the appropriate maturity tag is set on the relevant parent command.
| const ( | ||
| // Error messages. | ||
| errNilInput = "input is nil" | ||
| errEmptyClaimYAML = "invalid Claim YAML: parsed object is empty" | ||
| errNoAPIVersion = "Claim has no apiVersion" | ||
| errParseAPIVersion = "failed to parse Claim APIVersion" | ||
| errNoKind = "Claim has no kind section" | ||
| errNoSpecSection = "Claim has no spec section" |
There was a problem hiding this comment.
Make error messages more user-friendly and actionable.
The error message constants are developer-focused (e.g., "Claim has no apiVersion") rather than user-friendly. They don't provide context about what the user was trying to do or suggest next steps.
Consider messages like:
errNoAPIVersion: "The Claim YAML must include an 'apiVersion' field. Please check your Claim definition."errNoKind: "The Claim YAML must include a 'kind' field. Please check your Claim definition."errNoSpecSection: "The Claim YAML must include a 'spec' section. Please check your Claim definition."
As per coding guidelines, error messages must be meaningful to end users, not just developers—avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible. CLI error messages must be especially user-friendly.
🤖 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/xr/generate.go` around lines 136 - 143, Update the user-facing
error message constants (errNoAPIVersion, errNoKind, errNoSpecSection and
optionally errEmptyClaimYAML and errNilInput) to be user-friendly and
actionable: replace terse developer phrases with full-sentence guidance that
names the missing field and suggests a next step (for example: indicate the
Claim YAML must include an apiVersion/kind/spec and ask the user to check their
Claim definition or validate their YAML). Locate these constants by name in the
file (errNoAPIVersion, errNoKind, errNoSpecSection, errEmptyClaimYAML,
errNilInput) and update their string values accordingly so CLI output gives
clear, helpful instructions to the user.
There was a problem hiding this comment.
I disagree with coderabbit here - its suggested error messages are excessive 😆.
There was a problem hiding this comment.
@adamwg Fair enough! 😄 The concise style keeps the messages clean without being overly prescriptive. I'll note this preference for future reviews.
✏️ Learnings added
Learnt from: adamwg
Repo: crossplane/cli PR: 13
File: cmd/crossplane/xr/generate.go:136-143
Timestamp: 2026-05-20T21:52:28.707Z
Learning: In crossplane/cli, concise error messages (e.g., "Claim has no apiVersion", "Claim has no kind section") are preferred over verbose, instructional messages that spell out suggested next steps. Do not flag these short-form error strings as insufficiently user-friendly in code review.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: internal/project/build.go:361-364
Timestamp: 2026-05-12T22:54:33.936Z
Learning: When reviewing Go code targeting Go 1.26+ (per go.mod and/or CI build settings), do not flag `new(expr)` as a compile error. In Go 1.26, the built-in `new` accepts an expression (not only a type): `new(expr)` allocates a variable of the type of `expr`, initializes it to the value of `expr`, and returns a pointer to it (`*T`). This means usages like `new(someFunc())` or `new(someStringConstant)` are valid syntax and should be allowed under Go 1.26 or later.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: internal/project/build.go:361-364
Timestamp: 2026-05-12T22:54:33.936Z
Learning: When reviewing Go code, allow the Go 1.26+ syntax `new(expr)` where `expr` is any expression (not just a type). In Go 1.26, `new(expr)` allocates a value of the type of `expr`, initializes it to the value of `expr`, and returns a `*T` pointer to it (e.g., `new(someFunc())`, `new(someStringConstant)`, `new(pkg.Const)`). If the repository targets Go 1.26 or later (e.g., module `go` directive >= 1.26 and/or build/CI uses Go >= 1.26), do not flag these usages as compilation errors.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: cmd/crossplane/dependency/dependency.go:21-25
Timestamp: 2026-05-13T18:59:33.289Z
Learning: In crossplane/cli, experimental/beta CLI command gating is done via the kong command registration struct tag `maturity:"beta"` (or similar) placed on the *parent/top-level* command fields (e.g., top-level `Cmd` fields in `cmd/crossplane/main.go`). Subcommands registered under a parent are already considered gated by that parent maturity. During code review, do not request/introduce programmatic feature flags or add a wrapper function to conditionally register commands; treat the `maturity` tag as the correct and sufficient mechanism. Instead, focus review attention on whether the appropriate maturity tag is set on the relevant parent command.
| if !opts.Direct { | ||
| xrName = names.SimpleNameGenerator.GenerateName(claimName + "-") | ||
| labels[labelClaimName] = claim.GetName() | ||
|
|
||
| labels[labelClaimNamespace] = claim.GetNamespace() | ||
| if err := xrPaved.SetValue("spec.claimRef", map[string]any{ | ||
| "apiVersion": apiVersion, | ||
| "kind": claimKind, | ||
| "name": claimName, | ||
| "namespace": claim.GetNamespace(), | ||
| }); err != nil { | ||
| return nil, errors.Wrap(err, "failed to set claimRef") | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate namespace in non-Direct mode.
In non-Direct mode, the code uses claim.GetNamespace() (lines 246, 248, 253) without validating that the namespace is non-empty. Claims are namespace-scoped resources, but if a Claim is created without a namespace (or read from a YAML without one), this could result in an XR with empty namespace in spec.claimRef and labels, which would be semantically incorrect.
Should we validate that the namespace is present when not in Direct mode? Or document the expected behavior when namespace is empty?
🛡️ Suggested validation
if !opts.Direct {
+ claimNS := claim.GetNamespace()
+ if claimNS == "" {
+ return nil, errors.New("Claim must have a namespace when generating a non-direct XR")
+ }
xrName = names.SimpleNameGenerator.GenerateName(claimName + "-")
labels[labelClaimName] = claim.GetName()
- labels[labelClaimNamespace] = claim.GetNamespace()
+ labels[labelClaimNamespace] = claimNS
if err := xrPaved.SetValue("spec.claimRef", map[string]any{
"apiVersion": apiVersion,
"kind": claimKind,
"name": claimName,
- "namespace": claim.GetNamespace(),
+ "namespace": claimNS,
}); err != nil {
return nil, errors.Wrap(err, "failed to set claimRef")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !opts.Direct { | |
| xrName = names.SimpleNameGenerator.GenerateName(claimName + "-") | |
| labels[labelClaimName] = claim.GetName() | |
| labels[labelClaimNamespace] = claim.GetNamespace() | |
| if err := xrPaved.SetValue("spec.claimRef", map[string]any{ | |
| "apiVersion": apiVersion, | |
| "kind": claimKind, | |
| "name": claimName, | |
| "namespace": claim.GetNamespace(), | |
| }); err != nil { | |
| return nil, errors.Wrap(err, "failed to set claimRef") | |
| } | |
| } | |
| if !opts.Direct { | |
| claimNS := claim.GetNamespace() | |
| if claimNS == "" { | |
| return nil, errors.New("Claim must have a namespace when generating a non-direct XR") | |
| } | |
| xrName = names.SimpleNameGenerator.GenerateName(claimName + "-") | |
| labels[labelClaimName] = claim.GetName() | |
| labels[labelClaimNamespace] = claimNS | |
| if err := xrPaved.SetValue("spec.claimRef", map[string]any{ | |
| "apiVersion": apiVersion, | |
| "kind": claimKind, | |
| "name": claimName, | |
| "namespace": claimNS, | |
| }); err != nil { | |
| return nil, errors.Wrap(err, "failed to set claimRef") | |
| } | |
| } |
🤖 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/xr/generate.go` around lines 244 - 257, When building the XR
in non-Direct mode (the block guarded by if !opts.Direct), validate that
claim.GetNamespace() is non-empty before setting labels[labelClaimNamespace] or
calling xrPaved.SetValue("spec.claimRef", ...); if the namespace is empty return
a wrapped error (e.g. "claim namespace required in non-Direct mode") so we don't
create an XR with an empty claimRef namespace; perform this check early in the
if !opts.Direct block (before using claim.GetNamespace()) and only proceed to
set labels and call xrPaved.SetValue when the namespace is present.
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
LGTM overall - left a few small comments.
Thinking to the future, it would be nice to also be able to generate a scaffold/minimal XR from an XRD (we have this in up as up example generate). Maybe we could add a --from=<claim|xrd> flag to this command, similar to how we have --from=<xr|simpleschema> on crossplane xrd generate. No need to add it now - just thinking of whether we can add in the future without breaking the current functionality.
| // Flags. | ||
| OutputFile string `help:"The file to write the generated XR YAML to. If not specified, stdout will be used." placeholder:"PATH" predictor:"file" short:"o" type:"path"` | ||
| Name string `help:"The name to use for the XR. If empty, defaults to the Claim's name (direct mode) or the Claim's name with a random suffix (non-direct)." placeholder:"NAME" type:"string"` | ||
| Kind string `help:"The kind to use for the XR. If not specified, 'X' will be prepended to the Claim's kind (e.g. Infra -> XInfra)." placeholder:"KIND" type:"string"` |
There was a problem hiding this comment.
Instead of, or maybe in addition to, this flag, it would be nice to allow an XRD to be specified, which would provide the necessary kind mapping.
|
|
||
| claim := &unstructured.Unstructured{} | ||
| if err := yaml.Unmarshal(claimData, claim); err != nil { | ||
| return errors.Wrap(err, "Unmarshalling Error") |
There was a problem hiding this comment.
I agree a bit with coderabbit on adding a little detail here, but more importantly by convention error messages should start with a lower-case letter.
| return errors.Wrap(err, "Unmarshalling Error") | |
| return errors.Wrap(err, "cannot unmarshal claim") |
| const ( | ||
| // Error messages. |
There was a problem hiding this comment.
The Crossplane style guidelines no longer suggest error message constants, preferring literals in errors.Wrap calls.
| const ( | ||
| // Error messages. | ||
| errNilInput = "input is nil" | ||
| errEmptyClaimYAML = "invalid Claim YAML: parsed object is empty" | ||
| errNoAPIVersion = "Claim has no apiVersion" | ||
| errParseAPIVersion = "failed to parse Claim APIVersion" | ||
| errNoKind = "Claim has no kind section" | ||
| errNoSpecSection = "Claim has no spec section" |
There was a problem hiding this comment.
I disagree with coderabbit here - its suggested error messages are excessive 😆.
| // ConvertClaimToXR converts a Crossplane Claim to a Composite Resource (XR). | ||
| func ConvertClaimToXR(claim *unstructured.Unstructured, opts Options) (*composite.Unstructured, error) { |
There was a problem hiding this comment.
This mostly duplicates functionality from Crossplane's claim reconciler. Could we factor some code out of the reconciler into crossplane-runtime and consume it here? Similar to how the xcrd package handles XRD->CRD conversion for both the CLI and the XRD reconciler.
Not against having this in the CLI for now - we can get this merged and refactor afterwards.
2 participants
Description of your changes
Introduce a Claim to XR converter:
crossplane xr generate claim.yaml. Similarly to other converter functions/tools, it takes a Claim either on stdin or as file argument and generates an equivalent XR YAML.The parent
crossplane xris marked as maturity "alpha", which applies to the subtree as well.This new tool can be used together with the render command as shown in the example below:
crossplane render <(crossplane xr generate claim.yaml) composition.yaml functions.yamlThe library function ConvertClaimToXR can be used by downstream tools (eg crossplane-diff) that rely on Claim to XR conversion.
The tool (and as a result the converter function, via the Options struct) is configurable and provides the following flags:
--direct: omit spec.claimRef and the equivalent labels.--name: specify an explicit XR name, skipping either the random suffix in the non-direct mode or the Claim name fallback in direct mode.--kind: override the derived "X" default.--gen-uid: populate metadata.uid with a fresh random UUID.I didn't create new docs issue/PR, we can use crossplane/docs#1088
I have:
./nix.sh flake checkto ensure this PR is ready for review.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.