-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
update status code for network api #30699
update status code for network api #30699
Conversation
CI fails in janky:
I am afraid change I made takes affect in docker-py, but I have no idea what to do in docker-py. |
ping @shin- |
Looks like a random naming conflict - try kicking up the build again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
verified with docker 1.9 for the older API versions |
The 500 status code is not correct - the request is invalid (multiple IDs match the short ID), not the server having a problem, it should be a 400 error. So we should not document it, we should fix. I recommend http://racksburg.com/choosing-an-http-status-code/ |
Thanks for the double check @thaJeztah And thanks for your feedback, @justincormack . What you mentioned is reasonable, I think. At first, I think we should fix this without doubt. While maybe this is a two-phase work.
WDYT? @justincormack |
I think now pr #30453 added code https://github.com/docker/docker/blob/master/api/server/router/network/network_routes.go#L147 and https://github.com/docker/docker/blob/master/api/server/router/network/network_routes.go#L157 in docker/master. And this pr introduced a kind of error which will be returned a status code of 500. I think it seems OK for master to add 500. While for the legacy, any input? @justincormack @thaJeztah |
c4bf6f8
to
5c304f5
Compare
ping @justincormack @thaJeztah |
f6df244
to
af6c99a
Compare
ping @justincormack @thaJeztah Yeah, I totally agree on that While for the old versions of Docker, they still lack the more precise status code In master, I think there some places which return 500 now, like:
and
So, adding a 500 seems OK, but for 401 bad request, I think we need a follow-up. We should consider more about that, because |
af6c99a
to
a98046e
Compare
ping @thaJeztah @justincormack |
a98046e
to
439c677
Compare
Could we move on this pr currently, my friends? Any feedback helps. 😃 |
439c677
to
235c404
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: allencloud <allen.sun@daocloud.io>
235c404
to
56dca8b
Compare
Seems, z and powerpc, there is always one who is naughty to pick errors 😄 |
Signed-off-by: allencloud allen.sun@daocloud.io
fixes #30698
- What I did
GET /networks/(id or name)
, status code 500 is from https://github.com/docker/docker/blob/master/daemon/network.go#L61;DELETE /networks/(id or name)
, status code is from https://github.com/docker/docker/blob/master/daemon/network.go#L454;id
withid or name
.- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)