feat(comp): allow filtering downstream resources#322
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a --resource filter to crossplane-diff comp to restrict impact analysis to a specific set of named composites (XRs and/or Claims), with a preflight validation step and explicit surfacing of composites skipped due to Manual update policy.
Changes:
- Add
--resourceflag parsing/validation, including mutual exclusion with--namespace, and[namespace/]nameparsing. - Implement name-based composite fetching (
GetCompositesByName) and processor preflight to fail fast on globally-unmatched refs. - Introduce a new XR impact status
filtered_by_policy, including renderer handling and unit/integration test coverage.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents --resource, usage examples, and interaction with update-policy filtering. |
| cmd/diff/types/types.go | Adds ResourceRef type for stable namespace/name references. |
| cmd/diff/testutils/mocks.go | Extends mock CompositionClient with GetCompositesByName. |
| cmd/diff/testutils/mock_builder.go | Adds builder hook for configuring GetCompositesByName behavior in tests. |
| cmd/diff/renderer/structured_renderer.go | Adds XRStatusFilteredByPolicy status constant. |
| cmd/diff/renderer/comp_diff_renderer.go | Renders filtered_by_policy impacts in text output with an explicit marker and message. |
| cmd/diff/renderer/comp_diff_renderer_test.go | Adds JSON/text renderer tests and HasChanges() behavior coverage for filtered-only impacts. |
| cmd/diff/main.go | Tag-formatting-only changes in common CLI flags. |
| cmd/diff/diffprocessor/comp_processor.go | Adds resource-mode preflight, update-policy partitioning, and filtered-by-policy surfacing behavior. |
| cmd/diff/diffprocessor/comp_processor_test.go | Updates signatures and adds unit tests for --resource mode behavior and preflight failure. |
| cmd/diff/diff_integration_test.go | Extends integration harness to pass --resource and adds integration coverage for the feature. |
| cmd/diff/comp.go | Adds --resource flag, parsing, validation, and wiring into the processor call. |
| cmd/diff/comp_test.go | Adds unit tests for resource ref parsing and flag mutual exclusion validation. |
| cmd/diff/client/crossplane/composition_client.go | Adds GetCompositesByName implementation used by the preflight resolver. |
| cmd/diff/client/crossplane/composition_client_test.go | Adds unit tests for GetCompositesByName across XR/claim and error paths. |
| .requirements/20260518T224830Z_comp_resource_filter/REQUIREMENTS.md | Design/requirements doc capturing intent, behavior, and test plan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+899
to
+906
| unmatched = append(unmatched, ref) | ||
|
|
||
| continue | ||
| case !apierrors.IsNotFound(err): | ||
| return nil, nil, errors.Wrapf(err, "cannot fetch composite %s as %s", ref.String(), xrGVK) | ||
| } | ||
|
|
||
| // XR-GVK was 404. Try claim GVK if available. |
|
|
||
| return types.ResourceRef{Namespace: ns, Name: name}, nil | ||
| default: | ||
| return types.ResourceRef{}, errors.Errorf("invalid --resource value %q: expected [namespace/]name format, got %d slash-separated parts", value, len(parts)-1) |
jcogilvie
force-pushed
the
comp-resource-filter
branch
from
cf5470b to
86f4c1c
Compare
Comment on lines
+376
to
+379
| // processSingleComposition processes a single composition and builds the result. | ||
| // Returns (*CompositionDiff, error). | ||
| func (p *DefaultCompDiffProcessor) processSingleComposition(ctx context.Context, newComp *un.Unstructured, namespace string) (*renderer.CompositionDiff, error) { | ||
| // Returns (*CompositionDiff, error). When `resourceMode` is true, the function uses the | ||
| // caller-supplied `preMatched` set instead of calling FindCompositesUsingComposition, and | ||
| // surfaces update-policy-filtered composites in ImpactAnalysis with XRStatusFilteredByPolicy. |
Comment on lines
+703
to
+707
| // FindCompositesUsingComposition finds all composites (XRs and Claims) that use the specified composition. | ||
| func (c *DefaultCompositionClient) FindCompositesUsingComposition(ctx context.Context, compositionName string, namespace string) ([]*un.Unstructured, error) { | ||
| // findByListing implements default-discovery for FindComposites: list every XR (and Claim, if the | ||
| // XRD defines one) of the composition's target GVK, scoped to namespace, and filter by composition. | ||
| // Pre-existing behavior: if the composition itself isn't in the cluster (net-new), the GetComposition | ||
| // lookup fails and the error propagates to the caller, which is expected to handle "net-new" gracefully. |
Comment on lines
+841
to
+850
| // GetCompositesByName fetches the user-named composites for a composition. | ||
| // See the CompositionClient interface for the full contract. | ||
| // findByRefs implements ref-based lookup for FindComposites: each ref is resolved against the | ||
| // composition's XR GVK first, then the claim GVK on 404 if the XRD defines a claim type. Refs that | ||
| // fail both lookups, or that exist but reference a different composition, are silently dropped from | ||
| // the result. NotFound responses are tolerated; other errors propagate. | ||
| // | ||
| // Step 4 will split this into resolveCompositeTypes + lookupRef. | ||
| // findByRefs implements ref-based lookup for FindComposites: each ref is resolved against the | ||
| // composition's XR GVK first, then the claim GVK on 404 if the XRD defines a claim type. |
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
- lookupRef: when XR GVK returns a hit but the resource references a different composition, fall through to the claim GVK fallback instead of returning nil. This fixes a bug where same-name XR+Claim collisions in v2 namespaces could not be resolved by --resource. Refactor extracts the per-GVK probe into a tryLookupAtGVK helper called twice. - parseResourceRef: error message said "got %d slash-separated parts" but reported len(parts)-1 (the slash count). Reword to "got %d slashes" so the message matches the value. Adds a new lookupRef test case for the same-name collision path. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Stale references after the Step 3 method rename / Step 5 hoist: - composition_client.go: drop the "GetCompositesByName fetches..." header + duplicate sentence + completed "Step 4 will split..." TODO above findByRefs; drop the orphaned "FindCompositesUsingComposition finds..." line above findByListing. - comp_processor.go: rewrite the processSingleComposition doc to reference its actual params (affectedXRs, surfaceFiltered) and drop the resourceMode/preMatched/FindCompositesUsingComposition mentions. Update two inline comments inside collectXRDiffs that still named the removed FindCompositesUsingComposition method. Also use a switch instead of if/else in partitionXRsByUpdatePolicy now that we want to keep both the kept and dropped sets (consistent with the switch style used elsewhere in this file). Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
jcogilvie
force-pushed
the
comp-resource-filter
branch
from
86f4c1c to
baa50b5
Compare
The five hand-rolled subtests in TestDefaultCompDiffProcessor_DiffComposition_ResourceMode fall naturally into two tables and one standalone: - DispatchesToCorrectFindMode: table over (resources, namespace) verifying DiffComposition routes to ref-lookup when resources is non-empty and to default-discovery otherwise. Replaces EmptyResources_FallsBackToDefaultDiscovery and ResourceMode_AllMatch_UsesRefLookup. - SurfaceFilteredControlsImpactAnalysis: table over surfaceFiltered verifying that processSingleComposition surfaces Manual-policy XRs in ImpactAnalysis with XRStatusFilteredByPolicy only when surfaceFiltered=true. Replaces ResourceMode_ManualPolicyMatchSurfacedAsFiltered (drops a redundant DiffComposition pre-call whose result was discarded) and DefaultDiscoveryMode_ManualPolicyNotInImpactAnalysis. - ResourceMode_GloballyUnmatched_FailsFastNoRender stays standalone — its shape (assert on error string + stdout state) is unique. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Comment on lines
+732
to
734
| // WithResourcesForComposition sets FindComposites (default-discovery mode) to return specific resources | ||
| // for a given composition name and namespace. Refs-mode calls fall through to the "not implemented" default. | ||
| func (b *MockCompositionClientBuilder) WithResourcesForComposition(compositionName, namespace string, resources []*un.Unstructured) *MockCompositionClientBuilder { |
Comment on lines
+748
to
750
| // WithFindResourcesError sets FindComposites (default-discovery mode) to return an error. Refs-mode calls | ||
| // fall through to the "not implemented" default. | ||
| func (b *MockCompositionClientBuilder) WithFindResourcesError(errMsg string) *MockCompositionClientBuilder { |
|
|
||
| ### R1 — `ResourceRef` is replaced by `k8s.io/apimachinery/pkg/types.NamespacedName` | ||
|
|
||
| The `ResourceRef` struct and its `String()` method are removed from `cmd/diff/types/types.go`. The `CompositionProvider` declaration in that file stays (it's used elsewhere). Every reference to `dtypes.ResourceRef` in production and test code switches to `k8stypes.NamespacedName` (alias `k8stypes "k8s.io/apimachinery/pkg/types"`). A `formatRef(NamespacedName) string` helper is added in `cmd/diff/comp.go` to preserve the bare-name rendering for cluster-scoped refs in user-facing strings (error messages, logs). |
Stale documentation that didn't match the code: - mock_builder.go: WithResourcesForComposition / WithFindResourcesError said refs-mode calls "fall through to the not-implemented default," but they actually return an explicit error. Reword to describe the actual behavior and point users at WithFindComposites for both modes. - REQUIREMENTS.md (R1, AC1.4, Implementation): said formatRef lives in cmd/diff/comp.go, but it's in cmd/diff/diffprocessor/comp_processor.go (the only call site — preflightResourceRefs constructs the user-facing unmatched-refs error message there). Update to match. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
ctx captured from the outer *testing.T is bound to the parent test's lifecycle, which extends past each subtest. Using t.Context() inside the t.Run closure (where t shadows the outer t) gives a context bound to the subtest's lifecycle — properly cancelled when the subtest ends, correct for any future t.Parallel() conversion, and aligns context cancellation with t.Cleanup ordering. Affected tests: - composition_client_test.go: FindComposites_WithRefs, FindComposites_DefaultDiscovery, resolveCompositeTypes, lookupRef - comp_processor_test.go: DiffComposition_ResourceMode Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
WithResourcesExist already keys resources by apiVersion/kind/namespace/name (via MakeDiffKey) and falls back to NotFound for unknown lookups, which is exactly the pattern several FindComposites_WithRefs and lookupRef cases were rolling by hand. Replace the inline WithGetResource closures with WithResourcesExist for the matched-or-404 cases. Add a new WithGetResourceError helper (mirrors the naming of the existing WithListResourcesFailure / WithFindResourcesError) for the transport-error path: WithResourcesExist and WithResourceNotFound both produce 404s, so non-NotFound errors still need a dedicated builder. Two test cases (TransportErrorPropagated, XRTransportError_Propagates) now use it. For the same-name XR+Claim collision case, hoist the colliding XR and Claim into named locals (collisionXR, collisionClaim) so WithResourcesExist takes both directly rather than the test embedding a switch over GVK. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Comment on lines
+214
to
+220
| default: | ||
| typedComp := &apiextensionsv1.Composition{} | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(comp.Object, typedComp); err != nil { | ||
| return false, errors.Wrapf(err, "cannot convert composition %s to typed for default discovery", compositionID) | ||
| } | ||
|
|
||
| discovered, findErr := p.compositionClient.FindComposites(ctx, typedComp, dtypes.FindCompositesOptions{Namespace: namespace}) |
joinRefs was a one-line passthrough to strings.Join used at exactly one call site. %v prints the []string as [a b c] — fine for an error message, the slice already contains user-formatted refs from formatRef. Drops the helper and the strings import. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
…pers Adds two semantic mock builder helpers for FindComposites, mirroring the WithResourcesForComposition / WithFindResourcesError pattern but covering the remaining cells of the (mode × outcome) matrix: - WithCompositesByRef(matched ...): refs-mode only; default-discovery calls return an explicit "this helper is refs-only" error. Mirror of WithResourcesForComposition for the refs side. - WithNoMatchingComposites(): both modes return (nil, nil). For "nothing matches anywhere" tests. Mirrors the existing WithNoMatchingComposition (singular) for the FindMatchingComposition method. Refactor uses: - DispatchesToCorrectFindMode: drop the spy closure that counted defaultCalls/refCalls. Instead, EmptyResources_DefaultDiscovery uses WithResourcesForComposition (errors on refs-mode) and WithResources_RefLookup uses WithCompositesByRef (errors on default-mode). Wrong-mode dispatch surfaces as a non-nil error from DiffComposition. Drops two int fields from the table; CompositionClient is stored directly (no closure wrapper — mocks have no per-subtest state to reset). - ResourceMode_GloballyUnmatched_FailsFastNoRender: switch from the inline (nil, nil) closure to WithNoMatchingComposites(). Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Three bare if-assertions become a small table. Adds a Negative_HasSuffix case (pluralize returns "s" for any count != 1, so negatives should render with the suffix too) since the function does treat them that way and the test now explicitly documents it. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Comment on lines
+150
to
+167
| parts := strings.Split(trimmed, "/") | ||
| switch len(parts) { | ||
| case 1: | ||
| return k8stypes.NamespacedName{Name: parts[0]}, nil | ||
| case 2: | ||
| ns, name := parts[0], parts[1] | ||
| if ns == "" { | ||
| return k8stypes.NamespacedName{}, errors.Errorf("invalid --resource value %q: namespace must not be empty (use bare name for cluster-scoped composites)", value) | ||
| } | ||
|
|
||
| if name == "" { | ||
| return k8stypes.NamespacedName{}, errors.Errorf("invalid --resource value %q: name must not be empty", value) | ||
| } | ||
|
|
||
| return k8stypes.NamespacedName{Namespace: ns, Name: name}, nil | ||
| default: | ||
| return k8stypes.NamespacedName{}, errors.Errorf("invalid --resource value %q: expected [namespace/]name format, got %d slashes", value, len(parts)-1) | ||
| } |
Comment on lines
+950
to
+958
| switch { | ||
| case apierrors.IsNotFound(err): | ||
| c.logger.Debug("ref not found at GVK", | ||
| "ref", ref.String(), "gvk", gvk.String(), "via", kindLabel) | ||
|
|
||
| return nil, nil | ||
| case err != nil: | ||
| return nil, errors.Wrapf(err, "cannot fetch composite %s as %s", ref.String(), gvk) | ||
| } |
The parser and formatter for the --resource flag's [namespace/]name strings are inverses of each other. They were tacked onto whichever file each was first called from (parser in package main's comp.go, formatter in the diffprocessor's comp_processor.go) but neither file was the right home — the CLI layer can't be imported by the processor and the domain layer shouldn't own CLI-arg I/O. Promote them into a dedicated cmd/diff/ref package: - ref.Parse(string) (NamespacedName, error): single value - ref.ParseAll([]string) ([]NamespacedName, error): slice; first error stops parsing with no partial result - ref.Format(NamespacedName) string: inverse of Parse, preserves the user's original [namespace/]name spelling Update callers: - comp.go: replace the parse-loop with a single ref.ParseAll call; drop the now-unused k8stypes import. - comp_processor.go: formatRef(r) -> ref.Format(r); drop the local function. Tests for all three live in cmd/diff/ref/ref_test.go (TestParse, TestParseAll, TestFormat). The previous TestParseResourceRef in comp_test.go and TestFormatRef in comp_processor_test.go are removed — their content is preserved verbatim in the new package's tests. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Inputs like --resource='default / my-claim' currently pass through Parse as namespace "default " (with trailing space) and fail later during cluster lookup with a confusing error. Trim each component after the slash split so the failure either doesn't happen (clean refs) or surfaces immediately at parse time as an empty-name/empty-namespace error. Kubernetes names and namespaces can't contain whitespace, so per-part trimming never loses information. Adds two test cases (whitespace around slash, whitespace around bare cluster-scoped name). Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
NamespacedName.String() always renders "namespace/name" — so a cluster-scoped --resource value like "my-xr" came back through non-NotFound transport errors as "cannot fetch composite /my-xr as ...", contradicting the documented --resource format that allows bare names. Switch the user-facing errors.Wrapf in tryLookupAtGVK to use ref.Format, which preserves the user's original CLI spelling (bare "name" for cluster-scoped, "namespace/name" for namespaced) — same convention as the unmatched-refs preflight error. Debug log fields keep n.String() — structured logging consumers benefit from the unambiguous "/foo" form. Renames the local `ref` parameter (and findByRefs's loop variable) to `n` to free the `ref` identifier for the package import. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
…ry conversion) DiffComposition's default-discovery branch was converting the user-supplied unstructured Composition into a typed *apiextensionsv1.Composition just to call FindComposites — but the default-discovery path (findByListing) only ever uses comp.GetName(). The conversion introduced a failure mode (unknown fields / version skew in the input file would abort the comp diff) for data the listing path doesn't actually need. Push the typed conversion down into the client, where only the refs-mode path (resolveCompositeTypes / findByRefs needs spec.compositeTypeRef) actually converts. Default-discovery now reads comp.GetName() from the unstructured directly. API changes: - CompositionClient.FindComposites: comp *apiextensionsv1.Composition → comp *un.Unstructured. findByRefs internally converts to typed for Spec access; findByListing reads name from the unstructured. - MockCompositionClient.FindCompositesFn signature mirrors the interface. - All five mock builders (WithFindComposites, WithResourcesForComposition, WithFindResourcesError, WithCompositesByRef, WithNoMatchingComposites) take *un.Unstructured for the comp parameter. Caller updates: - DiffComposition's default-discovery branch drops the inline FromUnstructured call and the hard-error return path that came with it. - preflightResourceRefs drops its FromUnstructured (the client now owns conversion in refs mode). Tests: - TestDefaultCompositionClient_FindComposites_WithRefs: comp field type changes to *un.Unstructured; build via BuildAsUnstructured(). - TestDefaultCompositionClient_FindComposites_DefaultDiscovery: keeps a typed copy for the GetComposition cache, passes the unstructured to FindComposites. - TestDefaultCompDiffProcessor_findResourcesUsingComposition: build a minimal *un.Unstructured with the composition name; drops metav1 import. REQUIREMENTS.md updated to reflect the new cmd/diff/ref package as the home of Parse/ParseAll/Format (was: formatRef in comp_processor.go). Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
| @@ -0,0 +1,321 @@ | |||
| # Refactor: clean up the `--resource` filter architecture | |||
|
|
|||
| Companion plan file: `~/.claude/plans/peppy-sniffing-valley.md`. This document is the authoritative source during implementation. | |||
Comment on lines
+49
to
+51
| // When opts.Refs is empty, it performs default discovery scoped to opts.Namespace, listing all | ||
| // XRs (and Claims, if the XRD defines them) of the appropriate GVK and filtering by composition. | ||
| // NotFound responses are tolerated; non-NotFound transport errors propagate. |
- REQUIREMENTS.md: drop the reference to ~/.claude/plans/peppy-sniffing-valley.md.
That path is local to my dev environment and won't exist for other contributors;
the in-repo REQUIREMENTS.md is now the only authoritative source.
- composition_client.go: clarify the FindComposites doc comment around what
"tolerated" actually means. The previous wording lumped two different NotFound
scenarios together. Spell out:
* refs-mode: per-ref XR/Claim NotFounds are tolerated (silently omitted).
* default-discovery: XR/Claim list errors are tolerated, but a NotFound from
the composition cache lookup (net-new composition) propagates as the
returned error — callers treat that as "no affected XRs" rather than as
a hard failure.
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
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
Fixes #321
I have:
earthly -P +reviewableto ensure this PR is ready for review.[ ] Followed the API promotion workflow if this PR introduces, removes, or promotes an API.Need help with this checklist? See the cheat sheet.