render: Ignore the observed copy of the XR in --observed-resources - #256
render: Ignore the observed copy of the XR in --observed-resources#256Bham06 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe render conversion now removes an observed resource that matches the supplied composite resource by GVK, namespace, and name. Tests cover namespaced and cluster-scoped resources, unrelated resources, and empty input. ChangesObserved XR filtering
Estimated code review effort: 2 (Simple) | ~10 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.
🧹 Nitpick comments (1)
cmd/crossplane/render/convert_test.go (1)
60-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required
argsandreasonfields.Thanks for adding coverage for namespaced and cluster-scoped XRs. The table defines
xrandobserveddirectly and omits areasonfield. Nest the inputs in anargsfield, retainwant, and add a reason for each case. Include the reason in failure output.As per path instructions,
**/*_test.gomust use anargs/wantpattern and include reason fields.🤖 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/render/convert_test.go` around lines 60 - 64, Update the test table in the relevant conversion test to use an args field containing xr and observed, retain want, and add a reason field to every case. Pass each case’s reason into failure output so assertion messages identify the scenario, following the required args/want test pattern.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.
Nitpick comments:
In `@cmd/crossplane/render/convert_test.go`:
- Around line 60-64: Update the test table in the relevant conversion test to
use an args field containing xr and observed, retain want, and add a reason
field to every case. Pass each case’s reason into failure output so assertion
messages identify the scenario, following the required args/want test pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bd22eb3-3840-4cd6-aa17-eaf846038a7b
📒 Files selected for processing (2)
cmd/crossplane/render/convert.gocmd/crossplane/render/convert_test.go
A render run emits the composite resource as its first output document, so an --observed-resources file produced by a previous run usually contains a UID-less copy of the XR. Keyed by GVK, namespace and name, that copy overwrites the real XR in the engine's store and blanks its UID, which makes the ownership check drop every composed resource. The XR is already supplied as the positional argument, so drop the redundant copy when building the render request. Fixes crossplane#47 Signed-off-by: Bham06 <heu319@student.bham.ac.uk>
4b343cd to
1566195
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/crossplane/render/convert_test.go (1)
110-120: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd namespace and API-version identity cases.
Thanks for covering exact matches, cluster-scoped resources, empty input, and kind mismatches. The table does not verify that namespace and
apiVersionare also part of the match. A filter that ignores either field could pass the current tests and remove a valid observed resource. Could you add one case for each mismatch?The production contract compares GVK, namespace, and name, so each non-match should remain in the result.
Suggested additions
+ "KeepsSameNameDifferentNamespace": { + reason: "A resource with the XR's GVK and name in another namespace should be kept.", + args: args{ + xr: composite("example.org/v1", "XR", "default", "my-xr"), + observed: []composed.Unstructured{ + observed("example.org/v1", "XR", "other", "my-xr"), + }, + }, + want: []string{"example.org/v1/XR/other/my-xr"}, + }, + "KeepsSameNameDifferentAPIVersion": { + reason: "A resource with the same group, kind, namespace and name in another API version should be kept.", + args: args{ + xr: composite("example.org/v1", "XR", "default", "my-xr"), + observed: []composed.Unstructured{ + observed("example.org/v2", "XR", "default", "my-xr"), + }, + }, + want: []string{"example.org/v2/XR/default/my-xr"}, + },🤖 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/render/convert_test.go` around lines 110 - 120, Add table-driven cases in the test covering observed resources with the same GVK and name but a different namespace, and with the same kind, namespace, and name but a different apiVersion; update the expected results so each remains retained. Anchor the additions in the existing test table around “KeepsSameNameDifferentKind” and preserve the contract that matching requires apiVersion, kind, namespace, and name.
🤖 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 `@cmd/crossplane/render/convert_test.go`:
- Around line 110-120: Add table-driven cases in the test covering observed
resources with the same GVK and name but a different namespace, and with the
same kind, namespace, and name but a different apiVersion; update the expected
results so each remains retained. Anchor the additions in the existing test
table around “KeepsSameNameDifferentKind” and preserve the contract that
matching requires apiVersion, kind, namespace, and name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 181d48f1-801e-4016-80b0-ed9c74c98258
📒 Files selected for processing (1)
cmd/crossplane/render/convert_test.go
1 participant
Description of your changes
crossplane renderemits the composite resource as its first output document.When that output is fed back in via
--observed-resources— the iterativeworkflow the issue describes — the file contains a UID-less copy of the XR.
Observed resources are keyed by GVK, namespace and name, so that copy
overwrites the real XR in the engine's store and blanks its UID. The ownership
check then matches nothing, and every composed resource is silently dropped
from the output.
The XR is already supplied as the positional argument, so this drops the
redundant observed copy when building the render request.
This is the conservative fix discussed on the issue. @adamwg suggested that
rather than ignoring the observed XR we could merge it into the explicitly
supplied one, so an iterative run could feed back status fields set by a
previous render. That seems useful, but it is a behaviour change layered on top
of a currently-silent data loss bug, so I would suggest landing this first and
treating the merge semantics as a follow-up — happy to pick that up.
Covered by
TestFilterObservedXR: the observed copy is dropped for bothnamespaced and cluster-scoped XRs, a resource sharing the XR's name and
namespace but not its kind is kept, and the no-copy and empty-input cases are
left untouched.
Fixes #47
I have:
RunRan./nix.sh flake checkto ensure this PR is ready for review.go build ./...,go vet ./cmd/crossplane/render/...,gofmt, andgo test ./cmd/crossplane/render/...locally, all clean; relied on CI for the full flake check.TestFilterObservedXR.)Linked a PR or a docs tracking issue to document this change.Restores the documented behaviour of--observed-resources; no docs change needed.AddedLeft to maintainer discretion, though this may be worth backporting since it is silent data loss.backport release-x.ylabels to auto-backport this PR.