feat: Add support for old resources in validate#166
Conversation
cychiang
force-pushed
the
feat-add-validate-old-resources
branch
from
df837b5 to
642a44a
Compare
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds ChangesOld resources transition validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/crossplane/validate/cmd.go (1)
110-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThanks for adding this — but the stdin guard needs to grow with the new flag.
c.OldResourcescan also be"-"(it goes through the sameload.NewLoader), but this check only guards againstResources+Extensionsboth being stdin. If a user runs something like... resources.yaml - --old-resources -(or extensions+old-resources both-), two loaders will race to read the same stdin stream — one gets the real payload, the other silently gets nothing (or errors confusingly). Could we generalize the guard to cover all three inputs?🐛 Proposed fix generalizing the stdin guard
- if c.Resources == "-" && c.Extensions == "-" { - return errors.New("cannot use stdin for both extensions and resources") - } + stdinInputs := 0 + for _, in := range []string{c.Resources, c.Extensions, c.OldResources} { + if in == "-" { + stdinInputs++ + } + } + if stdinInputs > 1 { + return errors.New("cannot use stdin for more than one input") + }🤖 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/validate/cmd.go` around lines 110 - 113, The stdin guard in Cmd.Run only checks Resources and Extensions, but OldResources also uses load.NewLoader and can consume stdin. Update the validation in Cmd.Run to reject any combination where more than one of Resources, Extensions, or OldResources is "-" so only a single loader can read stdin, and keep the error message aligned with this broader guard.
🧹 Nitpick comments (1)
cmd/crossplane/validate/cmd.go (1)
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider enriching the help text to match the sibling
Resources/Extensionsargs.
Resourcesdocuments its accepted forms ("files, directories, or-for standard input"), butOldResources's help string doesn't mention format at all, even though it goes through the same loader. Spelling that out here would help users discover-/directory/comma-separated support without digging into the docs.♻️ Proposed help text tweak
- OldResources string `help:"Previous resource state for evaluating CEL transition rules."` + OldResources string `help:"Previous resource state for evaluating CEL transition rules, as a comma-separated list of files, directories, or '-' for standard input."`As per path instructions, "Review CLI commands for proper flag handling, help text, and error messages."
🤖 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/validate/cmd.go` at line 84, The OldResources flag help text is too vague compared with Resources and Extensions. Update the OldResources help string in the validate command definition to mention the accepted input forms handled by the shared loader, such as files, directories, comma-separated values, and - for standard input, so users can discover the supported formats directly from the CLI help.Source: Path instructions
🤖 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 `@pkg/validate/validate.go`:
- Around line 63-97: `resourceKey` is vulnerable to collisions because it
concatenates GVK, name, and namespace into a single string with hyphens, so
distinct resources can map to the same key and mispair old/new objects in
`Validate`; replace the string-based keying in `Validate` and `resourceKey` with
a struct or other unambiguous composite key built from `GroupVersionKind`,
`getResourceName`, and `GetNamespace`, and update the `oldByKey` map accordingly
so duplicate handling cannot silently match the wrong resource. If `fmt` is only
used by `resourceKey`, remove that import after the refactor.
---
Outside diff comments:
In `@cmd/crossplane/validate/cmd.go`:
- Around line 110-113: The stdin guard in Cmd.Run only checks Resources and
Extensions, but OldResources also uses load.NewLoader and can consume stdin.
Update the validation in Cmd.Run to reject any combination where more than one
of Resources, Extensions, or OldResources is "-" so only a single loader can
read stdin, and keep the error message aligned with this broader guard.
---
Nitpick comments:
In `@cmd/crossplane/validate/cmd.go`:
- Line 84: The OldResources flag help text is too vague compared with Resources
and Extensions. Update the OldResources help string in the validate command
definition to mention the accepted input forms handled by the shared loader,
such as files, directories, comma-separated values, and - for standard input, so
users can discover the supported formats directly from the CLI help.
🪄 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: 20f428bb-1258-4af1-831d-3f1b3de61f27
⛔ Files ignored due to path filters (3)
cmd/crossplane/validate/testdata/cmd/crd_transition.yamlis excluded by!**/testdata/**and included by**/*.yamlcmd/crossplane/validate/testdata/cmd/resources_transition_new.yamlis excluded by!**/testdata/**and included by**/*.yamlcmd/crossplane/validate/testdata/cmd/resources_transition_old.yamlis excluded by!**/testdata/**and included by**/*.yaml
📒 Files selected for processing (5)
cmd/crossplane/validate/cmd.gocmd/crossplane/validate/help/validate.mdcmd/crossplane/validate/validate_integration_test.gopkg/validate/validate.gopkg/validate/validate_test.go
Thread previous resource state into CEL validation so transition rules that reference oldSelf (such as immutability constraints) are evaluated. Old resources are matched to resources by GVK, name, and namespace; a resource with no match is validated as a create, leaving its transition rules skipped. Signed-off-by: Chuan-Yen Chiang <cychiang0823@gmail.com>
Signed-off-by: Chuan-Yen Chiang <cychiang0823@gmail.com>
Signed-off-by: Chuan-Yen Chiang <cychiang0823@gmail.com>
cychiang
force-pushed
the
feat-add-validate-old-resources
branch
from
903eb2b to
6b4632f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/validate/validate_test.go (1)
183-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNice regression test — consider folding it into the table-driven suite.
This is a sharp catch on the ambiguous-key collision scenario, thanks for adding it! One small thought: the repo's test convention favors table-driven tests with an
args/wantpattern (as seen elsewhere in this file), but this one is a standalone function with inline assertions. Would it make sense to fold this scenario intoTestSchemaValidate's table (or a sibling table) for consistency, or is a dedicated function clearer here given the two-resource-pair setup? Curious about the tradeoff you're seeing.As per path instructions,
**/*_test.goshould "Enforce table-driven test structure... args/want pattern."🤖 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 `@pkg/validate/validate_test.go` around lines 183 - 231, This regression test is valuable, but it should follow the repo’s table-driven test convention instead of a standalone function. Fold the ambiguous-key collision case into TestSchemaValidate’s existing args/want table (or a sibling table-driven helper) and express the two-resource-pair setup through the same pattern used elsewhere in validate_test.go, keeping the assertions aligned with the shared schema validation cases.Source: Path instructions
pkg/validate/validate.go (1)
66-79: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollision bug fixed — nice catch resolving that. One follow-up on scale.
Comparing
GroupVersionKind,namespace, andnamedirectly (instead of the old string-concatenated key) correctly eliminates the hyphen-collision bug flagged in the previous review — confirmed byTestSchemaValidateDistinguishesAmbiguousKeys. Thank you for fixing that cleanly.One thing worth considering: this is now a nested loop, so matching is O(resources × oldResources). For a CLI validating large manifest sets (e.g., a whole
compositiondirectory or generated resources) this could get slow as inputs scale. Sinceschema.GroupVersionKindis itself a comparable struct (used directly as a map key throughoutk8s.io/apimachinery), you could indexoldResourcesonce into a struct-keyed map for O(1) lookups per resource, keeping both the collision-safety and the efficiency:♻️ Proposed map-based indexing
+type oldResourceKey struct { + gvk runtimeschema.GroupVersionKind + namespace string + name string +} + result := &ValidationResult{ Resources: make([]ResourceValidationResult, 0, len(resources)), } + oldByKey := make(map[oldResourceKey]*unstructured.Unstructured, len(oldResources)) + for _, o := range oldResources { + oldByKey[oldResourceKey{o.GroupVersionKind(), o.GetNamespace(), getResourceName(o)}] = o + } for _, r := range resources { - // Find this resource's previous state, if supplied, so CEL transition - // rules (those referencing oldSelf) can be evaluated. A resource is - // matched to its old state by GroupVersionKind, namespace, and name; - // with no match oldObject stays nil and transition rules are skipped, - // exactly as on a Kubernetes create. - gvk, namespace, name := r.GroupVersionKind(), r.GetNamespace(), getResourceName(r) var oldObject map[string]any - for _, o := range oldResources { - if o.GroupVersionKind() == gvk && o.GetNamespace() == namespace && getResourceName(o) == name { - oldObject = o.Object - break - } - } + if old, ok := oldByKey[oldResourceKey{r.GroupVersionKind(), r.GetNamespace(), getResourceName(r)}]; ok { + oldObject = old.Object + } result.Resources = append(result.Resources, validateResource(ctx, r, oldObject, schemaValidators, structurals, crds)) }🤖 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 `@pkg/validate/validate.go` around lines 66 - 79, The resource-to-old-resource matching in validate.go now correctly uses GroupVersionKind, namespace, and name, but the current nested scan in validateResource lookup is O(resources × oldResources). Update the matching logic around the validateResource call to preindex oldResources into a map keyed by the comparable GroupVersionKind/namespace/name tuple, then do constant-time lookups for each resource while preserving the collision-safe behavior.
🤖 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.
Nitpick comments:
In `@pkg/validate/validate_test.go`:
- Around line 183-231: This regression test is valuable, but it should follow
the repo’s table-driven test convention instead of a standalone function. Fold
the ambiguous-key collision case into TestSchemaValidate’s existing args/want
table (or a sibling table-driven helper) and express the two-resource-pair setup
through the same pattern used elsewhere in validate_test.go, keeping the
assertions aligned with the shared schema validation cases.
In `@pkg/validate/validate.go`:
- Around line 66-79: The resource-to-old-resource matching in validate.go now
correctly uses GroupVersionKind, namespace, and name, but the current nested
scan in validateResource lookup is O(resources × oldResources). Update the
matching logic around the validateResource call to preindex oldResources into a
map keyed by the comparable GroupVersionKind/namespace/name tuple, then do
constant-time lookups for each resource while preserving the collision-safe
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: caf15e0d-82d8-44ed-98bb-cccf27cbedee
📒 Files selected for processing (2)
pkg/validate/validate.gopkg/validate/validate_test.go
1 participant
Description of your changes
Fixes crossplane/crossplane#7532
To run e2e tests:
I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.