opts: MountOpt: improve validation, and refactor #6773
Conversation
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
added
status/2-code-review
area/volumes
kind/refactor
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refactors the mount option parsing and validation code to improve code organization and maintainability. The changes extract validation logic into a separate utility file and enhance error messages for better user experience.
Changes:
- Extracts mount validation and helper functions to a new
opts/mount_utils.gofile - Improves error messages with better formatting (adding quotes around option names and more descriptive messages)
- Optimizes data structures by not pre-allocating empty maps when unnecessary
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| opts/mount_utils.go | New utility file containing validation functions (validateMountOptions, validateExclusiveOptions, parseBoolValue) and helper functions for ensuring mount options are initialized |
| opts/mount.go | Refactored to use utility functions, improved whitespace validation, and enhanced error messages |
| opts/mount_test.go | Updated test expectations to match new error messages and cleaner data structures (no empty maps) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This splits the validation code from parsing code, potentially allowing
us to either fully deferring it to the daemon, or to perform validation
separately.
For reference; daemon-side validation currently (docker 29.2.0) produces;
docker run --rm --mount type=bind,src=/var/run,target=/foo,bind-recursive=writable alpine
docker: Error response from daemon: mount options conflict: !ReadOnly && BindOptions.ReadOnlyNonRecursive
docker run --rm --mount type=bind,src=/var/run,target=/foo,bind-recursive=readonly alpine
docker: Error response from daemon: mount options conflict: !ReadOnly && BindOptions.ReadOnlyForceRecursive
Validation for BindOptions.Propagation is currently missing on the daemon;
docker run --rm --mount type=bind,src=/var/run,target=/foo,bind-recursive=readonly,readonly alpine
# no error
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
improve_mountopts
branch
from
7ed22ab to
df3e923
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO: implicitly set propagation and error if the user specifies a propagation in a future refactor/UX polish pass | ||
| // https://github.com/docker/cli/pull/4316#discussion_r1341974730 | ||
| default: | ||
| return fmt.Errorf(`invalid value for %s: %s (must be "enabled", "disabled", "writable", or "readonly")`, key, val) |
There was a problem hiding this comment.
Error message format is inconsistent with newly added error messages. The new validation code uses quotes around the key (e.g., "invalid value for '%s'"), but this existing error message doesn't use quotes. Consider updating to match the new format: "invalid value for '%s': %s"
| return fmt.Errorf(`invalid value for %s: %s (must be "enabled", "disabled", "writable", or "readonly")`, key, val) | |
| return fmt.Errorf(`invalid value for '%s': %s (must be "enabled", "disabled", "writable", or "readonly")`, key, val) |
There was a problem hiding this comment.
Think I had some changes stashed to align these; probably fine for a follow-up
| case "tmpfs-size": | ||
| sizeBytes, err := units.RAMInBytes(val) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid value for %s: %s", key, val) |
There was a problem hiding this comment.
Error message format is inconsistent with newly added error messages. The new validation code uses quotes around the key (e.g., "invalid value for '%s'"), but this existing error message doesn't use quotes. Consider updating to match the new format: "invalid value for '%s': %s"
| case "tmpfs-mode": | ||
| ui64, err := strconv.ParseUint(val, 8, 32) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid value for %s: %s", key, val) |
There was a problem hiding this comment.
Error message format is inconsistent with newly added error messages. The new validation code uses quotes around the key (e.g., "invalid value for '%s'"), but this existing error message doesn't use quotes. Consider updating to match the new format: "invalid value for '%s': %s"
3 participants
See individual commits for details.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)