Skip to content

Conversation

@hannseman
Copy link
Contributor

@hannseman hannseman commented Mar 13, 2019

The response contains a Warnings key that should be returned so it can be handled by the caller.

See the 200 Response documentation here: https://docs.docker.com/engine/api/v1.39/#operation/ServiceUpdate

Currently the only return value from this method is True. It can never return False (it will raise instead).

With this change checks like this on the return value will still work:

response = client.update_service(...)
if response:
  ...

But it would break backwards compatability if used like this:

response = client.update_service(...)
if response is True:
  ...

As previously stated both these checks does not make sense as the only possible return value is True.

Copy link
Contributor

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

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

LGTM

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! We can ship the change with 4.0, and the API change won't be an issue in that case.

@shin- shin- added this to the 4.0.0 milestone Mar 17, 2019
@shin- shin- merged commit 1047376 into docker:master Mar 17, 2019
@hannseman
Copy link
Contributor Author

We can ship the change with 4.0, and the API change won't be an issue in that case.

Sounds good!

@shin- and @ulyssessouza thanks for the review and merge.

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.

3 participants