wslc: add --all-tags to push for docker parity - #41500
Draft
ggarzia-MSFT wants to merge 3 commits into
Draft
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
WSLCSession::PushImage currently allows AllTags=TRUE with a tagged/digest reference by silently pushing all tags, diverging from the CLI/docker-parity behavior that rejects this combination.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Docker-parity support for --all-tags/-a on wslc push / wslc image push, and changes default push semantics for name-only references to push :latest (instead of implicitly pushing all tags), with updated localization and test coverage.
Changes:
- Add
--all-tags/-aCLI flag wiring for push, plus user-facing help/error strings. - Update
IWSLCSession::PushImage/WSLCSession::PushImageto accept anAllTagsboolean and adjust tag defaulting behavior. - Add/extend unit and E2E tests covering parsing, help text, and invalid combinations.
File summaries
| File | Description |
|---|---|
| test/windows/WSLCTests.cpp | Updates PushImage test calls for new AllTags parameter. |
| test/windows/wslc/WSLCCLICommandUnitTests.cpp | Adds unit coverage verifying --all-tags is registered with -a alias. |
| test/windows/wslc/e2e/WSLCE2EImagePushTests.cpp | New E2E tests for help text and invalid --all-tags combinations. |
| test/windows/wslc/CommandLineTestCases.h | Adds command-line parser test cases for push + --all-tags/-a. |
| test/windows/PluginTests.cpp | Updates PushImage call signature for plugin tests. |
| src/windows/wslcsession/WSLCSession.h | Extends PushImage COM method signature with AllTags. |
| src/windows/wslcsession/WSLCSession.cpp | Implements AllTags behavior and default tag handling in PushImage. |
| src/windows/wslc/tasks/ImageTasks.cpp | Adds CLI-side --all-tags parsing, validation, and default-tag messaging. |
| src/windows/wslc/services/ImageService.h | Extends ImageService::Push signature to accept allTags. |
| src/windows/wslc/services/ImageService.cpp | Forwards allTags into session PushImage call. |
| src/windows/wslc/commands/ImagePushCommand.cpp | Registers --all-tags argument + push-specific description. |
| src/windows/wslc/arguments/ArgumentDefinitions.h | Adds ArgType::AllTags to argument definitions. |
| src/windows/service/inc/wslc.idl | Updates IWSLCSession::PushImage signature in IDL. |
| localization/strings/en-US/Resources.resw | Adds new localized strings for --all-tags and the tag+all-tags error. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2122
to
+2130
| // Omitting the tag makes the daemon push every tag in the repository. | ||
| if (AllTags) | ||
| { | ||
| tagOrDigest.reset(); | ||
| } | ||
| else if (!tagOrDigest.has_value()) | ||
| { | ||
| tagOrDigest = "latest"; | ||
| } |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The session-layer AllTags implementation can silently override an explicitly provided tag/digest (instead of rejecting it), and the new default-tag push behavior lacks dedicated e2e coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/windows/wslcsession/WSLCSession.cpp:2126
- When AllTags is TRUE, this currently clears any explicitly provided tag/digest and pushes every tag in the repository. That makes a tagged reference like "repo:tag" behave very differently (and potentially unexpectedly) instead of being rejected like the CLI does.
// Omitting the tag makes the daemon push every tag in the repository.
if (AllTags)
{
tagOrDigest.reset();
}
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+26
to
+34
| WSLC_TEST_METHOD(WSLCE2E_Image_Push_AllTagsListedInHelp) | ||
| { | ||
| const auto result = RunWslc(L"image push --help"); | ||
| result.Verify({.Stderr = L"", .ExitCode = 0}); | ||
|
|
||
| VERIFY_IS_TRUE(result.StdoutContainsSubstring(L"--all-tags")); | ||
| VERIFY_IS_TRUE(result.StdoutContainsSubstring(Localization::WSLCCLI_PushAllTagsArgDescription())); | ||
| } | ||
|
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It includes a repository-inappropriate local script (merge-probe.ps1) and the PushImage signature change is not reflected in the SDK wrapper call site (e.g., src/windows/WslcSDK/wslcsdk.cpp), which would break compilation once headers regenerate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
771
to
773
| HRESULT Authenticate([in] LPCSTR ServerAddress, [in] LPCSTR Username, [in] LPCSTR Password, [out] LPSTR* IdentityToken); | ||
| HRESULT PushImage([in] LPCSTR Image, [in] LPCSTR RegistryAuthenticationInformation, [in, unique] IProgressCallback* ProgressCallback, [in, unique] IWarningCallback* WarningCallback); | ||
| HRESULT PushImage([in] LPCSTR Image, [in] LPCSTR RegistryAuthenticationInformation, [in] BOOL AllTags, [in, unique] IProgressCallback* ProgressCallback, [in, unique] IWarningCallback* WarningCallback); | ||
| HRESULT PruneVolumes([in, unique, size_is(FiltersCount)] const WSLCFilter* Filters, [in] ULONG FiltersCount, [in, unique] IWarningCallback* WarningCallback, [out, size_is(, *VolumesCount)] WSLCVolumeName** Volumes, [out] ULONG* VolumesCount, [out] ULONGLONG* SpaceReclaimed); |
Comment on lines
+1
to
+5
| $ErrorActionPreference = 'Continue' | ||
| Set-Location 'C:\Users\gavingarzia\repos\WSL' | ||
|
|
||
| $branches = @( | ||
| 'user/ggarzia/wslc-prune-docker-parity', |
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.
Summary of the Pull Request
Adds the
--all-tags/-aflag towslc pushandwslc image pushfor Docker CLI parity, and aligns the default (no-flag) push behavior withdocker push.PR Checklist
Detailed Description of the Pull Request / Additional comments
Behavior change worth calling out
At the Engine API level, "push all tags" is expressed by omitting the
tagquery parameter onPOST /images/{name}/push— the same thing the moby client does (if !options.All { query.Set("tag", tag) }).WSLCSession::PushImagepreviously forwarded the parsed tag verbatim, so an untagged reference such aswslc push myrepo/imgalready omittedtagand pushed every tag. That means simply adding--all-tagswould have made the flag a no-op relative to the default.This PR therefore also makes the untagged case default to
latest, matchingdocker pushand matching whatwslc pullalready does. After this change:wslc push myrepo/imgmyrepo/img:latest(and printsUsing default tag: latest)wslc push myrepo/img:v1myrepo/img:v1wslc push myrepo/img --all-tagsmyrepo/imgwslc push myrepo/img:v1 --all-tagstag can't be used with --all-tags/-aTests
--all-tags,-a, bothpushandimage pushspellings,--all-tags=true, flag-before-positional ordering, and the negative cases--alltagsand the case-sensitive-A.ImagePushCommand_HasAllTagsArgumentWithDockerAliasunit test.WSLCE2EImagePushTests.cppwithWSLCE2E_Image_Push_AllTagsListedInHelp,WSLCE2E_Image_Push_AllTagsRejectsTaggedReference, andWSLCE2E_Image_Push_AllTagsRejectsDigestReference.Note for reviewers
The companion
pull --all-tagsPR introduces the sameArgType::AllTagsentry and an equivalenttag can't be used with --all-tags/-astring under aWSLCCLI_PullAllTagsWithTagErrorkey. Whichever lands second should reuse the neutralWSLCCLI_AllTagsWithTagErrorkey added here rather than keeping both.Validation Steps Performed
Full
cmake --build .succeeds.wslc image push --helpwas run against the local build and lists-a --all-tags Push all tags of an image to the repository. Automated coverage above; CI runs the test suite.