Skip to content

Conversation

rycus86
Copy link
Contributor

@rycus86 rycus86 commented Feb 20, 2018

This PR would add a new shorthand method to restart a service, similarly to the recently added scale method.

Usage:

service = client.services.create(..)
service.restart()

Thanks!

@rycus86
Copy link
Contributor Author

rycus86 commented Feb 20, 2018

I think I've got some unrelated test failures on Jenkins:

22:45:48 [py3.6_17.12.0-ce] tests/integration/api_swarm_test.py::SwarmTest::test_init_swarm_with_ca_config FAILED

@shin-
Copy link
Contributor

shin- commented Feb 20, 2018

Yeah, sadly that test is hit or miss on 17.06 - don't worry about it.

Copy link
Contributor

@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.

Code LGTM, but not sure about the method's name.

service_mode,
fetch_current_spec=True)

def restart(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think restart doesn't accurately represent the operation that's being performed here. In the Docker ecosystem, restart usually means "take the existing containers with their configuration and stop them then start them again". The same idea exists in other common software like nginx with the reload vs restart distinction.

To my understanding. in this case, the containers for the service will actually be recreated with the same configuration - maybe force_update or reload is a more accurate name?

@rycus86
Copy link
Contributor Author

rycus86 commented Feb 21, 2018

Thanks again, @shin- !
I have renamed the method to force_update and changed the docstring to be the same (or very similar to) what the engine uses as help strings.

I've also snuck in another change: the force_update parameter on the service model's update method now accepts a bool, and if it is True then it does the counter increment logic. I thought it makes more sense to have it there, so you can just say service.update(force_update=True, **other_kwargs) and it's more inline with the docker service update --force flag being a boolean flag.

It would be great if you could give the change another look because of these extras. Thank you!

Copy link
Contributor

@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.

LGTM, thank you!

@shin-
Copy link
Contributor

shin- commented Feb 21, 2018

If you could just squash those commits into one, I can go ahead and merge it.

Signed-off-by: Viktor Adam <rycus86@gmail.com>
@rycus86 rycus86 force-pushed the service-restart-method branch from d6079f5 to e54e8f4 Compare February 21, 2018 19:55
@rycus86
Copy link
Contributor Author

rycus86 commented Feb 21, 2018

Sure, it's done now, thanks a lot!

@shin- shin- added this to the 3.1.0 milestone Feb 21, 2018
@shin- shin- merged commit ad5f49b into docker:master Feb 21, 2018
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.

2 participants