Skip to content

[cli] run --rm overrides restart: always - #1205

Merged
aanand merged 2 commits into
docker:masterfrom
josephpage:run-rm-restart
Apr 21, 2015
Merged

[cli] run --rm overrides restart: always#1205
aanand merged 2 commits into
docker:masterfrom
josephpage:run-rm-restart

Conversation

@josephpage

Copy link
Copy Markdown

Fix#1013

I think docs is not needed because it's an intuitive behavior.

@josephpage

Copy link
Copy Markdown
Author

All tests pass on local, why they failed with Jenkins ?

@aanand

Copy link
Copy Markdown

Your commit needs signing off:

$ git commit --amend --reuse-message=HEAD --signoff
$ git push --force

@aanand

Copy link
Copy Markdown

Also, could we get an integration test please? Thanks!

#1013
Signed-off-by: Joseph Page <joseph.page@rednet.io>
@josephpage

Copy link
Copy Markdown
Author

@aanand Because the container is created and immediately removed by docker-compose run --rm command, it's difficult to test that the container doesn't have a restart policy.
Note that the --rm option is not tested today.

What do you think about it ?

@patch('dockerpty.start')deftest_run_service_with_restart_always(self, __):
name='simple'self.command.base_dir='tests/fixtures/restart-composefile'self.command.dispatch(['run', name, '/bin/echo', 'helloworld'], None)
service=self.project.get_service(name)
container=service.containers(one_off=True)[0]
self.assertEqual(container.get('HostConfig.RestartPolicy.Name'), 'always')
self.command.dispatch(['run', '--rm', name, '/bin/echo', 'helloworld'], None)
self.assertEqual(len(service.containers()), 0)
self.assertEqual(len(service.containers(stopped=True)), 0)

with this tests/fixtures/restart-composefile/docker-compose.yml :

simple:
image: busybox:latestrestart: always

@aanand

Copy link
Copy Markdown

Hm, good point. In that case, maybe just a unit test - use mock_client to check that create_container() is called with restart=None.

Signed-off-by: Joseph Page <joseph.page@rednet.io>
@josephpage

Copy link
Copy Markdown
Author

Just added a unit test. Seems good to you @aanand ?

@jaimegildesagredo

Copy link
Copy Markdown

@aanand some updates here? we would need this fix merged. If I can be of some help just ping me.

@dnephin

Copy link
Copy Markdown

LGTM

@aanand

Copy link
Copy Markdown

OK, looks like the CLI code is too tightly coupled to improve that unit test without a lot of refactoring.

LGTM

aanand added a commit that referenced this pull request Apr 21, 2015
[cli] run --rm overrides restart: always
@aanand
aanand merged commit b317071 into docker:masterApr 21, 2015
@aanandaanand added this to the 1.3.0 milestone Apr 21, 2015
@aanandaanand mentioned this pull request Apr 22, 2015
@josephpage
josephpage deleted the run-rm-restart branch May 30, 2015 18:20
Sign up for freeto 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.

docker-compose run command continuously restarting

4 participants

@josephpage@aanand@jaimegildesagredo@dnephin