fix: use existing name for resource when looking up, in case composition changes it#290
Merged
fix: use existing name for resource when looking up, in case composition changes it#290
Conversation
…ion changes it Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves XR diff accuracy by preserving the identity (especially metadata.name) of already-existing composed resources found via Crossplane identifiers, preventing false “removed” diffs when compositions transform names. It also changes diff processing behavior to fail the XR diff when key phases (non-removal diffs or removal detection) error, so permission/lookup failures don’t silently produce misleading output.
Changes:
- Preserve existing resource identity even when
generateNameis empty (supports nested XRs with composition-mutated names). - Treat diff-calculation and removal-detection errors as fatal for the XR being processed, and log failures at a more visible level.
- Add/update unit tests to cover the identity preservation and surfaced error behavior; add Serena memory docs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/diff/diffprocessor/diff_processor.go | Makes per-resource failures more visible and fails the XR diff when diff/removal detection can’t be computed reliably. |
| cmd/diff/diffprocessor/diff_processor_test.go | Updates expectations for new error messages and verifies surfaced failure when resource-tree lookup fails. |
| cmd/diff/diffprocessor/diff_calculator.go | Preserves existing resource identity without requiring generateName. |
| cmd/diff/diffprocessor/diff_calculator_test.go | Adds coverage for identity preservation when only name is set (no generateName). |
| .serena/memories/suggested_commands.md | Adds developer command suggestions for build/test/e2e workflows. |
| .serena/memories/style_and_conventions.md | Documents local style/convention guidelines related to failing on errors and output expectations. |
| .serena/memories/project_overview.md | Adds a high-level project overview and architecture notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
…or XRDiff Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves diff accuracy and debuggability in crossplane-diff when composed resources (notably nested XRs) have their names altered by compositions, and when RBAC prevents dry-run apply or resource-tree lookup from succeeding.
Changes:
- Preserve existing resource identity (name / generateName / composite label) for diffing even when
generateNameis empty. - Stop swallowing “partial” diff and removal-detection errors; instead fail the XR diff so users get a clear surfaced error.
- Emit structured processing errors to stderr (in addition to including them in structured output), with accompanying unit tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cmd/diff/diffprocessor/diff_calculator.go |
Preserves identity for any existing resource with a name; wraps resource-tree errors with context. |
cmd/diff/diffprocessor/diff_calculator_test.go |
Adds coverage for identity preservation when generateName is empty (nested XR scenario). |
cmd/diff/diffprocessor/diff_processor.go |
Surfaces diff/removal-detection errors by failing the XR; prints OutputErrors to stderr. |
cmd/diff/diffprocessor/diff_processor_test.go |
Adds stderr error-output test; updates expectations to require surfaced removal-detection errors. |
.serena/memories/suggested_commands.md |
Adds local workflow command references. |
.serena/memories/style_and_conventions.md |
Records intended style/error-handling conventions. |
.serena/memories/project_overview.md |
Documents high-level project overview for contributors/tools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+230
to
+233
| // Emit detailed errors to stderr for human visibility alongside structured output. | ||
| // This ensures CI logs show actual error details even when using JSON/YAML output. | ||
| for _, outputErr := range outputErrors { | ||
| _, _ = fmt.Fprintln(p.config.Stderr, outputErr.FormatError()) |
Collaborator
Author
There was a problem hiding this comment.
Going to do this in another PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Use existing resource name to render diff when crossplane labels match
Fixes #289
Fixes #291
I have:
earthly -P +reviewableto ensure this PR is ready for review.- [ ] Added or updated e2e tests.- [ ] Documented this change as needed.- [ ] Followed the API promotion workflow if this PR introduces, removes, or promotes an API.Need help with this checklist? See the cheat sheet.