feat: optionally hide composition changes in comp subcommand by fernandezcuesta · Pull Request #375 · crossplane-contrib/crossplane-diff

fernandezcuesta · 2026-07-10T07:23:41Z

Description of your changes

Fixes #371

I have:

Read and followed Crossplane's contribution process.
Run earthly -P +reviewable to ensure this PR is ready for review.
Added or updated unit tests.
~~- [ ] 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.

Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>

Copilot

Pull request overview

Adds an opt-in --hide-composition flag to the crossplane-diff comp subcommand so users can focus on affected XRs and their downstream diffs without always printing the composition diff section (addressing #371).

Changes:

Introduces HideComposition plumbing from CLI (comp subcommand) → processor config → renderer options.
Updates human and structured (JSON/YAML) composition diff renderers to omit the composition diff when requested.
Adds unit test coverage and updates README usage/docs.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
README.md	Documents the new `--hide-composition` flag and example usage.
cmd/diff/renderer/diff_formatter.go	Adds `HideComposition` to shared `DiffOptions`.
cmd/diff/renderer/comp_diff_renderer.go	Implements hiding composition diffs in human + structured renderers.
cmd/diff/renderer/comp_diff_renderer_test.go	Adds tests validating hidden composition output behavior.
cmd/diff/diffprocessor/processor_config.go	Plumbs `HideComposition` through processor config/options into renderer options.
cmd/diff/comp.go	Adds the `--hide-composition` CLI flag and wires it into processor options.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+		// Render composition changes section. When --hide-composition is set we still
+		// surface processing errors (so failures aren't silently swallowed) but omit the
+		// composition diff itself.
+		if !r.opts.HideComposition || comp.Error != nil {
+			if err := r.renderCompositionChanges(&comp); err != nil {
+				return err
+			}
 		}


Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

+		// Render composition changes section. When --hide-composition is set we still
+		// surface processing errors (so failures aren't silently swallowed) but omit the
+		// composition diff itself, including the section header.
+		if r.opts.HideComposition && comp.Error != nil {
+			if _, err := fmt.Fprintf(stdout, "Error processing composition %s: %s\n\n", comp.Name, comp.Error.Error()); err != nil {
+				return errors.Wrap(err, "cannot write composition error")
+			}
+		} else if !r.opts.HideComposition {
+			if err := r.renderCompositionChanges(&comp); err != nil {
+				return err
+			}
 		}


+		// Render composition changes section. When --hide-composition is set we still
+		// surface processing errors (so failures aren't silently swallowed) but omit the
+		// composition diff itself, including the section header.
+		if r.opts.HideComposition && comp.Error != nil {
+			if _, err := fmt.Fprintf(stdout, "Error processing composition %s: %s\n\n", comp.Name, comp.Error.Error()); err != nil {
+				return errors.Wrap(err, "cannot write composition error")
+			}


jcogilvie · 2026-07-10T20:04:39Z

(output from claude based on back and forth with me -- ideas are mine, i just had it do a summary writeup)

PR #375 review — reframe `--hide-composition` → `--minimize-composition`

Thanks for this! The flag solves a real need. I'd like to
propose a small reframe that (a) makes the output less lossy, (b) resolves the Copilot
feedback, and (c) lines the flag up with a convention we already use elsewhere.

The reframe

Instead of hiding the composition-changes section entirely, minimize it: collapse a
changed composition down to a single labeled line that keeps the marker and the name, and
drop only the YAML body.

Consider (this is sort of ancillary to the task, but i think it matches more closely after changes I'm proposing):

rename the flag --hide-composition → --minimize-composition.

Why minimize beats hide

It's not lossy about identity. Full-hide prints nothing for the composition, so with
multiple composition files you can't tell which block belongs to which composition
(this is exactly Copilot's second comment). A minimized line names the composition, so
that ambiguity disappears — no separate identifier line needed.
It matches our house style. We never silently omit; we leave a labeled breadcrumb and
a marker. See the pinned-XR (Manual update policy) treatment below.
It keeps the change type visible. The existing ~~~ / +++ / === marker already
tells you modified / added / no-change. That's the signal that matters; we don't need a
line count (we don't surface line counts anywhere else in the tool, so let's not start).

Projected output

Without `--minimize-composition` (unchanged default)

=== Composition Changes ===

~~~ Composition/xcompdiffresources.compdiff.example.org
  apiVersion: apiextensions.crossplane.io/v1
  kind: Composition
  ...            (~45 lines of composition YAML diff)
      step: detect-ready-resources

---

Summary: 1 modified

=== Affected Composite Resources ===

  ⚠ XCompDiffResource/test-comp-resource (cluster-scoped)

Summary: 1 resource with changes

=== Impact Analysis ===

~~~ ClusterNopResource/test-comp-resource-XXXXX
  ...

With `--minimize-composition`

=== Composition Changes ===

~~~ Composition/xcompdiffresources.compdiff.example.org (minimized)

=== Affected Composite Resources ===

  ⚠ XCompDiffResource/test-comp-resource (cluster-scoped)

Summary: 1 resource with changes

=== Impact Analysis ===

~~~ ClusterNopResource/test-comp-resource-XXXXX
  ...

The ~~~ Composition/<name> (minimized) line reuses the existing modified-marker verbatim
(same glyph, same color) and just omits the body + its Summary: 1 modified footer.

Multiple compositions (the case that motivated the reframe)

Minimized, this collapses to a clean, marker-typed legend at the top — you see what changed
and which composition at a glance, then read the downstream impact below:

=== Composition Changes ===

~~~ Composition/xbuckets.example.org (minimized)
~~~ Composition/xdatabases.example.org (minimized)
=== Composition/xnetworks.example.org (minimized)

=== Affected Composite Resources ===
  ...

Label: `(minimized)`

Just (minimized).

Rationale, and it's a deliberate contrast with the pinned-XR breadcrumb below: the
--include-manual breadcrumb spells out the flag because that's an opt-in to reveal — the
user hasn't asked to see pinned XRs, so we tell them how. --minimize-composition is the
inverse: the user opted out by typing the flag. They know what it means; echoing it back
would be noise.

Comparison: this mirrors the pinned-XR (Manual policy) treatment

We already have precedent for "withhold detail, keep a labeled trace." When an XR uses the
Manual update policy:

# A pinned XR is surfaced but its diff withheld — user is told how to reveal it,
# because they did NOT opt in:
  ⊘ XR/manual-xr (namespace: ns) — filtered: Manual update policy (use --include-manual to evaluate)

# --minimize-composition withholds the composition body the same way,
# but stays terse because the user explicitly opted in:
~~~ Composition/xbuckets.example.org (minimized)

Same design language (typed marker + identity + parenthetical state); the only difference is
whether we spell out the reveal-flag, which follows from opt-in vs opt-out.

Edge cases (please pin these down)

No composition change (CompositionDiff == nil or DiffTypeEqual): keep the existing
No changes detected in composition <name> line as-is. It's already one line and already
names the composition — only changed compositions get the collapsed-body treatment.
Processing error (comp.Error != nil): never collapse an error. Still print
Error processing composition <name>: <err> in full, exactly as the current PR's error
carve-out does. The rule is "collapse the diff body, never the error." This keeps us
consistent with the project's "errors are never silently swallowed" principle.
Structured output (--output json|yaml): human-only flag; do NOT strip
compositionChanges. Per our "machine output = full fidelity" rule in CLAUDE.md
(Machine-Readable Error Handling), a display-only affordance shouldn't remove data a CI
consumer might parse. Machines can filter fields themselves. So --minimize-composition
affects the human renderer only; the JSON/YAML payload keeps full compositionChanges.
(Note this diverges from the current PR, which omits compositionChanges in structured
mode when hidden.)

On the existing Copilot comments

1 (header leak on error path): already fixed by commit 08610f5; the thread just
needs resolving. Under the minimize reframe this is moot anyway (errors print in full, no
header).
2 (no composition identifier when hidden): subsumed — the ~~~ Composition/<name> (minimized) line is the identifier.
3 (doc comment overstates "errors → Stderr"): still valid and worth fixing regardless
of the reframe. The real contract is two-tier: top-level/tool errors → stderr (+ structured
payload); per-composition messages → stdout as part of the diff narrative. Please update the
RenderCompDiff doc comments (interface + both impls) to say that.

Exit-code note (not a blocker)

--minimize-composition is cosmetic and does not affect hasDiffs/exit code. If the
composition changed but all XRs render identically downstream, you'll still get exit code 2
with only the minimized line explaining it — which is arguably clearer than full-hide (where
exit 2 came from an invisible change). Worth one sentence in the flag help or README.

Summary of the ask

Rename --hide-composition → --minimize-composition.
Changed composition → single ~~~ Composition/<name> (minimized) line ((minimized) only).
"No changes" and error lines: unchanged (never collapse an error).
Structured output: human-only flag; keep full compositionChanges fidelity.
Fix the RenderCompDiff doc comments (Copilot 3), resolve Copilot 1 thread.

jcogilvie

see review feedback comment on the PR at large

feat: optioanlly hide composition changes in comp subcommand

3ee6390

Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>

fernandezcuesta changed the title ~~feat: optioanlly hide composition changes in comp subcommand~~ feat: optionally hide composition changes in comp subcommand Jul 10, 2026

fernandezcuesta marked this pull request as ready for review July 10, 2026 07:26

fernandezcuesta requested review from jcogilvie and tampakrap as code owners July 10, 2026 07:26

Copilot AI review requested due to automatic review settings July 10, 2026 07:26

Copilot started reviewing on behalf of fernandezcuesta July 10, 2026 07:27 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

fix: copilot review

08610f5

Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 10, 2026 08:03

Copilot started reviewing on behalf of fernandezcuesta July 10, 2026 08:04 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

jcogilvie requested changes Jul 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: optionally hide composition changes in comp subcommand#375

feat: optionally hide composition changes in comp subcommand#375
fernandezcuesta wants to merge 2 commits into
crossplane-contrib:mainfrom
fernandezcuesta:371--hide-composition-diff

fernandezcuesta commented Jul 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

jcogilvie commented Jul 10, 2026 •

edited

Loading

Uh oh!

jcogilvie left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

fernandezcuesta commented Jul 10, 2026

Description of your changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

jcogilvie commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR #375 review — reframe --hide-composition → --minimize-composition

The reframe

Why minimize beats hide

Projected output

Without --minimize-composition (unchanged default)

With --minimize-composition

Multiple compositions (the case that motivated the reframe)

Label: (minimized)

Comparison: this mirrors the pinned-XR (Manual policy) treatment

Edge cases (please pin these down)

On the existing Copilot comments

Exit-code note (not a blocker)

Summary of the ask

Uh oh!

jcogilvie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jcogilvie commented Jul 10, 2026 •

edited

Loading

PR #375 review — reframe `--hide-composition` → `--minimize-composition`

Without `--minimize-composition` (unchanged default)

With `--minimize-composition`

Label: `(minimized)`