Skip to content

Conversation

allencloud
Copy link
Contributor

Since PR in moby/moby moby/moby#32122 hopes to get status code from rpc code which is determined in swarmkit.

So, when we made this rule, we found that some status code from moby/moby needs changed.

While in integration test of moby/moby, we will use docker-py to test daemon's API. It always fails in this status code. So I try to make this change.

ping @shin- @thaJeztah @stevvooe

Signed-off-by: allencloud allen.sun@daocloud.io

@shin-
Copy link
Contributor

shin- commented May 12, 2017

We should test both 500 and 400 so the test still validates pre-existing releases.

@stevvooe
Copy link

How about status_code >= 400? That will ensure that it is resilient to changes.

@shin-
Copy link
Contributor

shin- commented May 13, 2017

The ideal situation from an API consumer standpoint would be for status code changes to happen as rarely as possible, and to not affect existing API versions.

I understand why that's not necessarily possible, and I know from our previous discussions that you do not want anyone relying on status codes to detect and identify errors, but at this time, and until proper error codes become an integral part of the engine API, this is the only indicator users have. If that type of test can be a small reminder of that, and the possible consequences of that type of change in the real world, I don't necessarily see it as a bad thing.

Signed-off-by: allencloud <allen.sun@daocloud.io>
@allencloud allencloud force-pushed the update-test-status-code-from-500-to-400 branch from 50f6179 to 717459d Compare May 13, 2017 01:31
@allencloud
Copy link
Contributor Author

@shin- @stevvooe
Thanks for your feedback. Currently I tried to modify the comparison to be >=400 to take into legacy api into consideration.

And Yeah, actually I understand the consequence of changing status code, while I think we still need to pay more attention on locking the APIs and be very careful.

@shin- shin- merged commit 2aa63dd into docker:master May 15, 2017
@allencloud allencloud deleted the update-test-status-code-from-500-to-400 branch May 16, 2017 12:04
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