feat(xr): Introduce xr patch --xrd#61
Conversation
New subcommand that does the same as `render --xrd` but without running render. Useful when we want to patch our XR with its default values from the XRD. It is using the same code as `render --xrd`, but that codeis moved under `xr/xrd.go` and `render --xrd` imports/uses it. Signed-off-by: Theo Chatzimichos <tampakrap@gmail.com>
|
Warning Review limit reached
More reviews will be available in 10 minutes and 47 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 (6)
📝 WalkthroughWalkthroughThis PR moves XRD defaulting into a new xr package, adds an ChangesXR Patch Command Feature
Sequence DiagramsequenceDiagram
participant CLI as patchCmd.Run
participant Loader as XRD Loader
participant XrPkg as xrcmd.ApplyXRDDefaults
participant Structural as structuraldefaulting.Default
participant Output as Writer
CLI->>Loader: load XRD from --xrd path
CLI->>XrPkg: call ApplyXRDDefaults(xr, xrd)
XrPkg->>Structural: convert OpenAPI schema -> structural schema, apply defaults to XR map
Structural-->>XrPkg: XR mutated with defaults
XrPkg-->>CLI: return
CLI->>Output: marshal and write patched YAML (stdout/file)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 1
🧹 Nitpick comments (2)
cmd/crossplane/xr/patch.go (1)
61-63: 💤 Low valueTiny wording nit on the "at least one of" message.
With only
--xrdavailable today, "at least one of--xrd" reads a little oddly. Since the help text already explains more flags are coming, maybe something simpler like"no patching flag provided: --xrd must be set"reads better for now? Totally your call — happy to defer if you'd rather keep the forward-looking phrasing.🤖 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/patch.go` around lines 61 - 63, Update the error message emitted when c.XRD is empty in patch.go to use simpler wording: replace the current "no patching flag provided: at least one of --xrd must be set" with a clearer message such as "no patching flag provided: --xrd must be set" so the check for c.XRD reads naturally; locate the check that references c.XRD and change only the string returned by errors.New(...) accordingly.cmd/crossplane/xr/xrd.go (1)
69-76: ⚡ Quick winCould we wrap these two errors with a bit of user-facing context?
The
errors.ErrorfAPI-version case is lovely and descriptive, but theConvert_...andschema.NewStructuralerrors are returned bare here. Since this is the canonical defaulting path for bothrender --xrdand the newxr patch, a user hitting a malformed schema would just see a low-level conversion error with no hint about which XRD/version caused it. Wrapping with crossplane-runtime'serrors.Wrapwould keep it consistent with the rest of the file.♻️ Suggested wrapping
- if err := extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(version.Schema.OpenAPIV3Schema, &k, nil); err != nil { - return err - } - - crdWithDefaults, err := schema.NewStructural(&k) - if err != nil { - return err - } + if err := extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(version.Schema.OpenAPIV3Schema, &k, nil); err != nil { + return errors.Wrapf(err, "cannot convert OpenAPI v3 schema for version %q", apiVersion) + } + + crdWithDefaults, err := schema.NewStructural(&k) + if err != nil { + return errors.Wrapf(err, "cannot build structural schema for version %q", apiVersion) + }As per coding guidelines: "Use crossplane-runtime/pkg/errors for wrapping" and "Ensure all error messages are meaningful to end users ... include context about what the user was trying to do."
🤖 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/xrd.go` around lines 69 - 76, Wrap the low-level errors returned from the conversion and structural construction calls with user-facing context using crossplane-runtime/pkg/errors.Wrap (or errors.Wrapf) so callers see which XRD and version failed; specifically, when calling extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(version.Schema.OpenAPIV3Schema, &k, nil) and when calling schema.NewStructural(&k), wrap each returned err with a descriptive message that includes the XRD name/version context (to match the existing errors.Errorf style used elsewhere) before returning.
🤖 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/xrd.go`:
- Around line 65-69: The code dereferences version.Schema.OpenAPIV3Schema before
checking for a nil Schema; update DefaultValues (the routine using version) to
validate that version.Schema is non-nil and return a clear error if it is nil
instead of panicking. Specifically, before calling
extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps with
version.Schema.OpenAPIV3Schema, add a guard that checks if version.Schema == nil
and return an errors.Errorf (or wrap) stating the CRD version lacks a Schema;
keep references to Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps,
version.Schema.OpenAPIV3Schema, and the
DefaultValues/ApplyXRDDefaults/xcrd.ForCompositeResource call sites in mind so
callers that already populate Schema remain unchanged.
---
Nitpick comments:
In `@cmd/crossplane/xr/patch.go`:
- Around line 61-63: Update the error message emitted when c.XRD is empty in
patch.go to use simpler wording: replace the current "no patching flag provided:
at least one of --xrd must be set" with a clearer message such as "no patching
flag provided: --xrd must be set" so the check for c.XRD reads naturally; locate
the check that references c.XRD and change only the string returned by
errors.New(...) accordingly.
In `@cmd/crossplane/xr/xrd.go`:
- Around line 69-76: Wrap the low-level errors returned from the conversion and
structural construction calls with user-facing context using
crossplane-runtime/pkg/errors.Wrap (or errors.Wrapf) so callers see which XRD
and version failed; specifically, when calling
extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(version.Schema.OpenAPIV3Schema,
&k, nil) and when calling schema.NewStructural(&k), wrap each returned err with
a descriptive message that includes the XRD name/version context (to match the
existing errors.Errorf style used elsewhere) before returning.
🪄 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: 2bcc940d-cc03-4d19-b63a-c75c1e5528b4
📒 Files selected for processing (7)
cmd/crossplane/render/xr/cmd.gocmd/crossplane/render/xrd.gocmd/crossplane/xr/help/patch.mdcmd/crossplane/xr/patch.gocmd/crossplane/xr/xr.gocmd/crossplane/xr/xrd.gocmd/crossplane/xr/xrd_test.go
💤 Files with no reviewable changes (1)
- cmd/crossplane/render/xrd.go
| if version == nil { | ||
| return errors.Errorf("the specified API version '%s' does not exist in the XRD", apiVersion) | ||
| } | ||
|
|
||
| if err := extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(version.Schema.OpenAPIV3Schema, &k, nil); err != nil { |
There was a problem hiding this comment.
Guard against a nil version.Schema before dereferencing?
Since DefaultValues is exported as the "lower-level routine for callers that already have a CRD in hand," a CRD version without a Schema would panic at version.Schema.OpenAPIV3Schema on Line 69. The ApplyXRDDefaults path via xcrd.ForCompositeResource always populates it, so this is purely defensive for direct callers — but a small nil check would turn a panic into a friendly error.
🛡️ Suggested guard
if version == nil {
return errors.Errorf("the specified API version '%s' does not exist in the XRD", apiVersion)
}
+
+ if version.Schema == nil || version.Schema.OpenAPIV3Schema == nil {
+ return errors.Errorf("the specified API version '%s' has no schema in the XRD", apiVersion)
+ }📝 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 version == nil { | |
| return errors.Errorf("the specified API version '%s' does not exist in the XRD", apiVersion) | |
| } | |
| if err := extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(version.Schema.OpenAPIV3Schema, &k, nil); err != nil { | |
| if version == nil { | |
| return errors.Errorf("the specified API version '%s' does not exist in the XRD", apiVersion) | |
| } | |
| if version.Schema == nil || version.Schema.OpenAPIV3Schema == nil { | |
| return errors.Errorf("the specified API version '%s' has no schema in the XRD", apiVersion) | |
| } | |
| if err := extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(version.Schema.OpenAPIV3Schema, &k, nil); err != nil { |
🤖 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/xrd.go` around lines 65 - 69, The code dereferences
version.Schema.OpenAPIV3Schema before checking for a nil Schema; update
DefaultValues (the routine using version) to validate that version.Schema is
non-nil and return a clear error if it is nil instead of panicking.
Specifically, before calling
extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps with
version.Schema.OpenAPIV3Schema, add a guard that checks if version.Schema == nil
and return an errors.Errorf (or wrap) stating the CRD version lacks a Schema;
keep references to Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps,
version.Schema.OpenAPIV3Schema, and the
DefaultValues/ApplyXRDDefaults/xcrd.ForCompositeResource call sites in mind so
callers that already populate Schema remain unchanged.
| // | ||
| // Callers starting from an XRD should prefer ApplyXRDDefaults; this is the | ||
| // lower-level routine for callers that already have a CRD in hand. | ||
| func DefaultValues(xr map[string]any, apiVersion string, crd extv1.CustomResourceDefinition) error { |
There was a problem hiding this comment.
I kept the same name and code that it was there. But maybe we should rename this function to ApplyCRDDefaults to match the other? If we do that though, we might break someone's setup that might be using it.
There was a problem hiding this comment.
We'll already have broken them by moving it from cmd/crossplane/render to cmd/crossplane/xrd, so I'd be in favor of renaming as well.
Might also be a good time to move this code out of cmd/ since multiple commands rely on it. Maybe a new exported package like pkg/xr or pkg/composite.
There was a problem hiding this comment.
good idea, what is your preference?
There was a problem hiding this comment.
renamed and both moved to pkg/xr. I chose that to avoid potential confusion/conflicts with crossplane-runtime/pkg/resource/unstructured/composite
| // | ||
| // Callers starting from an XRD should prefer ApplyXRDDefaults; this is the | ||
| // lower-level routine for callers that already have a CRD in hand. | ||
| func DefaultValues(xr map[string]any, apiVersion string, crd extv1.CustomResourceDefinition) error { |
There was a problem hiding this comment.
We'll already have broken them by moving it from cmd/crossplane/render to cmd/crossplane/xrd, so I'd be in favor of renaming as well.
Might also be a good time to move this code out of cmd/ since multiple commands rely on it. Maybe a new exported package like pkg/xr or pkg/composite.
| OutputFile string `help:"The file to write the patched XR YAML to. Defaults to stdout." placeholder:"PATH" predictor:"file" short:"o" type:"path"` | ||
|
|
||
| // Patching flags. | ||
| XRD string `help:"A YAML file specifying the CompositeResourceDefinition (XRD) whose schema defaults are applied to the XR." name:"xrd" placeholder:"PATH" predictor:"file" type:"path"` |
There was a problem hiding this comment.
Can we give this a more descriptive name like --defaults-from-xrd or just --xrd-defaults? It's not really clear from the flag name --xrd that it indicates an action.
There was a problem hiding this comment.
I chose xr patch --xrd to be the same as render --xrd, which is not descriptive either, but people know it very well. In theory, in the future passing the XRD could mean additional things apart from applying the defaults. Furthermore, I don't expect this command to be heavily used, I believe it will be used only for debugging purposes. The libraries behind this command though are going to be used in the upcoming test subcommand, which will have patch.xrd as defined in our design doc.
That said, I'm fine using a more descriptive name to the flag, I just wanted to give that background before we take a decision though.
Signed-off-by: Theo Chatzimichos <tampakrap@gmail.com>
tampakrap
force-pushed
the
theo/feat_patchxr_xrd
branch
from
902fc2c to
46ba7f1
Compare
tampakrap
force-pushed
the
theo/feat_patchxr_xrd
branch
2 times, most recently
from
4efe1c0 to
66f6e70
Compare
Signed-off-by: Theo Chatzimichos <tampakrap@gmail.com>
tampakrap
force-pushed
the
theo/feat_patchxr_xrd
branch
from
66f6e70 to
f10f6f6
Compare
2 participants
Description of your changes
New subcommand that does the same as
render --xrdbut without running render. Useful when we want to patch our XR with its default values from the XRD.It is using the same code as
render --xrd, but that codeis moved underxr/xrd.goandrender --xrdimports/uses it.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.