--mount: support arbitrary mount-argument order #7655
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
Support an arbitrary order in which arguments are specified to the `--mount` flag. Previously, Podman expected `type=...` to come first which was breaking compatibility with Docker. Note that this is the ground work to default to "volume" (again Docker compat). However, this will require some further massaging as we have to assign a name. Fixes: containers#7628 Signed-off-by: Valentin Rothberg <[email protected]>
|
LGTM |
| arr := strings.SplitN(mount, ",", 2) | ||
| if len(arr) < 2 { | ||
| return nil, nil, errors.Wrapf(errInvalidSyntax, "%q", mount) | ||
| // TODO: Docker defaults to "volume" if no mount type is specified. |
There was a problem hiding this comment.
Why not just make this default in findMountType?
There was a problem hiding this comment.
See commit message:
Note that this is the ground work to default to "volume" (again Docker
compat). However, this will require some further massaging as we have
to assign a name.
Defaulting to "volume" fails as volumes need to be named. I am sure we can tackle it much easier now but refrained from doing that in the bug fix.
There was a problem hiding this comment.
Volumes don't technically need to be named. If you pass libpod a named volume with no name, we will make an anonymous volume (pseudorandom ID as name)
There was a problem hiding this comment.
Curious. I got an error when playing with the default. Maybe it was a red herring.
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Support an arbitrary order in which arguments are specified to the
--mountflag. Previously, Podman expectedtype=...to comefirst which was breaking compatibility with Docker.
Note that this is the ground work to default to "volume" (again Docker
compat). However, this will require some further massaging as we have
to assign a name.
Fixes: #7628
Signed-off-by: Valentin Rothberg [email protected]