Skip to content

Open up v2 http status code checks for put and head checks#10403

Merged
tiborvass merged 1 commit intomoby:masterfrom
stevvooe:v2-status-code-narrow
Jan 28, 2015
Merged

Open up v2 http status code checks for put and head checks#10403
tiborvass merged 1 commit intomoby:masterfrom
stevvooe:v2-status-code-narrow

Conversation

@stevvooe
Copy link
Contributor

Under certain cases, such as when putting a manifest or check for the existence
of a layer, the status code checks in session_v2.go were too narrow for their
purpose. In the case of putting a manifest, the handler only cares that an
error is not returned. Whether it is a 304 or 202 does not matter, as long as
the server reports success. Having the client only accept specific http codes
inhibits future protocol evolution.

Signed-off-by: Stephen J Day [email protected]

Closes #10402.

Under certain cases, such as when putting a manifest or check for the existence
of a layer, the status code checks in session_v2.go were too narrow for their
purpose. In the case of putting a manifest, the handler only cares that an
error is not returned. Whether it is a 304 or 202 does not matter, as long as
the server reports success. Having the client only accept specific http codes
inhibits future protocol evolution.

Signed-off-by: Stephen J Day <[email protected]>
@stevvooe
Copy link
Contributor Author

cc @icecrime @jfrazelle

@icecrime icecrime added this to the 1.5.0 milestone Jan 28, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

should be just two ifs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch implies coverage over an input domain, which is what we want here. Arguably, default return should be pulled up into switch for clarity.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're not covering input domain of http status codes here and this looks just weird.

@icecrime
Copy link
Contributor

This sounds reasonable to me, and has very low impact. I'm not going to take position in the style comments, so LGTM!

@icecrime
Copy link
Contributor

@LK4D4 Do we have your LGTM or do you feel your comments are blockers?

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 28, 2015

LGTM
Though I'm not best person to ask :)

@tiborvass
Copy link
Contributor

LGTM

tiborvass added a commit that referenced this pull request Jan 28, 2015
Open up v2 http status code checks for put and head checks
@tiborvass tiborvass merged commit c575cc6 into moby:master Jan 28, 2015
@stevvooe stevvooe deleted the v2-status-code-narrow branch January 28, 2015 19:09
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.

http status code checks in registry/session_v2.go too narrow

4 participants