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.

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

Add expanded mount format to stack deploy #30597

Merged
merged 4 commits into from Mar 14, 2017

Conversation

@dnephin
Member

dnephin commented Jan 31, 2017

Fixes #29193
Fixes #31224
Also (should) fix some windows support for volumes

I've added this to Compose format v3.1 for now, but it might be more appropriate for a v3.2, depending on when it gets merged and where it gets cherry-picked.

@cpuguy83 I'd appreciate your review of 3c74323. It adds a client-side parser for the old volume spec. As far as I know this is the first time we're attempting to fully parse the spec on the client (without being aware of the engine platform). It assumes no knowledge of the platform, so windows and linux container paths are both supported on all clients.

@shin- we will probably need to implement this on the python side as well. It should be a smaller change. I think we'll only need to copy the schema and implement something like 3c74323

cc @vdemeester

@vdemeester

SGTM 👼
I wonder how using volume fits with ongoing discussion on #28527 (volumes, mounts ?).

/cc @icecrime @stevvooe

services:
web:
image: busybox
volumes:

This comment has been minimized.

@stevvooe

stevvooe Feb 7, 2017

Contributor

Shouldn't these be called mounts? It is very confusing to have a volume declaration here then a volume declaration at the top level. This contributes to confusion. Not that this section will also handle binds, so calling it volumes is inaccurate.

@stevvooe

stevvooe Feb 7, 2017

Contributor

Shouldn't these be called mounts? It is very confusing to have a volume declaration here then a volume declaration at the top level. This contributes to confusion. Not that this section will also handle binds, so calling it volumes is inaccurate.

This comment has been minimized.

@dnephin

dnephin Feb 7, 2017

Member

Yes, but it's also a breaking change, so not something we can change in the v3.1 format. We missed our chance with v3, so we can queue it up for v4 I guess (depending on the outcome of the docker run flag change).

@dnephin

dnephin Feb 7, 2017

Member

Yes, but it's also a breaking change, so not something we can change in the v3.1 format. We missed our chance with v3, so we can queue it up for v4 I guess (depending on the outcome of the docker run flag change).

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Feb 7, 2017

Contributor

The format looks good, in general. I would really prefer that we call it mounts, or at least have that as an alias. It is very weird to have volumes, then type: volume. It is also problematic to mix binds, volume, tmpfs, (and soon image mounts!) into volume. We should focus the term volume around the orchestratable object.

Contributor

stevvooe commented Feb 7, 2017

The format looks good, in general. I would really prefer that we call it mounts, or at least have that as an alias. It is very weird to have volumes, then type: volume. It is also problematic to mix binds, volume, tmpfs, (and soon image mounts!) into volume. We should focus the term volume around the orchestratable object.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 7, 2017

Member

We could do a mount alias

Member

dnephin commented Feb 7, 2017

We could do a mount alias

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 16, 2017

Member

Rebased

Member

dnephin commented Feb 16, 2017

Rebased

}
switch errors[mostSpecificError].Type() {
case "number_one_of", "number_any_of":

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Feb 28, 2017

Member

Can we make these strings constant values?

@AkihiroSuda

AkihiroSuda Feb 28, 2017

Member

Can we make these strings constant values?

This comment has been minimized.

@dnephin

dnephin Feb 28, 2017

Member

sure

@dnephin

dnephin Feb 28, 2017

Member

sure

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack
Contributor

justincormack commented Mar 1, 2017

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 3, 2017

Contributor

Spoke with Daniel since I was having some issues trying to get this going. From a functionality POV, just have a couple of issues:

  • When "driver" is not defined on the volume, "driver_opts" are not passed through
  • The --mount cli supports templating everything about the mount spec, including Source which allows for some really powerful use-cases... this is kind of lost here afaict.
Contributor

cpuguy83 commented Mar 3, 2017

Spoke with Daniel since I was having some issues trying to get this going. From a functionality POV, just have a couple of issues:

  • When "driver" is not defined on the volume, "driver_opts" are not passed through
  • The --mount cli supports templating everything about the mount spec, including Source which allows for some really powerful use-cases... this is kind of lost here afaict.
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 3, 2017

Member

The --mount cli supports templating everything about the mount spec, including Source which allows for some really powerful use-cases... this is kind of lost here afaict.

I believe it's still possible for all fields (it even works with the old short syntax, see #30770) . For source it requires that you use an external volume, because the source name in the service has to be a reference to a volume definition. The volume definition can use a template:

data:
  external:
    name: '{{ .Template }}'
Member

dnephin commented Mar 3, 2017

The --mount cli supports templating everything about the mount spec, including Source which allows for some really powerful use-cases... this is kind of lost here afaict.

I believe it's still possible for all fields (it even works with the old short syntax, see #30770) . For source it requires that you use an external volume, because the source name in the service has to be a reference to a volume definition. The volume definition can use a template:

data:
  external:
    name: '{{ .Template }}'

dnephin added some commits Jan 19, 2017

Add expanded mount syntax to Compose schema and types.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Support expanded mounts in Compose loader
Add a test for loading expanded mount format.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Parse a volume spec on the client, with support for windows drives
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Convert new compose volume type to swarm mount type
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 8, 2017

Member

ping

Member

dnephin commented Mar 8, 2017

ping

@cpuguy83

LGTM

@vdemeester

Should we add a mount alias though ? 👼

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 10, 2017

Member

When it's supported by the CLI sure, but I think that's not required to merge this. We can add it later.

Member

dnephin commented Mar 10, 2017

When it's supported by the CLI sure, but I think that's not required to merge this. We can add it later.

@AkihiroSuda AkihiroSuda referenced this pull request Mar 11, 2017

Closed

[UX] Guidelines for short/long syntax flags #28527

2 of 3 tasks complete
@vdemeester

Sounds fair.
LGTM 🦁

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 13, 2017

Member

@dnephin can you open a pull request in the https://github.com/docker/docker.github.io repository for the syntax changes in the docker-compose file?

Member

thaJeztah commented Mar 13, 2017

@dnephin can you open a pull request in the https://github.com/docker/docker.github.io repository for the syntax changes in the docker-compose file?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin
Member

dnephin commented Mar 13, 2017

@dnephin dnephin referenced this pull request Mar 13, 2017

Merged

Compose file v3.2 #31795

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 13, 2017

Contributor

@dnephin what cli? Service is --mount as is.

Contributor

cpuguy83 commented Mar 13, 2017

@dnephin what cli? Service is --mount as is.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 14, 2017

Member

True. The compose file fields are named after the docker run cli. Either way we can't drop volumes as the key in a minor version, so we can either decide to add an alias of mounts during code freeze, or we can decide to worry about it for the next version.

Member

dnephin commented Mar 14, 2017

True. The compose file fields are named after the docker run cli. Either way we can't drop volumes as the key in a minor version, so we can either decide to add an alias of mounts during code freeze, or we can decide to worry about it for the next version.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Mar 14, 2017

Contributor

@dnephin is there any reason we can't do a major bump of compose format for 17.04?

Contributor

justincormack commented Mar 14, 2017

@dnephin is there any reason we can't do a major bump of compose format for 17.04?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 14, 2017

Member

You mean make it v4? There are no backwards incompatible changes, or any significantly large changes, so v4 would not be appropriate.

Member

dnephin commented Mar 14, 2017

You mean make it v4? There are no backwards incompatible changes, or any significantly large changes, so v4 would not be appropriate.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Mar 14, 2017

Contributor

@dnephin I meant so we can add mounts which you said was not suitable for a minor bump.

Contributor

justincormack commented Mar 14, 2017

@dnephin I meant so we can add mounts which you said was not suitable for a minor bump.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Mar 14, 2017

Contributor

Ok, lets leave any mounts change for another PR that bumps the API.

Contributor

justincormack commented Mar 14, 2017

Ok, lets leave any mounts change for another PR that bumps the API.

@justincormack justincormack merged commit 49376cd into moby:master Mar 14, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31363 has succeeded
Details
janky Jenkins build Docker-PRs 39981 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11049 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.04.0 milestone Mar 14, 2017

@dnephin dnephin deleted the dnephin:add-expanded-mount-format-to-stack-deploy branch Mar 14, 2017

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 14, 2017

Member

I see what you mean. I think we'll probably want to batch up a few other breaking changes all at once before making a v4.

Member

dnephin commented Mar 14, 2017

I see what you mean. I think we'll probably want to batch up a few other breaking changes all at once before making a v4.

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