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 VOLUME instruction to buildfile #1124

Merged
merged 3 commits into from Jul 12, 2013

Conversation

Projects
None yet
5 participants
@crosbymichael
Contributor

crosbymichael commented Jul 4, 2013

Add the VOLUME instruction to the Dockerfile builder.

FROM ubuntu
VOLUME ["/data"]

OR

FROM ubuntu
VOLUME /data
@shykes

This comment has been minimized.

Show comment
Hide comment
@shykes

shykes Jul 8, 2013

Collaborator

Hey @crosbymichael, thanks for contributing this. Would you mind adding a test which makes sure that the built image does indeed expose a volume at the given path? Your test does verify that the keyword doesn't cause a build to fail, but that's currently the case for all keywords since they are ignored by default.

Happy to help, and looking forward to merging this.

Collaborator

shykes commented Jul 8, 2013

Hey @crosbymichael, thanks for contributing this. Would you mind adding a test which makes sure that the built image does indeed expose a volume at the given path? Your test does verify that the keyword doesn't cause a build to fail, but that's currently the case for all keywords since they are ignored by default.

Happy to help, and looking forward to merging this.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jul 8, 2013

Contributor

Thanks for the feedback. Done!

Contributor

crosbymichael commented Jul 8, 2013

Thanks for the feedback. Done!

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Jul 10, 2013

Collaborator

@crosbymichael could you rebase to master ? tests are broken.

Thanks

Collaborator

vieux commented Jul 10, 2013

@crosbymichael could you rebase to master ? tests are broken.

Thanks

@shykes

This comment has been minimized.

Show comment
Hide comment
@shykes

shykes Jul 12, 2013

Collaborator

LGTM

Collaborator

shykes commented Jul 12, 2013

LGTM

@creack

This comment has been minimized.

Show comment
Hide comment
@creack

creack Jul 12, 2013

Contributor

LGTM

Contributor

creack commented Jul 12, 2013

LGTM

creack added a commit that referenced this pull request Jul 12, 2013

Merge pull request #1124 from crosbymichael/buildfile-volumes
+ Builder: Add VOLUME instruction to buildfile

@creack creack merged commit 637eceb into moby:master Jul 12, 2013

@matthewmueller

This comment has been minimized.

Show comment
Hide comment
@matthewmueller

matthewmueller Aug 18, 2013

Contributor

Is volume host:container supported? Or volume [host:container]?

I wasn't able to get that working.

Contributor

matthewmueller commented Aug 18, 2013

Is volume host:container supported? Or volume [host:container]?

I wasn't able to get that working.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Aug 18, 2013

Contributor

@matthewmueller no, host mounts are not supported in the Dockerfile

Contributor

crosbymichael commented Aug 18, 2013

@matthewmueller no, host mounts are not supported in the Dockerfile

@matthewmueller

This comment has been minimized.

Show comment
Hide comment
@matthewmueller

matthewmueller Aug 18, 2013

Contributor

@crosbymichael should they be? It seems like an asymmetry between the CLI and the Dockerfile.

Contributor

matthewmueller commented Aug 18, 2013

@crosbymichael should they be? It seems like an asymmetry between the CLI and the Dockerfile.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Aug 18, 2013

Contributor

No. Dockerfiles should be portable.

On Saturday, August 17, 2013, Matthew Mueller wrote:

@crosbymichael https://github.com/crosbymichael should they be? It
seems like an asymmetry between the CLI and the Dockerfile.


Reply to this email directly or view it on GitHubhttps://github.com/moby/moby/pull/1124#issuecomment-22824254
.

Thanks,


Michael Crosby
812-250-6603
crosby.michael@gmail.com

Contributor

crosbymichael commented Aug 18, 2013

No. Dockerfiles should be portable.

On Saturday, August 17, 2013, Matthew Mueller wrote:

@crosbymichael https://github.com/crosbymichael should they be? It
seems like an asymmetry between the CLI and the Dockerfile.


Reply to this email directly or view it on GitHubhttps://github.com/moby/moby/pull/1124#issuecomment-22824254
.

Thanks,


Michael Crosby
812-250-6603
crosby.michael@gmail.com

@matthewmueller

This comment has been minimized.

Show comment
Hide comment
@matthewmueller

matthewmueller Aug 18, 2013

Contributor

Oh good point, so it seems like VOLUME is more to serve volumes-from then because an empty volume inside a container isn't very useful

Contributor

matthewmueller commented Aug 18, 2013

Oh good point, so it seems like VOLUME is more to serve volumes-from then because an empty volume inside a container isn't very useful

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