Skip to content

Conversation

AkihiroSuda
Copy link
Member

- What I did

Revert #28838
Update #28527

Plese refer to #28527 (comment) for the discussion so far.

This PR adds --mount to docker run.

The syntax of docker run --mount is identical to docker service create --mount.

Some notes:

  • --volume-driver is ignored for --mounts. A warning will be printed on the client side when specified.
  • --mount still only supports "CSV" form. i.e. --mount type=volume,src=foo,dst=/bar works but --mount foo:/bar does NOT work.
  • user can mix up --mount and -v simultaneously.

- How I did it

Revert #28838 + warning about "--volume-driver is ignored for --mount volumes"

- How to verify it

$ docker run --rm --mount type=bind,src=/host-foo,dst=/foo --mount type=bind,src=/host-bar,dst=/bar -v /host-baz:/baz  busybox ls /foo /bar /baz
/bar:
..
/baz:
..
/foo:
..
$ docker run --volume-driver foo --mount type=volume,dst=/foo -it --rm busybox
WARN[0000] `--volume-driver` is ignored for volumes specified via `--mount`. Use `--mount type=volume,volume-driver
...

- Description for the changelog

cli: add --mount to docker run

- A picture of a cute animal (not mandatory but encouraged)
penguins

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@stevvooe
Copy link
Contributor

LGTM

@cpuguy83
Copy link
Member

Moving to code review.

Choose a reason for hiding this comment

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

mnt1 + ":/foo" instead of Sprintf?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Choose a reason for hiding this comment

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

mnt1 + ":/foo" instead of Sprintf?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@aaronlehmann
Copy link

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

one nit, LGTM otherwise 😅

(sorry, forgot to submit my review)

Copy link
Member

Choose a reason for hiding this comment

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

Can you also add the --mount flag to OPTIONS below?

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda
Copy link
Member Author

@thaJeztah done
cc @icecrime @mstanleyjones

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants