feat(xr): Convert a Claim to XR via crossplane xr generate#13
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a new ChangesXR Generate Command
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Thanks for this contribution — would you like me to flag any specific areas to focus on during review (naming, label handling, or UID behavior)? 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
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.
tampakrap
force-pushed
the
theo/feat_claimtoxr_v2
branch
from
d6f07ac to
9ad92f0
Compare
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>
tampakrap
force-pushed
the
theo/feat_claimtoxr_v2
branch
from
9ad92f0 to
0faee7c
Compare
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
LGTM, with the follow-up plans we've already discussed in previous comments.
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.