Repository navigation
wslc: add --digests to image list for docker parity - #41457
Conversation
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request adds --digests support to wslc image list to match Docker parity, wiring the flag through CLI argument parsing into the image listing service and updating both table and JSON render paths to only surface digests when explicitly requested.
Changes:
- Added a new
--digestsflag (no short alias) forimage list/image ls/images, and threaded it throughImageService::List. - Implemented digest formatting by reducing service “repo@sha256:…” values to Docker’s “sha256:…” form via
DigestFromRepoDigest, and conditionally added a DIGEST column in table output. - Added unit + E2E tests to lock the gating behavior (digests only appear when
--digestsis passed) and validate output parity between JSON and table formats.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/windows/wslc/WSLCCLIImageDigestUnitTests.cpp | New unit tests for DigestFromRepoDigest behavior. |
| test/windows/wslc/WSLCCLICommandUnitTests.cpp | Verifies image list/images register --digests with no short alias. |
| test/windows/wslc/e2e/WSLCE2EImageListTests.cpp | Adds E2E coverage for DIGEST column placement, gating, help output, and quiet behavior. |
| src/windows/wslc/tasks/ImageTasks.cpp | Threads --digests into listing and conditionally adds DIGEST column + JSON field population. |
| src/windows/wslc/services/ImageService.h | Extends ImageService::List signature with a digests parameter (defaulted). |
| src/windows/wslc/services/ImageService.cpp | Sets the WSLC digest flag and populates ImageInformation::Digest only when requested. |
| src/windows/wslc/services/ImageModel.h | Adds Digest field to the image model and introduces DigestFromRepoDigest. |
| src/windows/wslc/commands/ImageListCommand.cpp | Registers the new ArgType::Digests argument for the list command. |
| src/windows/wslc/arguments/ArgumentDefinitions.h | Defines the Digests argument metadata and localization key mapping. |
| localization/strings/en-US/Resources.resw | Adds localized description string for --digests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
localization/strings/en-US/Resources.resw:3023
- Most CLI argument descriptions in Resources.resw do not end with a period; this new string does, which makes help output inconsistent.
<value>Show image digests.</value>
David Bennett (dkbennett)
left a comment
There was a problem hiding this comment.
--quiet and --digests are valid options together and Docker CLI allows both. --quiet means only image ids are output, but --digests means that an image with multiple digests can appear multiple times in the output (same is true for tags)
Should add tests here which verify the --quiet and --digests combination (and also images with multiple tags should appear multiple times with --quiet).
This may be a pre-existing issue with quiet that could be addressed here.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…olumn Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
localization/strings/en-US/Resources.resw:3024
- Most CLI argument descriptions in Resources.resw omit trailing punctuation (e.g., "Run container in detached mode"). For consistency, drop the trailing period from this new description.
<data name="WSLCCLI_DigestsArgDescription" xml:space="preserve">
<value>Show image digests.</value>
</data>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
test/windows/wslc/e2e/WSLCE2EImageListTests.cpp:495
- This assertion is logically correct, but it reads as a double-negative ("IS_FALSE(find != npos)"). Using an explicit npos comparison makes failures easier to interpret and avoids precedence/clarity pitfalls in future edits.
VERIFY_IS_FALSE(defaultResult.GetStdoutLines()[0].find(L"DIGEST") != std::wstring::npos);
src/windows/wslcsession/WSLCSession.cpp:1958
- When WSLCListImagesFlagsDigests is not set, this branch still emits a digest value (it->second.front()) if RepoDigests are present. Since the flag is meant to gate whether digest info is included at all, it would be safer to always emit an empty digest when digests==false (even if the backend unexpectedly provides RepoDigests).
else if (!digests)
{
rows.push_back({&e, tag, it->second.front()});
}
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
test/windows/wslc/WSLCCLITableOutputUnitTests.cpp:274
- In this test, namePos/statusPos are used for arithmetic without first verifying the substrings were found. If either find() returns npos, the expected-position math can overflow and the assertion may become misleading.
const auto namePos = dataLine.find(L"container-a");
const auto statusPos = dataLine.find(L"running");
const auto expected = namePos + wcslen(L"container-a") + TableOutput<3>::DefaultColumnPadding + wcslen(L"DIGEST") +
TableOutput<3>::DefaultColumnPadding;
VERIFY_ARE_EQUAL(expected, statusPos);
test/windows/wslc/WSLCCLITableOutputUnitTests.cpp:306
- This assertion uses namePos in arithmetic without checking that "container-a" (and "running") were actually found. If either find() returns npos, the computed offset can overflow and hide formatting regressions.
const auto& dataLine = cap.lines()[1];
const auto namePos = dataLine.find(L"container-a");
VERIFY_ARE_EQUAL(namePos + wcslen(L"container-a") + TableOutput<3>::DefaultColumnPadding, dataLine.find(L"running"));
localization/strings/en-US/Resources.resw:3024
- This new argument description includes a trailing period, which is inconsistent with nearby argument descriptions (e.g., "Run container in detached mode") and will show up in --help output.
<data name="WSLCCLI_DigestsArgDescription" xml:space="preserve">
<value>Show image digests.</value>
</data>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
test/windows/wslc/e2e/WSLCE2EImageListTests.cpp:549
- This assertion only checks whether the digest text appears anywhere in the table output. That can pass even if the value appears in a different column (or in multiple places), so it doesn't actually verify the DIGEST column as the comment claims. Consider extracting the DIGEST column slice using the header offsets and comparing against that field.
// Every digest reported by json output must appear in the table's DIGEST column, so the two
// renderings cannot drift.
for (const auto& image : ParseNdjsonOutputAs<ImageOutputInformation>(jsonResult))
{
const auto digest = wsl::shared::string::MultiByteToWide(image.Digest);
VERIFY_IS_TRUE(
tableResult.StdoutContainsSubstring(digest),
WEX::Common::String().Format(L"'%ls' was missing from the table DIGEST column", digest.c_str()));
}
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain around digest-row coverage and matching, default listing semantics, and dangling-image test-key uniqueness.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/windows/wslcsession/WSLCSession.cpp:1981
Repository.Nameintentionally preserves the original spelling rather than the normalized identity. Keying this map byNamecan fail to associate valid equivalent references such asubuntu:latestanddocker.io/library/ubuntu@sha256:..., producing an undigested tag row plus an extra digest-only row. Use a canonical key for matching while retaining the original repository name for display.
digestsByRepo[reference->Repository.Name].push_back(repoDigest);
src/windows/wslcsession/WSLCSession.cpp:2025
- When
RepoTagsis empty but the daemon still returnsRepoDigests(for example, a pulled image after its last tag was removed), the normal listing now emitsrepoNamewith a<none>tag instead of the existing<none>:<none>dangling-image row. This changes default image-list semantics even though--digestswas not requested; skip these digest-only rows whendigestsis false and let the fallback below create the unnamed row.
if (!digests)
{
rows.push_back({&e, repoName, std::string{}});
test/windows/wslc/e2e/WSLCE2EImageListTests.cpp:570
<none>is not a unique repository/tag key: the service emits<none>:<none>for every dangling image (WSLCTests.cpp:1167-1171). If the test environment contains two dangling images, the firstemplaceID is reused and this test fails even though the digest rows are valid; skip placeholder rows or include the image ID in the uniqueness key.
const auto it = idByRepoTag.emplace(std::make_pair(image.Repository, image.Tag), image.ID).first;
VERIFY_ARE_EQUAL(it->second, image.ID, L"Rows sharing a repository and tag must report the same image ID");
VERIFY_IS_TRUE(
seen.emplace(image.Repository, image.Tag, image.Digest).second,
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
…rtions Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate issues remain regarding repository grouping, default digest-only rows, and non-empty digest test coverage.
Review details
Suppressed comments (3)
src/windows/wslcsession/WSLCSession.cpp:1981
- This groups by
Repository.Name, which deliberately preserves the input spelling rather than the normalized repository identity. If Docker returns a short RepoTag such asubuntu:latestand a fully qualified RepoDigest such asdocker.io/library/ubuntu@sha256:..., the lookup at line 1997 misses it: the tagged row gets<none>and a separate digest-only row is emitted. Key the tag/digest groups byRepository.GetCanonical()while retaining an original name separately for display.
digestsByRepo[reference->Repository.Name].push_back(repoDigest);
src/windows/wslcsession/WSLCSession.cpp:2025
- When
digestsis false, this branch still emits a row for every repository found only inRepoDigests, using a bare repository name. The previous behavior emitted<none>:<none>wheneverRepoTagswas empty, and the CLI relies on that sentinel, so a digest-only image can change from a dangling row torepo/<none>in the default listing. Skip digest-only repositories when!digestsand let the fallback below create the unnamed row.
if (!digests)
{
rows.push_back({&e, repoName, std::string{}});
test/windows/wslc/e2e/WSLCE2EImageListTests.cpp:567
- All of these e2e cases use tar-loaded images with no
RepoDigests, so none exercises the non-empty digest path: service field population,repo@sha256reduction, or per-digest row expansion. The tests would still pass if those new branches were broken; add a deterministic local-registry fixture or a direct service/session fixture containing a real digest.
// The fixtures carry no repo digest, so every row reports the placeholder.
VERIFY_ARE_EQUAL(std::string{c_none}, image.Digest);
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (3)
src/windows/wslcsession/WSLCSession.cpp:1981
- These lookup keys use
Repository.Name, which preserves the daemon's original token. IfRepoTagscontainsubuntu:latestwhileRepoDigestscontainsdocker.io/library/ubuntu@sha256:..., the lookup at lines 1996-1997 misses, so--digestsprints<none>for the tagged row and an extra digest-only row. Key both collections by a normalized repository name while retaining the appropriate familiar/original name for display, and cover mixed qualification with a test.
digestsByRepo[reference->Repository.Name].push_back(repoDigest);
src/windows/wslcsession/WSLCSession.cpp:2009
- This expands one tagged repository/tag into one output row per
RepoDigestsentry.ImageService::Listand both formatters consume eachWSLCImageInformationas a row, so an image carrying two digests for the same repository will duplicate the REPOSITORY/TAG/IMAGE ID rows. Docker'simages --digestskeeps one row per tag and puts the digest in that row; preserve the one-row-per-tag behavior and select the digest to display instead of expanding the row set.
for (const auto& repoDigest : it->second)
{
rows.push_back({&e, tag, repoDigest});
test/windows/wslc/e2e/WSLCE2EImageListTests.cpp:540
- This assertion searches the entire table output, but every fixture digest is
<none>and that placeholder can already occur in REPOSITORY or TAG. The test can therefore pass even if the DIGEST cells are missing or empty, leaving the real sha256 table/json consistency path untested; assert the value within the DIGEST column (or add a pulled-image table case).
const auto digest = wsl::shared::string::MultiByteToWide(image.Digest);
VERIFY_IS_TRUE(
tableResult.StdoutContainsSubstring(digest),
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
A critical row-cardinality regression remains, and multi-digest E2E coverage is incomplete.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
test/windows/wslc/e2e/WSLCE2EImageListTests.cpp:554
- This test claims to cover a tag with several digests, but it uses the tar-loaded fixtures and then requires every
Digestto be<none>at lines 566-567. It therefore never exercises the multi-digest path added inWSLCSession.cpp:2007-2010; an implementation that ignored or collapsed additionalRepoDigestswould still pass. Add a repository/tag state with at least two real repo digests and assert the expected rows.
// A tag carrying several digests is emitted once per digest, so
// rows stay unique per repository, tag and digest while the image ID repeats across them.
std::map<std::pair<std::string, std::string>, std::string> idByRepoTag;
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate digest-grouping issues remain unresolved, along with one test-coverage nit.
Review details
Suppressed comments (4)
src/windows/wslc/tasks/ImageTasks.cpp:260
- The real-digest registry test validates only JSON; the table tests use tar-loaded images whose DIGEST is always
<none>. A regression in the non-empty digest table cell would therefore pass. Extend the registry scenario to runimage list --digestsand assert each pulled digest appears in the DIGEST column.
table.WriteRow({
MultiByteToWide(entry.Repository),
MultiByteToWide(entry.Tag),
digests ? MultiByteToWide(entry.Digest) : std::wstring{},
MultiByteToWide(entry.ID),
src/windows/wslcsession/WSLCSession.cpp:2020
- This digest-only repository loop runs even when
digestsis false: the!digestsbranch appends a row for everyRepoDigestsentry. For a tagless image whose daemon response includes repo digests, the default listing therefore changes from the previous<none>:<none>row to repository-only rows, even though digest references were not requested. Only enumerate these repositories whendigestsis true so the fallback below preserves the no-flag behavior.
for (const auto& [repoName, repoDigests] : digestsByRepo)
{
if (taggedRepos.contains(repoName))
{
continue;
src/windows/wslcsession/WSLCSession.cpp:1981
- This correlation is keyed by
Repository.Name, but that field intentionally preserves the original spelling whileRepositoryReference::Parsenormalizes equivalent names intoServer/Path(for example,ubuntuanddocker.io/library/ubuntu). If the daemon uses those valid spellings acrossRepoTagsandRepoDigests, the lookup misses, leaving the tagged row at<none>and emitting an extra digest-only row. Use a canonicalServer/Pathkey for matching while retaining the original name for display.
digestsByRepo[reference->Repository.Name].push_back(repoDigest);
src/windows/wslcsession/WSLCSession.cpp:2010
- The new multi-digest expansion path is not exercised by the added E2E coverage:
Digests_RepeatsIdOncePerDigestuses tar-loaded images and asserts every digest is<none>, while the registry test creates only one digest per repository. Add a fixture with at least two repo digests (or a digest-only repository) and assert the resulting row/quiet multiplicity so this loop cannot regress silently.
for (const auto& repoDigest : it->second)
{
rows.push_back({&e, tag, repoDigest});
}
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary of the Pull Request
Adds
--digeststowslc image list(and itsimage ls/imagesspellings), matchingdocker images --digests. The DIGEST column was previously hardcoded to<none>in json output and absent from the table.PR Checklist
Detailed Description of the Pull Request / Additional comments
WSLCListImagesFlagsDigestswas already defined and already honored end to end —WSLCSession::ListImagesforwards it toDocker().ListImages(all, digests, filters)and populatesWSLCImageInformation::Digest. The CLI never set the flag and never read the field, soToImageOutputhardcodedDigestto<none>.ArgType::Digests(--digests, no short alias, matching docker where only listing commands'--filtercarries-f).models::ImageInformationgains aDigestfield, andImageService::Listtakes a defaultedbool digeststhat sets the flag and populates it.--digestsis passed, matching docker'sdefaultImageTableFormatWithDigest.repo@sha256:...); docker's DIGEST column shows only the digest. NewDigestFromRepoDigesthelper inImageModel.hperforms that reduction and is unit tested directly.needDigest(ctx), which isctx.Digest || format.Contains("{{.Digest}}"), and--format jsonsatisfies neither. Soimage list --format jsonstill reports<none>, and only--digests --format jsonreports a real digest. An earlier revision of this change leaked the digest into plain json output; the tests below lock the gated behavior in.constexprvalues so the 5- and 6-column tables cannot drift.Validation Steps Performed
All new and existing tests in the touched areas were run against a locally deployed build. 25/25 pass.
Unit tests (
WSLCCLIImageDigestUnitTests, new file, 3 tests):DigestFromRepoDigest_StripsRepositoryPrefix— plain, fully qualified, and registry-with-port repositories all reduce to the baresha256:digest. The port case guards the split from being confused by a colon.DigestFromRepoDigest_PassesThroughBareValues— empty, already-bare, and<none>values are untouched.DigestFromRepoDigest_SplitsOnFirstSeparator— splitting happens once, so a digest is never truncated further.Unit test (
WSLCCLICommandUnitTests):ImageListCommand_HasDigestsArgument— both theimage listand root-scopedimagesconstructions register--digestsas an optional flag with no short alias.E2E tests (
WSLCE2EImageListTests, 22 total, 5 new):Digests_AddsColumnBetweenTagAndImageId— asserts DIGEST sits after TAG and before IMAGE ID, and that the default listing does not gain the column.Digests_ReportsDigestOnlyWhenRequested— json reports<none>without the flag; anything reported with the flag must be a baresha256:digest with norepo@prefix.Digests_TableMatchesJson— every digest in json output appears in the table's DIGEST column, so the two renderings cannot drift.Digests_QuietStillOutputsIdsOnly—--quietwins over--digests, emitting bare ids with no header, as docker does.Digests_ListedInHelp—--digestsand its localized description appear inimage list --help.Note on the test images: they are provisioned via
image loadfrom a tarball, so they carry no repo digest and correctly report<none>even with--digests— the same as docker for a tar-loaded image. The digest-reduction logic is therefore covered by the unit tests rather than by asserting a live registry digest in e2e, which would make the suite network dependent.