[FEAT] Enable crossplane-diff to render k8s resources

[aribeiro94]

No description provided.

[Copilot]

🟡 Changes recommended

The new discovery-based scope checks can trigger many repeated discovery API calls (one per resource), which is likely to cause unnecessary performance/load without adding caching.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR extends crossplane-diff’s ability to handle/render built-in Kubernetes resource types (e.g., Secret, Namespace) by avoiding CRD-only assumptions and preferring discovery-based scope resolution where appropriate.

Changes:

• Update schema validation to skip built-in (non-CRD) resource types while still applying CRD defaults and enforcing scope constraints.
• Add discovery-based scope resolution (with CRD fallback) in both schema validation and namespace-stripping logic.
• Update factories and tests to plumb a ResourceClient into schema validation and cover the new behavior.

File summaries

File	Description
cmd/diff/testdata/diff/resources/external-resource-configmap-other-namespace.yaml	Fixture tweak (leading blank line) for the cross-namespace ExtraResources reproduction data set.
cmd/diff/diffprocessor/schema_validator.go	Skip CRD schema validation for built-ins; use discovery for scope resolution with CRD fallback.
cmd/diff/diffprocessor/schema_validator_test.go	Add/update tests for skipping built-in schema validation and using discovery for scope.
cmd/diff/diffprocessor/processor_config.go	Extend SchemaValidator factory signature to accept a ResourceClient.
cmd/diff/diffprocessor/diff_processor.go	Use discovery (with CRD fallback) to determine resource scope when stripping namespaces from cluster-scoped resources.
cmd/diff/diffprocessor/diff_processor_test.go	Add coverage ensuring namespace stripping uses discovery for built-in types.

Review details

• Files reviewed: 20/20 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

It includes a large, unrelated removal of the repo’s .github/ automation and templates that should be reverted or split into a separate PR before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 20/20 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes are targeted, consistently wired through factories, and are supported by new unit tests covering the built-in-resource discovery and schema-validation behavior.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[jcogilvie]

thanks for your contribution @aribeiro94

can you please put the PR checklist in your PR body and fill it out? you'll also need to sign the DCO (per said checklist).

[jcogilvie]

(posted by my 🤖 on my behalf)

Thanks for this — the underlying problem is real and worth fixing, and the discovery-based scope lookup is the right mechanism for it. I want to flag one change I think should come out, plus some convention alignment.

I've split this into what's blocking and what's polish. Two things first, so you don't chase them:

• The lint failure is ours, not yours. It's pre-existing breakage on main from the golangci-lint v2.13.2 exhaustruct → exhaustruct_v5 rename — the disable entry in .golangci.yml stopped matching the renamed linter, so it fired ~1475 findings repo-wide. Fix is up in #455; once that lands, rebase and lint should go green. Please ignore it until then.
• CI hadn't run on this PR until just now — that was my approval gate as maintainer, not anything you did. Your unit-tests run passes.

---

Summary

The PR contains three changes. Two are good; one I'd like removed.

Change	Verdict
Scope determination prefers discovery over GetCRD (getResourceScope, isNamespacedResource)	✅ Keep
Threading k8.ResourceClient into SchemaValidator + factory	✅ Keep
resourcesRequiringCRDSchema() filtering built-ins out of schema validation	❌ Please drop

The good part: GetCRD fails for Secret/ConfigMap/Namespace because built-ins have no CRD, so scope lookup hard-failed the diff. Discovery fixes that properly, and it's consistent with how requirements_provider.go (lines 212, 303) already resolves scope. Both of the tests covering this are genuine — I verified by stubbing out the discovery branch and watching them fail.

---

Blocking

1. resourcesRequiringCRDSchema() removes validation coverage, and isn't needed

Upstream crossplane/cli/v2 v2.5.0 already validates built-in Kubernetes types. pkg/validate.SchemaValidate has an internal kubernetesResourceValidator (validate.go:42, :177) that registers kubescheme + extv1 + apiregistrationv1 into a runtime.Scheme and strict-decodes recognized GVKs. In validateResource (~:125-133), when no CRD-derived validator matches a GVK it tries that Kubernetes validator first, and only falls through to ValidationStatusMissingSchema if the type isn't recognized.

Running the real module with an empty CRD list:

v1/ConfigMap cm   -> status=valid     errors=0
v1/ConfigMap cm2  -> status=invalid   errors=1   [schema] unknown field "dataz"
v1/Secret    s    -> status=valid      errors=0
summary: total=3 valid=2 invalid=1 missingSchemas=0

Built-ins already pass without a CRD, and field typos are still caught. A genuinely unknown custom resource still correctly reports missingSchema. So the filter addresses a problem that upstream already solved — and it costs real coverage. With a composed ConfigMap carrying a typo'd top-level field dataz:

• this branch: ValidateResources returns nil — the typo is silently accepted
• filter removed (only change): v1/ConfigMap default/cm: unknown field "dataz" [schema]

Because IsCRDRequired keys off API group (empty group, apps, batch, extensions, policy, autoscaling, *.k8s.io), this also silences validation for Deployment, Job, NetworkPolicy, and friends — not just Secret/ConfigMap. That runs against the "Accuracy Above All Else" principle in CLAUDE.md.

If you hit a concrete failure that motivated this filter, please share the error — I'd like to fix the actual cause rather than lose the validation.

2. TestDefaultSchemaValidator_ValidateResourcesSkipsBuiltInResourcesForSchemaValidation doesn't test its change

This test passes with the production change it names fully reverted. Swapping SchemaValidate(ctx, schemaResources, …) back to SchemaValidate(ctx, resources, …) leaves both the named test and the entire ./cmd/diff/diffprocessor suite green.

The reason: it only asserts err == nil plus "GetCRD wasn't called for the Secret", but EnsureComposedResourceCRDs already skips non-CRD-required GVKs on main — so nothing observable changes either way. If change #1 comes out this test goes with it; the note is here mainly because it's worth knowing the assertion was vacuous.

3. Please add a TestDiffIntegration case for a CRD-less resource

This is the most valuable thing missing. The bug is user-visible end-to-end behavior — "can crossplane-diff diff a composition that renders a plain ConfigMap" — and that's exactly what the integration tests pin. A case here would have caught both the original bug and the validation regression in #1, which unit tests with mocked clients structurally cannot.

Concretely, as a case in the TestDiffIntegration map (cmd/diff/diff_integration_test.go:408):

• a composition fixture whose pipeline emits a plain ConfigMap (plus function-auto-ready)
• an XR input fixture under testdata/diff/
• assert with structured JSON via tu.ExpectDiff() rather than an .ansi golden file — per the "Structured JSON Testing (Preferred for New Tests)" guidance in CLAUDE.md, since this is a semantic test, not a formatting one
• if it's cheap to include, a case where the ConfigMap has an invalid field would lock in that validation still fires on built-ins

4. DCO sign-off

The DCO check is failing — none of the commits carry a Signed-off-by trailer. It's a required check, so please sign off (git commit -s, or git rebase --signoff for the existing range).

5. Fill in the PR description

The body is currently empty, which is also why checklist-completed is failing — that check requires the task list from .github/PULL_REQUEST_TEMPLATE.md. Please fill in the template, including the "I have:" checklist.

On the description itself: I'd frame this as a fix rather than a feat. Nothing here adds a capability — no new flag, subcommand, or supported input. The tool was already meant to handle these resources and aborted with cannot determine scope for resource … CRD not found; this repairs a scope-determination path that wrongly assumed every resource has a CRD. Suggested title:

fix(diffprocessor): determine resource scope via discovery for CRD-less built-in types

A description sketch, covering only the changes I'd keep:

crossplane-diff failed when a composition rendered built-in Kubernetes resources
(ConfigMap, Secret, Namespace, …) rather than only Crossplane managed resources.
Scope determination went exclusively through SchemaClient.GetCRD(); built-in types have
no CRD, so the lookup failed and the diff aborted. Two call sites were affected:
DefaultSchemaValidator.getResourceScope() and
DefaultDiffProcessor.removeNamespacesFromClusterScopedResources(). Both now resolve
scope from the discovery API via ResourceClient.IsNamespacedResource(), matching how
RequirementsProvider already does it, with a CRD fallback when discovery is unavailable.

---

Non-blocking (conventions)

Fold the new tests into the existing tables. This repo prefers table-driven tests, and all three additions are standalone functions. The scope test in particular should be rows in the existing TestDefaultSchemaValidator_ValidateScopeConstraints table (schema_validator_test.go:867) — that table already has a setupClient func() *tu.MockSchemaClient field; adding a sibling setupResourceClient lets "BuiltInNamespacedResourceViaDiscovery" and "BuiltInClusterScopedResourceViaDiscovery" drop straight in as rows.

Use the mock builder helpers rather than hand-rolled closures. The tests write WithIsNamespacedResource(func(...) { switch gvk {...} }), but WithNamespacedResource(gvks...) and WithClusterScopedResource(gvks...) already exist (testutils/mock_builder.go:227, :243). Note a genuine limitation: neither can express "Secret namespaced and Namespace cluster-scoped" in one mock, because each overwrites the single IsNamespacedResourceFn. The right fix is to extend the builder so the two helpers accumulate into a shared scope map — that's the convention here (extend the builder with an expressive method) rather than bypassing it.

Assertion style. The surrounding tests use t.Errorf("\n%s\n…", tt.reason, …) to surface the case's reason; the new ones use bare t.Fatalf messages.

Return a typed scope. getResourceScope returns string and callers compare against "Namespaced"/"Cluster" literals, while the new discovery branch imports extv1.NamespaceScoped/extv1.ClusterScoped. Returning extv1.ResourceScope would make the whole path consistent.

Nil-guards driven by test construction. In DefaultDiffProcessor.isNamespacedResource, the p.resourceClient != nil / p.schemaClient == nil guards — and the "Fallback for tests and any stale discovery path" comment in getResourceScope — exist only because the new test builds &DefaultDiffProcessor{...} by hand instead of going through NewDiffProcessor. In production both are always set. Folding these cases into the existing tables should remove the need for the guards.

Consider whether the discovery fallback should be silent. A discovery failure is swallowed to a Debug log and retried via CRD, whereas requirements_provider.go:212,303 hard-errors on the same IsNamespacedResource failure. Worth being deliberate: swallowing can mask a real RBAC or connectivity problem, which cuts against the accuracy-first principle. I don't feel strongly about which way, but the two paths should agree.

Update the design doc. Per the trigger table in CLAUDE.md, a changed SchemaValidator signature needs §6 of design/design-doc-cli-diff.md updated (§6.5, ~line 637 — the constructor signature and the new ResourceClient dependency), plus the affected .mermaid diagrams regenerated to .svg.

Stray whitespace. cmd/diff/testdata/diff/resources/external-resource-configmap-other-namespace.yaml gained a blank first line, unrelated to the change — worth reverting to keep the diff clean.

---

Happy to pair on the integration test fixtures if that's the fiddly part — and if the validation filter was working around something specific, let's dig into that error together.