Skip to content
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

Review status codes for 1.0 #68

Closed
dmcgowan opened this issue Aug 14, 2019 · 12 comments
Closed

Review status codes for 1.0 #68

dmcgowan opened this issue Aug 14, 2019 · 12 comments
Labels
clarification help wanted Extra attention is needed
Milestone

Comments

@dmcgowan
Copy link
Member

dmcgowan commented Aug 14, 2019

We need to go through and review all the returned status codes for the various endpoints. This includes what is defined in the overview, what is defined in the detail, and whether it tested and which status code the test uses.

The result of this should be removing the disclaimer about discrepancies between header and detail.

API endpoint Overview Detail Tests Status
GET /v2/ 200 200
GET /v2/<name>/tags/list 200
GET /v2/<name>/tags/list?n=<integer>&last=<integer> 200
GET /v2/<name>/manifests/<reference> 200
PUT /v2/<name>/manifests/<reference> 201
DELETE /v2/<name>/manifests/<reference> 202
GET /v2/<name>/blobs/<digest> 200
GET /v2/<name>/blobs/<digest> + Range 206
DELETE /v2/<name>/blobs/<digest> 202 202 Inconsistent wording (layer vs blob)
POST /v2/<name>/blobs/uploads/?digest=<digest> 201
POST /v2/<name>/blobs/uploads/ 202
POST /v2/<name>/blobs/uploads/?mount=<digest>&from=<repository name> 201
GET /v2/<name>/blobs/uploads/<uuid> 204 204
PATCH /v2/<name>/blobs/uploads/<uuid> 202 202
PUT /v2/<name>/blobs/uploads/<uuid>?digest=<digest> 201 204 Needs fix
DELETE /v2/<name>/blobs/uploads/<uuid> undefined 204 Needs fix
@dmcgowan dmcgowan added this to the v1.0.0-rc1 milestone Aug 14, 2019
@vbatts
Copy link
Member

vbatts commented Dec 16, 2019

cc @jdolitsky

@jdolitsky
Copy link
Member

@vbatts @dmcgowan as far as the endpoints go, is this a complete list?

@vbatts
Copy link
Member

vbatts commented Dec 19, 2019

looks like it.
I'd like to copy this table into the doc, and make each one a link to the anchor currently in the spec

@vbatts
Copy link
Member

vbatts commented Dec 20, 2019

@pmengelbert
Copy link
Contributor

@dmcgowan @vbatts the "PUT /v2//blobs/uploads/?digest=" endpoint -- should it be 201 or 204? It's ambiguous in the spec. Someone asked about this on the mailing list today, and the conformance tests aren't sure which one to look for.

Also, would it be possible to add the response codes to the table in the spec itself? It would be handy to have that all in one place (once the conflicts are resolved, of course)

@vbatts
Copy link
Member

vbatts commented Feb 1, 2020

@pmengelbert great question. I see that a 201 would be for new content created, and a 204 would be nothing new done. I'd have to compare which status code is already most commonly used, and whether it depends on the uploaded blob being new to the server or not (which honestly could be a silly side-channel)

@vbatts vbatts added clarification help wanted Extra attention is needed labels Apr 1, 2020
@jzelinskie
Copy link
Member

As discussed today on the call, it would be nice to use the conformance tests to see what existing registries return today such that we can use that as feedback when reviewing acceptable status codes for v1.0.0.

@jzelinskie
Copy link
Member

@jdolitsky

@jdolitsky
Copy link
Member

Initial list of real world results: https://docs.google.com/spreadsheets/d/1uwKBnUDoaoHICIN_NlIXJTyAsAFX9eqQGcD1oJqv72g/edit#gid=0

Summary:

  • POST /v2/<name>/blobs/uploads/?digest=<digest> should allow for 202
  • DELETE calls should allow 405's

Additionally, almost no registries return a 416 if chunked blob upload is out of order, but I think they probably should.

Thoughts?

@jdolitsky
Copy link
Member

Anybody opposed to the following changes to the spec? @opencontainers/distribution-spec-maintainers

  • POST /v2/<name>/blobs/uploads/?digest=<digest> should allow for 202 Accepted
  • All DELETE calls should allow 405 Method Not Allowed

If so, I think we can finalize this table for 1.0.0

@jdolitsky
Copy link
Member

Can we consider this closed?

@jdolitsky
Copy link
Member

Resolved by #206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants