Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cli/command/container: parse: remove client-side warning #5579

Merged
merged 1 commit into from
Oct 30, 2024
Merged

cli/command/container: parse: remove client-side warning #5579

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions cli/command/container/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/docker/docker/errdefs"
"github.com/docker/go-connections/nat"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/pflag"
cdi "tags.cncf.io/container-device-interface/pkg/parser"
)
Expand Down Expand Up @@ -364,10 +363,6 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
return nil, errors.Errorf("invalid value: %d. Valid memory swappiness range is 0-100", swappiness)
}

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.")
}
Comment on lines -367 to -370
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@thaJeztah thaJeztah Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

var binds []string
volumes := copts.volumes.GetMap()
// add any bind targets to the list of container volumes
Expand Down Expand Up @@ -697,7 +692,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
Tmpfs: tmpfs,
Sysctls: copts.sysctls.GetAll(),
Runtime: copts.runtime,
Mounts: mounts,
Mounts: copts.mounts.Value(),
MaskedPaths: maskedPaths,
ReadonlyPaths: readonlyPaths,
Annotations: copts.annotations.GetAll(),
Expand Down
Loading