Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new exhaustive regression test omits standalone coverage for three cluster-specific flags.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes cluster-volume option detection and corrects capacity help text.
Changes:
- Detects secret and topology flags as cluster options.
- Corrects minimum/maximum capacity descriptions.
- Adds per-flag regression tests.
| File | Description |
|---|---|
cli/command/volume/create.go |
Fixes cluster detection and help text. |
cli/command/volume/create_test.go |
Adds option-detection tests. |
docs/reference/commandline/volume_create.md |
Updates generated capacity documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Availability: volume.AvailabilityDrain, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Good catch, thanks. Added standalone cases for --scope, --sharing and --type and an AccessMode assertion, so each of the three is now exercised on its own. I checked that misspelling any of them in hasClusterVolumeOptionSet makes exactly that subtest fail. Squashed into the same commit.
hasClusterVolumeOptionSet decides whether a ClusterVolumeSpec is sent with the request. It looked up a flag named "secrets", but the flag is registered as "secret", so pflag's Changed() always returned false for it. The "topology-required" and "topology-preferred" flags were not checked at all. As a result, running "docker volume create" with only one of those options (for example "--secret key=val" or "--topology-required region=R1") silently created a plain, non-cluster volume and dropped the option without any warning. Also swap the help text of --limit-bytes and --required-bytes, which were reversed: RequiredBytes is the minimum capacity and LimitBytes is the maximum capacity, as documented on volume.CapacityRange. Add a test that sets each cluster-only flag on its own and verifies the resulting ClusterVolumeSpec. Signed-off-by: Fadi Romdhan <fadiromdhan3@gmail.com>
33452d8 to
cc669bb
Compare
|
Heads-up on the noise here: this morning my fork was hit by a git worm that used my credentials to force-push a payload commit ( |
Summary
hasClusterVolumeOptionSetdecides whether aClusterVolumeSpecis attached to the create request. It checked for a flag namedsecrets, but the flag is registered assecret, soflags.Changed("secrets")was always false. The--topology-requiredand--topology-preferredflags were not part of the check at all.Running
docker volume createwith only one of those options, for example:therefore created a plain, non-cluster volume and silently dropped the option.
This PR:
hasClusterVolumeOptionSetand adds the two topology flags to it--limit-bytes/--required-bytes, which were reversed:RequiredBytesis the minimum capacity andLimitBytesthe maximum, per thevolume.CapacityRangedocs. The generated markdown table is updated to matchTestVolumeCreateClusterOptionDetection, which sets each cluster-only flag on its own and checks that the resultingClusterVolumeSpeccarries it. The new test fails forsecret,topology-requiredandtopology-preferredon master and passes with this changeVerify with:
$ go test ./cli/command/volume/ -run TestVolumeCreate -vRelease notes (optional)