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

Remove anonymous volumes when using run --rm. #3756

Merged
merged 1 commit into from Mar 2, 2017

Conversation

nkovacs
Copy link

@nkovacs nkovacs commented Jul 22, 2016

Named volumes will not be removed.
This is consistent with the behavior of docker run --rm.

Fixes #2419, #3611

@aanand
Copy link

aanand commented Jul 22, 2016

Thanks! LGTM

ping @dnephin

if volumes is None:
volumesBefore = 0
else:
volumesBefore = len(volumes)
Copy link

Choose a reason for hiding this comment

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

I don't think this is the right way to test this. It's too prone to failure. It assumes the engine being used is not in use by anything else, which is not a good assumption. The test could also pass if no volume was created, even though the remove didn't work.

A better way to test this would be to inspect the container that is created, get the volume id, and assert that the volume doesn't exist when the container exits. It's a little more involved to write, but you can do it with start_process() and wait_on_process().

You'll need to run a non-terminating command instead of /bin/true.

start_process(...)
wait_on_condition(ContainerCountCondition(self.project, 1))
container, = self.project.get_service('test').containers(...)
self.dispatch(["stop"])

# inspect volume by id and assert a not found error

@nkovacs
Copy link
Author

nkovacs commented Jul 22, 2016

Done. Do you want me to squash the commits?

@bfirsh
Copy link

bfirsh commented Jul 25, 2016

@nkovacs yes please!

Named volumes will not be removed.
This is consistent with the behavior of docker run --rm.

Fixes docker#2419, docker#3611

Signed-off-by: Nikola Kovacs <nikola.kovacs@gmail.com>
@klausseiler
Copy link

Hi,
I have one question, when will this fix be added to the docker:master branch?
It's now quite a while ago when @nkovacs fixed that.

Copy link

@shin- shin- left a comment

Choose a reason for hiding this comment

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

Thank you!

@shin- shin- added this to the 1.12.0 milestone Mar 2, 2017
@shin- shin- merged commit 1a77a7f into docker:master Mar 2, 2017
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.

None yet

7 participants