Various improvements to PullImage() + custom registry support#14549
Various improvements to PullImage() + custom registry support#14549benhillis merged 10 commits intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves WSLA image pulling by normalizing image repositories to support non-library/ and non-docker.io registries, and by surfacing pull errors that can arrive during the progress stream after an initial HTTP 200 response.
Changes:
- Add
wslutil::NormalizeRepo()and use it inDockerHTTPClient::PullImage()to construct a fully-qualified pull reference. - Update
WSLASession::PullImage()to capture streamed error details from progress JSON and improve HTTP error parsing using response headers. - Expand WSLA test coverage for repo normalization and pulling additional image reference shapes; add a new localized string.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WSLATests.cpp | Adjusts pull tests to cover non-library/custom-registry cases; adds NormalizeRepo unit tests. |
| src/windows/wslasession/WSLASession.cpp | Enhances PullImage streaming/error handling by tracking HTTP response and parsing progress JSON errors. |
| src/windows/wslasession/DockerHTTPClient.cpp | Uses NormalizeRepo() to build the pull fromImage parameter for custom registries. |
| src/windows/inc/docker_schema.h | Extends pull progress schema to include optional errorDetail. |
| src/windows/common/wslutil.h | Declares NormalizeRepo() API. |
| src/windows/common/wslutil.cpp | Implements repo normalization logic aligned with distribution/reference behavior. |
| localization/strings/en-US/Resources.resw | Adds a new localized string for invalid image repo messaging. |
Comments suppressed due to low confidence (1)
test/windows/WSLATests.cpp:5992
- In
ImageParsinginvalid inputs,ParseImage("")is asserted twice. This is redundant and makes the test a bit harder to scan; consider removing the duplicate assertion.
// Invalid inputs
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(""); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(":debian:latest"); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage("debian:latest@"); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(""); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(":"); }), E_INVALIDARG);
| if (domain != localhost && domain != legacyDomain && domain.find_first_of(".:") == std::string::npos && !std::ranges::any_of(domain, isupper)) | ||
| { |
There was a problem hiding this comment.
std::ranges::any_of(domain, isupper) can invoke undefined behavior because isupper expects an unsigned char value (or EOF), but char may be signed and negative for non-ASCII bytes. Use std::isupper(static_cast<unsigned char>(c)) (and qualify with std::) in a lambda predicate to avoid UB.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
test/windows/WSLATests.cpp:5992
ParseImage("")is asserted twice in the invalid-inputs section, which is redundant and makes the test noisier than necessary. Keep only one of the duplicate checks.
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(""); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(":debian:latest"); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage("debian:latest@"); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(""); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(":"); }), E_INVALIDARG);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
test/windows/WSLCTests.cpp:1
- The
ParseImage(\"\")invalid-input assertion is duplicated, which adds noise without increasing coverage. Remove one of the duplicates to keep the test focused.
/*++
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
test/windows/WSLCTests.cpp:6013
- The invalid-input assertions include
ParseImage("")twice, which is redundant and makes it harder to see what’s actually being covered. Remove one of the duplicate checks.
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(""); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(":debian:latest"); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage("debian:latest@"); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(""); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(":"); }), E_INVALIDARG);
| auto comError = wsl::windows::common::wslutil::GetCOMErrorInfo(); | ||
| VERIFY_IS_TRUE(comError.has_value()); | ||
|
|
||
| // The error message can't be compared directly because it contains an unpredicable path: |
There was a problem hiding this comment.
Typo in comment: "unpredicable" → "unpredictable".
| // The error message can't be compared directly because it contains an unpredicable path: | |
| // The error message can't be compared directly because it contains an unpredictable path: |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
test/windows/WSLCTests.cpp:6013
ParseImage("")is asserted twice in the invalid-inputs section, which is redundant and makes failures noisier. Drop one of the duplicate checks (keep a single assertion for the empty-string case).
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(""); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(":debian:latest"); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage("debian:latest@"); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(""); }), E_INVALIDARG);
VERIFY_ARE_EQUAL(wil::ResultFromException([]() { ParseImage(":"); }), E_INVALIDARG);
| validatePull("pytorch/pytorch", "pytorch/pytorch:latest"); | ||
| validatePull("registry.k8s.io/pause:3.2", "registry.k8s.io/pause:3.2"); | ||
|
|
There was a problem hiding this comment.
PullImageAdvanced is still SKIP_TEST_UNSTABLE() and the nearby TODO mentions enabling once custom registries are supported, but this PR adds custom-registry repo normalization and expands coverage for non-docker.io repos. Update the TODO/skip rationale so it matches the current behavior/intent of the test.
Summary of the Pull Request
This change solves two issues with PullImage()
library/ docker.io registryThanks to @kevpar for his help in figuring out the right way to normalize repos.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed