wslc: add docker --quiet to image load, image push and container cp - #41466
wslc: add docker --quiet to image load, image push and container cp#41466ggarzia-MSFT wants to merge 8 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends wslc’s Docker-parity support for -q/--quiet by registering the flag on image load, image push, and container cp, and by honoring it for image push output suppression.
Changes:
- Register
-q/--quietonImageLoadCommand,ImagePushCommand, andContainerCpCommandwith per-command localized help text. - Implement quiet semantics for
image pushby suppressing progress output and emitting the canonical reference on success. - Add parser/unit/E2E coverage for the new flag across the three commands.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/windows/wslc/tasks/ImageTasks.cpp |
Implements quiet-mode behavior for image push by disabling progress callback and printing canonical reference on success. |
src/windows/wslc/commands/ImagePushCommand.cpp |
Registers --quiet/-q for image push with localized description. |
src/windows/wslc/commands/ImageLoadCommand.cpp |
Registers --quiet/-q for image load with localized description. |
src/windows/wslc/commands/ContainerCpCommand.cpp |
Registers --quiet/-q for container cp with localized description. |
localization/strings/en-US/Resources.resw |
Adds new localized strings for the three quiet-flag descriptions. |
test/windows/wslc/CommandLineTestCases.h |
Adds table-driven parsing cases for --quiet/-q (including --quiet=false and invalid values). |
test/windows/wslc/WSLCCLICommandUnitTests.cpp |
Adds a unit test asserting the three commands register ArgType::Quiet consistently. |
test/windows/wslc/e2e/WSLCE2EPushPullTests.cpp |
Adds E2E tests validating push -q/--quiet output suppression and failure behavior. |
test/windows/wslc/e2e/WSLCE2EImageSaveTests.cpp |
Adds E2E coverage for image load -q/--quiet Docker-parity behavior. |
test/windows/wslc/e2e/WSLCE2EContainerCpTests.cpp |
Adds E2E coverage for container cp -q/--quiet acceptance and behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
localization/strings/en-US/Resources.resw:2781
- The
image loadquiet flag description says it "Suppress the load output", butimage load --quietstill prints theLoaded image:line (and there is no other progress output to suppress). This makes--helpmisleading; consider describing it as suppressing progress output instead.
<data name="WSLCCLI_ImageLoadQuietArgDescription" xml:space="preserve">
<value>Suppress the load output</value>
</data>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
test/windows/wslc/e2e/WSLCE2EPushPullTests.cpp:134
- The local registry port here is set to 15005, which is already used by WSLCE2EImageBuildTests (c_registryPort = 15005). If E2E tests run in parallel, this can cause flaky failures due to port collisions. Use an unused port value for this test (or centralize/allocate ports) so each class has a distinct registry port.
auto [registryContainer, registryAddress] = StartLocalRegistry(*session, "", "", 15005);
localization/strings/en-US/Resources.resw:2633
- This help text says --quiet suppresses copy progress, but ContainerCp currently doesn't use ArgType::Quiet at all (the flag is accepted but has no behavioral effect). The description should match actual behavior to avoid misleading users.
<data name="WSLCCLI_CpQuietArgDescription" xml:space="preserve">
<value>Suppress progress output during copy. Progress output is automatically suppressed if no terminal is attached</value>
</data>
localization/strings/en-US/Resources.resw:2781
- This description implies that --quiet suppresses output for
image load, but LoadImage ignores ArgType::Quiet and still prints the "Loaded image:" line (quiet is currently accepted for compatibility only). Update the help text so it doesn't promise output suppression that doesn't occur.
<data name="WSLCCLI_ImageLoadQuietArgDescription" xml:space="preserve">
<value>Suppress the load output</value>
</data>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A few user-facing/help/test reliability details are inconsistent (quiet descriptions vs actual behavior, and a fixed-port reuse that can increase E2E flakiness).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
localization/strings/en-US/Resources.resw:2781
- The
image load --quiethelp text says it suppresses load output, butLoadImage()always usesWSLCImageLoadCallback, which printsLoaded image:messages even in quiet mode (and the new E2E test asserts that line still appears). The description should be updated to match the actual behavior (quiet currently doesn’t suppress the final "Loaded image" line).
<data name="WSLCCLI_ImageLoadQuietArgDescription" xml:space="preserve">
<value>Suppress the load output</value>
</data>
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
David Bennett (dkbennett)
left a comment
There was a problem hiding this comment.
Nice use of single argument type across multiple commands to have a clean implementation in one place in the image tasks.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
There are user-facing/help-text and localization maintainability issues (misleading image load --quiet description and duplicated quiet description strings) that should be addressed before approval.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/windows/wslc/commands/ImagePushCommand.cpp:32
- WSLCCLI_ImagePushQuietArgDescription duplicates the existing WSLCCLI_PullQuietArgDescription value (both are "Suppress verbose output"), which increases localization surface area without changing behavior. Consider reusing the existing key for push and removing the new string entry to avoid redundant translations.
localization/strings/en-US/Resources.resw:2781
- The --quiet description for image load says it "Suppress[es] the load output", but the CLI still prints a "Loaded image:" line (and the new E2E test relies on it). Consider narrowing the help text to clarify that only progress/verbose output is suppressed, not the final loaded-image line.
<data name="WSLCCLI_ImageLoadQuietArgDescription" xml:space="preserve">
<value>Suppress the load output</value>
</data>
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Untagged quiet pushes can push every local tag while reporting only latest.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The load option’s help text contradicts its tested behavior, and one new local name violates repository conventions.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
test/windows/wslc/WSLCCLICommandUnitTests.cpp:267
VerifyQuietArgumentis a local lambda variable, so its PascalCase name violates the repository's camelCase convention for locals. Rename the lambda and its call sites toverifyQuietArgument.
localization/strings/en-US/Resources.resw:2780
- This help text promises that
--quietsuppresses load output, butLoadImagestill installsWSLCImageLoadCallback, and the added E2E test explicitly requires theLoaded image:output to remain. Describe the option as suppressing progress output so the CLI help matches its observable behavior.
<value>Suppress the load output</value>
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Non-quiet name-only pushes no longer report that the default latest tag was selected.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/windows/wslc/tasks/ImageTasks.cpp:305
- For a name-only push, the non-quiet path now correctly pins
:latest, but it omits the accompanyingUsing default tag: latestline. Docker's push implementation emits that line whenever a name-only reference is resolved and quiet mode is off, andPullImagealready follows the same behavior atImageTasks.cpp:267-272. Emit the localized default-tag message here before starting the push so normalpush NAMEoutput retains parity; quiet mode should continue suppressing it.
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Summary of the Pull Request
Adds docker's
-q/--quietflag to three commands that were missing it:image load,image pushand
container cp.image pull,image list,container list,network listandvolume listalready had it.
The flag is honored where wslc actually has output to suppress (
push) and accepted withdocker-matching semantics where it does not (
load,cp)PR Checklist
Validation Steps Performed
Added tests only; no existing tests needed changing.
Parser cases (
CommandLineTestCases.h) — 15 new table-driven cases covering both spellings onall three commands, the alias chain
cp -aq,--quiet=false, and rejection of--quiet=invalid.Unit test —
QuietParityCommands_RegisterQuietArgumentassertsImageLoadCommand,ImagePushCommandandContainerCpCommandeach register--quietwith aliasqas aKind::Flag. The existingAllCommands_NoAmbiguousArgumentNamesOrAliasestest confirms-qdoesnot collide on any of them.
E2E tests
WSLCE2E_Image_Push_QuietOption— pushes to a local registry and asserts stdout is exactly oneline equal to the canonical reference, proving progress was suppressed. Checks both spellings.
WSLCE2E_Image_Push_QuietOption_FailurePrintsNoReference— a failed quiet push leaves stdoutempty, proving the reference is only printed after success.
WSLCE2E_Image_Load_QuietFlag— assertsload --quietstill printsLoaded image:(dockerparity), that the loaded image runs, and that
-qand--quietproduce identical output.WSLCE2E_Container_Cp_QuietFlag— quiet copy in both directions succeeds with empty stdout andstderr and the file lands correctly.
Build is clean.