cli/command/container: parse: remove client-side warning#5579
cli/command/container: parse: remove client-side warning#5579thaJeztah merged 1 commit intodocker:masterfrom
Conversation
remove a client-side warning about volume drivers combined with "mounts" in favor of producing the warning on the daemon side. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
added
status/2-code-review
kind/refactor
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5579 +/- ##
=======================================
Coverage 59.60% 59.60%
=======================================
Files 345 345
Lines 29103 29099 -4
=======================================
- Hits 17346 17345 -1
+ Misses 10788 10786 -2
+ Partials 969 968 -1 |
| mounts := copts.mounts.Value() | ||
| if len(mounts) > 0 && copts.volumeDriver != "" { | ||
| logrus.Warn("`--volume-driver` is ignored for volumes specified via `--mount`. Use `--mount type=volume,volume-driver=...` instead.") | ||
| } |
There was a problem hiding this comment.
Should this be API-version gated to match the lack of warning on pre 1.48 daemons?
Considering a case where a >=1.48 client talks to a <1.48 daemon.
There was a problem hiding this comment.
I was briefly considering if, but in all honesty, I think it's a very corner-case on its own, and even more so considering a current CLI connecting to an older daemon, so I thought the world wouldn't explode if someone would miss that warning.
The reason I stated looking into this was that the current warning is not very user-friendly because we use logrus with the default formatting;
docker run --volume-driver something --rm --mount=type=volume,target=/toto busybox
time="2024-10-26T20:20:54+02:00" level=warning msg="`--volume-driver` is ignored for volumes specified via `--mount`. Use `--mount type=volume,volume-driver=...` instead."I started a PR to change it to a bare fmt.Printfln(), but even wondered if logging should happening here at all (this might be a function to export somewhere at some point to be used as library code). Then considered that we already have warnings, but those were returned by the API (which is somewhat better as it can act as the canonical source of truth here).
There was a problem hiding this comment.
Oh, LOL, and to make my "Yak-shaving" tour complete; The acttual reason I arrived here in the first place was that I was considering that we should deprecate --volume-driver, and instead require explicitly setting --mount with the desired options. I still need to consider the full scope of that, because --volume-driver can be used as a default for anonymous volumes (but not sure if we really think that's a good idea - perhaps?) and even in that case, perhaps it's still better for the user to explicitly define that they want a specific volume to use a specific driver.
3 participants
remove a client-side warning about volume drivers combined with "mounts" in favor of producing the warning on the daemon side.
- A picture of a cute animal (not mandatory but encouraged)