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

config: relax specification of Config.Volumes #694

Merged
merged 2 commits into from Jun 26, 2017

Conversation

Projects
None yet
5 participants
@stevvooe
Contributor

stevvooe commented Jun 9, 2017

Relaxes the specification of Config.Volumes by avoiding references to
the concept of "data volumes". Implementors are merely instructed to
provide mounts outside the container's root filesystem.

Signed-off-by: Stephen J Day stephen.day@docker.com

Closes #687
Supersedes #504

cc @vbatts @cyphar

@stevvooe stevvooe added this to the v1.0.0 milestone Jun 9, 2017

config.md Outdated
A set of directories which SHOULD be created as data volumes in a container running this image.
If a file or folder exists within the image with the same path as a data volume, that file or folder will be replaced by the data volume and never be merged.
A set of directories describing where the process is likely write data specific to a container instance.
Implementations SHOULD provide mounts for these locations such that application data is not written to the container's root filesystem.

This comment has been minimized.

@vbatts

vbatts Jun 16, 2017

Member

In this sentence, "implementations" seems to be directed at runtime implementations, right?

This comment has been minimized.

@stevvooe

stevvooe Jun 16, 2017

Contributor

I was explicitly vague on purpose. Should I qualify this further?

This comment has been minimized.

@cyphar

cyphar Jun 19, 2017

Member

IMO if we want to specify what a runtime should do we should put it in conversion.md, to avoid the mixing of semantics between the two.

config.md Outdated
If a file or folder exists within the image with the same path as a data volume, that file or folder will be replaced by the data volume and never be merged.
A set of directories describing where the process is likely write data specific to a container instance.
Implementations SHOULD provide mounts for these locations such that application data is not written to the container's root filesystem.
If a _new_ image is created from a container based on the image described by this configuration, data in these paths SHOULD NOT be included in the _new_ image.

This comment has been minimized.

@vbatts

vbatts Jun 16, 2017

Member

So we are excluding the use case of seeding a volume?

This comment has been minimized.

@stevvooe

stevvooe Jun 16, 2017

Contributor

No, this is covering the case where an image is created from a running container. This is pretty awkward.

This comment has been minimized.

@stevvooe

stevvooe Jun 21, 2017

Contributor

I'll add a line above this indicating that seeding volumes with image data is okay.

@vbatts

This comment has been minimized.

Member

vbatts commented Jun 16, 2017

needs a rebase to pass tests.
a couple questions, but SGTM

@stevvooe stevvooe force-pushed the stevvooe:relax-volumes-specification branch from f211f5f to 950dcf5 Jun 16, 2017

stevvooe added some commits Jun 9, 2017

config: relax specification of Config.Volumes
Relaxes the specification of `Config.Volumes` by avoiding references to
the concept of "data volumes". Implementors are merely instructed to
provide mounts outside the container's root filesystem.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
config, conversion: move volume behavior to conversion
Signed-off-by: Stephen J Day <stephen.day@docker.com>

@stevvooe stevvooe force-pushed the stevvooe:relax-volumes-specification branch from 950dcf5 to 8f42721 Jun 21, 2017

@stevvooe

This comment has been minimized.

Contributor

stevvooe commented Jun 21, 2017

@vbatts PTAL. Moved to conversion. Added clause about seeding the mount.

@erikh

This comment has been minimized.

Contributor

erikh commented Jun 23, 2017

Thanks for listening to my carping, I know it's a bit much at times. LGTM!

@stevvooe

This comment has been minimized.

Contributor

stevvooe commented Jun 23, 2017

Thanks for listening to my carping, I know it's a bit much at times. LGTM!

I think this language is much better. Thanks for the feedback!

@vbatts

This comment has been minimized.

Member

vbatts commented Jun 23, 2017

i think this covers this loosely enough.
LGTM

I would love to get @cyphar review as well

Approved with PullApprove

@philips

This comment has been minimized.

Contributor

philips commented Jun 23, 2017

LGTM

Approved with PullApprove

@stevvooe

This comment has been minimized.

Contributor

stevvooe commented Jun 23, 2017

@cyphar please... review... resisting... urge... to... merge... with... two... LGTMs.

@cyphar

This comment has been minimized.

Member

cyphar commented Jun 25, 2017

Haha, thanks for pinging me @stevvooe.

LGTM, sorry for the delay (exams are fun).

@vbatts

This comment has been minimized.

Member

vbatts commented Jun 26, 2017

👐

@vbatts vbatts merged commit d9ffa2f into opencontainers:master Jun 26, 2017

2 checks passed

code-review/pullapprove Approved by philips, vbatts
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stevvooe stevvooe deleted the stevvooe:relax-volumes-specification branch Jun 27, 2017

@vbatts vbatts referenced this pull request Jul 5, 2017

Merged

bump to v1.0.0-rc7 #708

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