xrd: Handle v2 XRDs in crossplane xrd convert#122
Conversation
Previously, the `crossplane xrd convert` command accepted only v1 XRDs, even though the conversion logic works fine for v2 XRDs as well. Update the GVK check to include v2 so that the command works for either XRD version. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
📝 WalkthroughWalkthroughThe ChangesXRD Convert v2 GVK Support
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
🚥 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.
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.go (1)
73-87:⚠️ Potential issue | 🟡 MinorAdd test coverage for v2 XRD input to verify struct compatibility.
The code now accepts v2 XRDs via the GVK check (lines 78–81), but I notice the tests only cover v1 XRD structs. The comment in the test says "A v2 cluster-scoped XRD," but it's actually constructing a v1 struct with a v1 scope value—not testing v2 YAML input.
Since the code unmarshals directly into a
apiextensionsv1.CompositeResourceDefinitionstruct and then passes it to thexcrdfunctions, it would be helpful to verify that:
- v2 YAML successfully unmarshals into the v1 Go struct without losing any v2-specific fields
- The downstream
xcrd.ForCompositeResourceandxcrd.ForCompositeResourceClaimcalls handle the data correctly when it came from a v2 sourceCould you add a test case that unmarshals actual v2 XRD YAML? This would confirm the v2 support works end-to-end.
🤖 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 73 - 87, Add a test case that uses actual v2 XRD YAML input (not v1 YAML with a misleading v2 comment) to verify that v2 YAML successfully unmarshals into the apiextensionsv1.CompositeResourceDefinition struct without data loss and that the downstream toCRDs function correctly processes the data from v2 sources. This test should confirm end-to-end v2 support by exercising the same code path as the updated GVK validation check for apiextensionsv2.CompositeResourceDefinitionGroupVersionKind.
🧹 Nitpick comments (1)
cmd/crossplane/xrd/convert.go (1)
78-85: ⚡ Quick winConsider a more user-friendly error message format.
The current error message uses
%vto format the GVK slice, which will produce verbose Go struct output like:input is not one of [{apiextensions.crossplane.io v1 CompositeResourceDefinition} {apiextensions.crossplane.io v2 CompositeResourceDefinition}]; got ...This format includes technical Go struct notation that may confuse CLI users. Per the coding guidelines, CLI error messages should avoid technical jargon and be meaningful to end users.
Would something like this be clearer?
return errors.Errorf("input must be a CompositeResourceDefinition with apiVersion apiextensions.crossplane.io/v1 or v2; got %s", xrd.GroupVersionKind())Or even more concise:
return errors.Errorf("input must be a v1 or v2 XRD; got %s", xrd.GroupVersionKind().GroupVersion().String()+"/"+xrd.GroupVersionKind().Kind)Thank you for expanding XRD support to v2!
🤖 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 78 - 85, Replace the error message in the XRD GroupVersionKind validation check (around the errors.Errorf call with the xrdGVKs comparison) to provide a clearer, more user-friendly message that avoids using %v to format the slice of GroupVersionKind structs. Instead of showing verbose Go struct notation, craft a message that clearly states in plain language that the input must be a CompositeResourceDefinition with either v1 or v2 apiVersion, then include the actual GroupVersionKind that was received. This makes the error meaningful to CLI users rather than exposing technical Go struct details.Source: Coding guidelines
🤖 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.go`:
- Around line 73-87: Add a test case that uses actual v2 XRD YAML input (not v1
YAML with a misleading v2 comment) to verify that v2 YAML successfully
unmarshals into the apiextensionsv1.CompositeResourceDefinition struct without
data loss and that the downstream toCRDs function correctly processes the data
from v2 sources. This test should confirm end-to-end v2 support by exercising
the same code path as the updated GVK validation check for
apiextensionsv2.CompositeResourceDefinitionGroupVersionKind.
---
Nitpick comments:
In `@cmd/crossplane/xrd/convert.go`:
- Around line 78-85: Replace the error message in the XRD GroupVersionKind
validation check (around the errors.Errorf call with the xrdGVKs comparison) to
provide a clearer, more user-friendly message that avoids using %v to format the
slice of GroupVersionKind structs. Instead of showing verbose Go struct
notation, craft a message that clearly states in plain language that the input
must be a CompositeResourceDefinition with either v1 or v2 apiVersion, then
include the actual GroupVersionKind that was received. This makes the error
meaningful to CLI users rather than exposing technical Go struct details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7f75b5c-554f-41b7-8e58-d357eb57228e
📒 Files selected for processing (1)
cmd/crossplane/xrd/convert.go
2 participants
Description of your changes
Previously, the
crossplane xrd convertcommand accepted only v1 XRDs, even though the conversion logic works fine for v2 XRDs as well. Update the GVK check to include v2 so that the command works for either XRD version.I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Added or updated unit tests.- [ ] Linked a PR or a docs tracking issue to document this change.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.