Skip to content
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

adding VOLUME command #62

Merged
merged 16 commits into from
Apr 6, 2018
Merged

adding VOLUME command #62

merged 16 commits into from
Apr 6, 2018

Conversation

sharifelgamal
Copy link
Contributor

No description provided.

@@ -0,0 +1,12 @@
[
{
"Image1": "gcr.io/sharif-test/docker-test-volume:latest",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: kbuild-test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -54,6 +54,8 @@ func GetCommand(cmd instructions.Command, buildcontext string) (DockerCommand, e
return &LabelCommand{cmd: c}, nil
case *instructions.UserCommand:
return &UserCommand{cmd: c}, nil
case *instructions.VolumeCommand:
return &VolumeCommand{cmd: c, buildcontext: buildcontext}, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the buildcontext used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not, I'm getting rid of it.

// Create any directories that don't already exist
destVolume := volume
logrus.Infof("Creating directory %s", destVolume)
if err := os.MkdirAll(destVolume, 0755); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I think you actually don't have to create the volume if it doesn't exist, it looks like you just have to add it to the config and then docker mounts it during "docker run". That might explain why container-diff isn't finding certain directories in the docker built version of the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that makes my life easy. Will fix.

@priyawadhwa
Copy link
Collaborator

So the reference says that any changes to a VOLUME mounted directory after the VOLUME command should be ignored. Maybe we could add volume mounted directories to the whitelist so that any changes within those directories are ignored?

@sharifelgamal
Copy link
Contributor Author

I'm now adding volume mounted directories to our whitelist, but this causes RUN commands to fail rather than ignore them. Is that intended?

@priyawadhwa
Copy link
Collaborator

Oh yah I think RUN commands should still be able to execute, we just shouldn't snapshot any changes made in volume mounted directories. If we create the directory but also add it to the whitelist this should work

@sharifelgamal
Copy link
Contributor Author

Yep, verified that that's how it works and added a test for it.

Copy link
Collaborator

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

lgtm!

@sharifelgamal sharifelgamal merged commit ce2b515 into GoogleContainerTools:master Apr 6, 2018
@sharifelgamal sharifelgamal deleted the volume-cmd branch April 6, 2018 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants