feat(xr): Convert a Claim to XR via crossplane xr generate#13
Open
tampakrap wants to merge 1 commit into
Open
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>
5 tasks
📝 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).
Comment on lines
+136
to
+143
| 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.
Comment on lines
+244
to
+257
| 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.
Suggested change
| 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.