Skip to content

fix(display): wire --dry-run flag into progress writers - #14053

Open
htoyoda18 wants to merge 1 commit into
docker:mainfrom
htoyoda18:fix/dry-run-progress-display
Open

htoyoda18 wants to merge 1 commit into
docker:mainfrom
htoyoda18:fix/dry-run-progress-display

Conversation

@htoyoda18

@htoyoda18 htoyoda18 commented Aug 16, 2026 •

Copy link
Copy Markdown
Contributor

What I did

docker compose --dry-run ... never rendered the DRY-RUN MODE prefix in progress output, on any of the three renderers.

The pieces are there but nothing connects them:

TTY: termWriter has a dryRun field and tty_layout.go prefixes every row with it, and WithDryRun() exists as a TermOption — but nothing ever passes that option, so it is always false.
plain / json: plainWriter/jsonWriter both have a dryRun field used at render time, but neither Plain() nor JSON() accepts a value for it, so it is always false too.
This wires the already-parsed --dry-run flag through selectEventProcessor to all three:

TTY: passes the existing display.WithDryRun() option — cmd/display/tty.go itself is unchanged.
plain / json: adds a dryRun bool parameter to the two constructors.
Tests: the TTY case asserts on the rendered frame (the marker actually reaches the output) rather than on the field; plain_test.go / json_test.go cover the other two renderers.

Note: this PR was originally written before the tty.go model/layout/screen refactor and has been rebased and reworked on top of it. The earlier version added a dryRun parameter to display.Full(); it now uses the WithDryRun() option that the refactor introduced, which keeps tty.go untouched.

Related issue
N/A

(not mandatory) A picture of a cute animal, if possible in relation to what you did
🐈

@htoyoda18
htoyoda18 requested review from a team as code owners August 16, 2026 12:53
@htoyoda18
htoyoda18 requested review from glours and ndeloof August 16, 2026 12:53
@htoyoda18
htoyoda18 force-pushed the fix/dry-run-progress-display branch from 1454b74 to 03831bc Compare August 23, 2026 13:10
@htoyoda18

Copy link
Copy Markdown
Contributor Author

Hi @ndeloof — this fixes the FIXME you left in cmd/display/tty.go (dryRun field was never wired up across the tty/plain/json progress writers, so --dry-run never showed the DRY-RUN prefix in progress output). I've since rebased on top of your PersistentPreRunE refactor and the display.Mode resolution changes, so it should apply cleanly. Would appreciate a review when you have a chance — thanks!

@htoyoda18

Copy link
Copy Markdown
Contributor Author

Hi @ndeloof @glours 👋 Just a friendly nudge on this one too — it's been a couple of weeks with no review yet. Happy to address anything if you get a chance to take a look. Thanks!

The tty, plain, and json EventProcessor constructors accepted no
dry-run flag, so the DRY-RUN prefix was never rendered even when
--dry-run was passed.

Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>
@htoyoda18
htoyoda18 force-pushed the fix/dry-run-progress-display branch from 04e5074 to b8d86e5 Compare September 26, 2026 03:43
@htoyoda18

Copy link
Copy Markdown
Contributor Author

Hi @ndeloof @glours 👋 Gentle nudge on this one when you have a moment.

Rebased on the new tty model/layout/screen split, and reworked along the way:
--dry-run never reached the progress writers, so the DRY-RUN marker was never
rendered. TTY now just gets the existing display.WithDryRun() option (so
tty.go itself is untouched), and Plain/JSON take a dryRun param.

Verified against a real daemon with a 2-service project:

  • plain: Image alpine:3.20 Pulling → DRY-RUN MODE - Image alpine:3.20 Pulling
  • json: "dry-run":true now present
  • tty: ✔ DRY-RUN MODE - Network xxx_default Created
  • without --dry-run: no prefix and no dry-run field, so no regression

If you don't think this is worth merging, just let me know and I'll close it.
Thanks!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant