Skip to content

CLI: Initial support for volume commands#40139

Draft
AmelBawa-msft wants to merge 8 commits intofeature/wsl-for-appsfrom
user/amelbawa/volume
Draft

CLI: Initial support for volume commands#40139
AmelBawa-msft wants to merge 8 commits intofeature/wsl-for-appsfrom
user/amelbawa/volume

Conversation

@AmelBawa-msft
Copy link
Copy Markdown

Summary of the Pull Request

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Copilot AI review requested due to automatic review settings April 8, 2026 23:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds initial WSLC CLI support for managing named volumes (create/list/inspect/remove) and validates the new --driver argument, with E2E coverage to ensure named volumes can be used across containers.

Changes:

  • Introduces new wslc volume command group with create/remove/inspect/list subcommands.
  • Adds volume task/service/model plumbing to call existing session COM volume APIs and render output (table/json/quiet).
  • Extends Windows E2E tests to validate named volume persistence across container runs/creates.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp Adds E2E scenarios validating named-volume write/read and multi-container persistence with container run.
test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp Adds E2E scenarios validating named-volume persistence across container create/start and subsequent containers.
src/windows/wslc/tasks/VolumeTasks.h Declares volume task entry points used by the new commands.
src/windows/wslc/tasks/VolumeTasks.cpp Implements volume tasks: create/delete/list/inspect, including output formatting (table/json/quiet).
src/windows/wslc/services/VolumeService.h / .cpp Adds service wrapper over session COM APIs for volume CRUD/list/inspect.
src/windows/wslc/services/VolumeModel.h Introduces VolumeInformation model for list output + JSON serialization.
src/windows/wslc/core/ExecutionContextData.h Adds Data::Volumes mapping to store volume lists in the execution context.
src/windows/wslc/commands/VolumeCommand.* Adds root volume command and wires up subcommands.
src/windows/wslc/commands/Volume*Command.cpp Implements argument sets, descriptions, validation, and task pipelines for each volume subcommand.
src/windows/wslc/commands/RootCommand.cpp Registers volume under the CLI root.
src/windows/wslc/arguments/ArgumentDefinitions.h Adds VolumeDriver (--driver/-d) and positional VolumeName.
src/windows/wslc/arguments/ArgumentValidation.h / .cpp Adds validation for the --driver argument (currently supports vhd).
localization/strings/en-US/Resources.resw Adds localized help text for volume commands and new arguments.

--*/
#pragma once
#include "CLIExecutionContext.h"
#include "Task.h"
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task.h is included but not used in this header (unlike ContainerTasks.h, which defines a Task-derived type). Dropping the unused include would reduce unnecessary coupling and rebuilds.

Suggested change
#include "Task.h"

Copilot uses AI. Check for mistakes.

#pragma once

#include <wslc.h>
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<wslc.h> doesn’t appear to be needed for VolumeInformation (it only uses std::string and the nlohmann JSON macro). Consider removing the unused include to keep the model header lightweight, consistent with other model headers like ImageModel.h.

Suggested change
#include <wslc.h>

Copilot uses AI. Check for mistakes.
@benhillis benhillis force-pushed the user/amelbawa/volume branch from f5e1b45 to b851ae9 Compare April 9, 2026 18:45
Copilot AI review requested due to automatic review settings April 14, 2026 04:39
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 11 comments.

Comment on lines +66 to 67
_(Options, "opt", L"o", Kind::Value, L"Set driver specific options") \
_(Output, "output", L"o", Kind::Value, Localization::WSLCCLI_OutputArgDescription()) \
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new short options introduce an unambiguous parsing conflict: --opt uses -o, which is already used by --output. Similarly, --driver uses -d, which is already used by the existing --detach argument. Short-option collisions typically make argument parsing ambiguous or cause one flag to shadow the other. Fix by assigning unused short options (or NO_ALIAS) to Driver and Options.

Copilot uses AI. Check for mistakes.
/*_(DNSDomain, "dns-domain", NO_ALIAS, Kind::Value, Localization::WSLCCLI_DNSDomainArgDescription())*/ \
/*_(DNSOption, "dns-option", NO_ALIAS, Kind::Value, Localization::WSLCCLI_DNSOptionArgDescription())*/ \
/*_(DNSSearch, "dns-search", NO_ALIAS, Kind::Value, Localization::WSLCCLI_DNSSearchArgDescription())*/ \
_(Driver, "driver", L"d", Kind::Value, L"Specify volume driver name (default vhd)") \
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New end-user facing strings are hardcoded for --driver and --opt while nearby args use Localization::... accessors. This breaks localization consistency (and the PR checklist item). Prefer using localized resources for these descriptions (there is already WSLCCLI_VolumeDriverArgDescription in Resources.resw; --opt should get a similar resource).

Suggested change
_(Driver, "driver", L"d", Kind::Value, L"Specify volume driver name (default vhd)") \
_(Driver, "driver", L"d", Kind::Value, Localization::WSLCCLI_VolumeDriverArgDescription()) \

Copilot uses AI. Check for mistakes.
/*_(NoDNS, "no-dns", NO_ALIAS, Kind::Flag, Localization::WSLCCLI_NoDNSArgDescription())*/ \
_(NoPrune, "no-prune", NO_ALIAS, Kind::Flag, Localization::WSLCCLI_NoPruneArgDescription()) \
_(NoTrunc, "no-trunc", NO_ALIAS, Kind::Flag, Localization::WSLCCLI_NoTruncArgDescription()) \
_(Options, "opt", L"o", Kind::Value, L"Set driver specific options") \
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New end-user facing strings are hardcoded for --driver and --opt while nearby args use Localization::... accessors. This breaks localization consistency (and the PR checklist item). Prefer using localized resources for these descriptions (there is already WSLCCLI_VolumeDriverArgDescription in Resources.resw; --opt should get a similar resource).

Copilot uses AI. Check for mistakes.

// Create the named volume
auto result = RunWslc(std::format(L"volume create {}", volumeName));
result.Verify({.Stderr = L"", .ExitCode = S_OK});
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new assertions use S_OK for .ExitCode, while similar E2E tests in the other file use numeric process exit codes (0/1). Even though S_OK and 0 are equivalent, mixing conventions makes the tests harder to read and can confuse expectations about whether .ExitCode is an HRESULT or a process exit code. Consider standardizing on one representation across the E2E suite.

Copilot uses AI. Check for mistakes.
L"container run --rm --volume {}:/data {} sh -c \"echo -n 'named_volume_data' > /data/testfile\"",
volumeName,
AlpineImage.NameAndTag()));
result.Verify({.Stdout = L"", .Stderr = L"", .ExitCode = S_OK});
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new assertions use S_OK for .ExitCode, while similar E2E tests in the other file use numeric process exit codes (0/1). Even though S_OK and 0 are equivalent, mixing conventions makes the tests harder to read and can confuse expectations about whether .ExitCode is an HRESULT or a process exit code. Consider standardizing on one representation across the E2E suite.

Copilot uses AI. Check for mistakes.
L"container run --rm --volume {}:/data {} cat /data/testfile",
volumeName,
AlpineImage.NameAndTag()));
result.Verify({.Stdout = L"named_volume_data", .Stderr = L"", .ExitCode = S_OK});
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new assertions use S_OK for .ExitCode, while similar E2E tests in the other file use numeric process exit codes (0/1). Even though S_OK and 0 are equivalent, mixing conventions makes the tests harder to read and can confuse expectations about whether .ExitCode is an HRESULT or a process exit code. Consider standardizing on one representation across the E2E suite.

Copilot uses AI. Check for mistakes.
Comment on lines +544 to +547
RunWslc(std::format(L"volume rm {}", volumeName));

// Create the named volume
auto result = RunWslc(std::format(L"volume create {}", volumeName));
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests rely on explicit cleanup at the end of the method. If a Verify(...) throws/fails mid-test, the final cleanup line won’t execute, which can leave volumes behind and cause later E2E runs to become flaky (e.g., name collisions). A more robust pattern is to use a scope guard/RAII cleanup so volume rm always runs even on early failure.

Copilot uses AI. Check for mistakes.
result.Verify({.Stdout = L"run_vol_test", .Stderr = L"", .ExitCode = 0});

// Cleanup
RunWslc(std::format(L"volume rm {}", volumeName));
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests rely on explicit cleanup at the end of the method. If a Verify(...) throws/fails mid-test, the final cleanup line won’t execute, which can leave volumes behind and cause later E2E runs to become flaky (e.g., name collisions). A more robust pattern is to use a scope guard/RAII cleanup so volume rm always runs even on early failure.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings April 14, 2026 20:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.

Comment on lines +49 to +56
if (execArgs.Contains(ArgType::Format))
{
auto format = execArgs.Get<ArgType::Format>();
if (!IsEqual(format, L"json") && !IsEqual(format, L"table"))
{
throw CommandException(Localization::WSLCCLI_InvalidFormatError());
}
}
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a command-specific format validation that duplicates logic already centralized in validation::GetFormatTypeFromString(...) (used later by ListVolumes). To avoid drift/inconsistent error details, prefer using the shared validation helper here as well (or remove this override and rely on the existing validation path).

Suggested change
if (execArgs.Contains(ArgType::Format))
{
auto format = execArgs.Get<ArgType::Format>();
if (!IsEqual(format, L"json") && !IsEqual(format, L"table"))
{
throw CommandException(Localization::WSLCCLI_InvalidFormatError());
}
}
UNREFERENCED_PARAMETER(execArgs);
// Format validation is handled by the shared validation path used by ListVolumes.

Copilot uses AI. Check for mistakes.
Comment on lines +218 to +223
auto result = RunWslc(L"volume list --format json");
result.Verify({.Stderr = L"", .ExitCode = 0});
auto volumes = wsl::shared::FromJson<std::vector<wsl::windows::wslc::models::VolumeInformation>>(result.Stdout.value().c_str());
for (const auto& vol : volumes)
{
if (vol.Name == wsl::shared::string::WideToMultiByte(volumeName))
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WideToMultiByte(volumeName) is recomputed on every iteration in both VerifyVolumeIsListed and VerifyVolumeIsNotListed. Convert once before the loop (e.g., auto needle = WideToMultiByte(volumeName);) and compare against that to avoid repeated allocations/conversions.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants