wslc: add --details to container logs for docker parity - #41467
wslc: add --details to container logs for docker parity#41467ggarzia-MSFT wants to merge 5 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds Docker CLI parity for wslc container logs/wslc logs by introducing a --details flag, plumbing it through the CLI/task/service layers into the Docker Engine GET /containers/{id}/logs request, and validating behavior via parser, unit, and E2E tests.
Changes:
- Introduce a new
ArgType::Detailsflag (no short alias) forcontainer logsand ensure help text is localized. - Thread the
detailsoption throughContainerTasks→ContainerService→ container COM flags → Docker HTTP query parameter. - Add parser test cases, a CLI command unit test, and E2E coverage (including “no short alias” enforcement and help output).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/wslc/WSLCCLICommandUnitTests.cpp | Adds a unit test asserting --details exists and has no short alias. |
| test/windows/wslc/e2e/WSLCE2EContainerLogsTests.cpp | Adds E2E coverage for --details, -d rejection, and help output. |
| test/windows/wslc/CommandLineTestCases.h | Extends command-line parsing coverage for --details and invalid values/alias behavior. |
| src/windows/wslcsession/DockerHTTPClient.cpp | Adds details query parameter on container logs requests when the new flag is set. |
| src/windows/wslc/tasks/ContainerTasks.cpp | Parses --details and forwards it into the logs service call. |
| src/windows/wslc/services/ContainerService.h | Extends ContainerService::Logs API to include a details boolean. |
| src/windows/wslc/services/ContainerService.cpp | Converts the details boolean into WSLCLogsFlagsDetails. |
| src/windows/wslc/commands/ContainerLogsCommand.cpp | Registers the --details argument for the logs command. |
| src/windows/wslc/arguments/ArgumentDefinitions.h | Defines the Details argument (NO_ALIAS) and binds its localized description. |
| src/windows/service/inc/WSLCShared.idl | Adds WSLCLogsFlagsDetails and updates valid-flag mask. |
| localization/strings/en-US/Resources.resw | Adds localized help text for the new --details flag. |
💡 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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
test/windows/wslc/e2e/WSLCE2EContainerLogsTests.cpp:79
- This test currently asserts an exact stdout string for
--details(including the exact prefix formatting). That makes the E2E test brittle against Docker engine/log formatting changes that still preserve the semantic requirement (details output differs from plain output). Consider verifying exit code + presence of expected log lines, and keep the key invariant (plain != details) without pinning the full formatting.
auto details = RunWslc(std::format(L"container logs --details {}", WslcContainerName));
details.Verify({.Stdout = L" detail1\n detail2\n", .Stderr = L"", .ExitCode = 0});
VERIFY_ARE_NOT_EQUAL(plain.Stdout.value(), details.Stdout.value());
// --details composes with the other log options.
test/windows/wslc/e2e/WSLCE2EContainerLogsTests.cpp:88
- The help test hard-codes the English description text. Since help output is localized and the string already has a
Localization::WSLCCLI_DetailsArgDescription()resource, using the localization accessor will keep the test stable if the wording changes.
auto result = RunWslc(L"container logs --help");
result.Verify({.Stderr = L"", .ExitCode = 0});
VERIFY_IS_TRUE(result.StdoutContainsSubstring(L"--details"));
VERIFY_IS_TRUE(result.StdoutContainsSubstring(L"Show extra details provided to logs"));
}
| COMMAND_LINE_TEST_CASE(L"container logs --details cont1", L"logs", true) | ||
| COMMAND_LINE_TEST_CASE(L"logs --details cont1", L"logs", true) | ||
| COMMAND_LINE_TEST_CASE(L"container logs --details=true cont1", L"logs", true) | ||
| COMMAND_LINE_TEST_CASE(L"container logs --details=false cont1", L"logs", true) | ||
| COMMAND_LINE_TEST_CASE(L"container logs --details=invalid cont1", L"logs", false) | ||
| COMMAND_LINE_TEST_CASE(L"container logs --details --timestamps --tail 5 cont1", L"logs", true) |
There was a problem hiding this comment.
But what if another option adds a d alias... this seems like improper negative testing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| <value>Run container in detached mode</value> | ||
| </data> | ||
| <data name="WSLCCLI_DetailsArgDescription" xml:space="preserve"> | ||
| <value>Show extra details provided to logs</value> |
There was a problem hiding this comment.
| <value>Show extra details provided to logs</value> | |
| <value>Show extra details for logs</value> |
"provided to" sounds awkward.
There was a problem hiding this comment.
🟢 Approval recommended
The --details option is plumbed consistently across layers, the flags validation is updated correctly, and targeted tests were added/updated to cover parsing and end-to-end behavior.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary of the Pull Request
Adds the
--detailsoption towslc container logs(and itswslc logsalias) for docker CLI parity.docker logs --detailsasks the daemon to include the extra attributes that were supplied to the container's logging driver alongside each log line.wslc logshad no equivalent, so those attributes were unreachable from the CLI.The flag is plumbed straight through to the Docker
GET /containers/{id}/logsendpoint as thedetailsquery parameter, alongside the existingfollow/timestamps/tailparameters.PR Checklist
Detailed Description of the Pull Request / Additional comments
Docker parity
docker/clicli/command/container/logs.godeclares the option as:Note
BoolVar, notBoolVarP— docker deliberately gives--detailsno short alias. This change matches that:ArgType::Detailsis registered withNO_ALIAS, socontainer logs -dis not accepted.Query parameter encoding
The flag needs no client-side interpretation; it is handed to
DockerHTTPClient::URL::SetParameteras abool, exactly like the neighbouringfollow,stdout,stderrandtimestampsparameters, so the request carriesdetails=trueordetails=false.That differs textually from docker's own client, which omits the parameter when false and sets it to
1when true (moby/mobyclient/container_logs.go):Both encodings are equivalent at the daemon, which parses the parameter with
httputils.BoolValue(moby/mobydaemon/server/httputils/form.go):"true"falls into the default branch and enables details;"false"is in the false list and is indistinguishable from omitting the parameter. Keeping thebooloverload therefore preserves observable behaviour while staying consistent with how every other boolean parameter inDockerHTTPClient.cppis serialized.Validation Steps Performed
Parser coverage (
CommandLineTestCases.h) — 6 new cases:container logs --details cont1and the rootlogs --details cont1alias--details=true/--details=falseexplicit forms--details=invalidrejected--detailscombined with--timestampsand--tailUnit tests (
WSLCCLICommandUnitTests.cpp):AllCommands_NoAmbiguousArgumentNamesOrAliasestree walk continues to pass, confirming the new argument introduces no name or alias collisions anywhere in the command tree.End-to-end tests (
WSLCE2EContainerLogsTests.cpp):WSLCE2E_Container_Logs_Details— runs a container emitting two lines, then verifies thatlogs --detailsexits 0 and that its output differs from a plainlogs. The daemon serializes the log driver attributes followed by a separator space, and emits that separator unconditionally, so a container with no configured attributes still gains exactly one leading space per line (" detail1\n detail2\n"versus"detail1\ndetail2\n"). That difference is the observable proof the flag reached the daemon. The test also checks that--details --tail 1still tails correctly.WSLCE2E_Container_Logs_Details_ListedInHelp— the flag and its description appear incontainer logs --help.