-
Notifications
You must be signed in to change notification settings - Fork 47
[PULP-255] Remove backward compatibility for manifest with artifact #1890
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
Conversation
2b3b472 to
daf186d
Compare
pulp_container/app/registry_api.py
Outdated
| return redirects.redirect_to_content_app("manifests", pk) | ||
| elif manifest: | ||
| return redirects.issue_manifest_redirect(manifest) | ||
| return Response(manifest.data) |
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.
TODO: #1644 (comment)
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.
@git-hyagi @gerrod3 Any idea which headers specifically he was referring to?
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.
I was checking the OCI spec and found this:
"In a successful response, the Content-Type header will indicate the type of the returned manifest. The Content-Type header SHOULD match what the client pushed as the manifest's Content-Type. If the manifest has a mediaType field, clients SHOULD reject unless the mediaType field's value matches the type specified by the Content-Type header."
"A successful response SHOULD contain the digest of the uploaded blob in the header Docker-Content-Digest."
and searching the sample (return web.Response(...)) provided by Lubos, I found these headers definitions (which comply with the oci spec):
pulp_container/pulp_container/app/registry.py
Lines 137 to 142 in 18dab98
| headers = { | |
| "Content-Type": media_type, | |
| "Docker-Content-Digest": digest, | |
| "Docker-Distribution-API-Version": "registry/2.0", | |
| } | |
| return web.Response(text=raw_text_manifest, headers=headers) |
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.
I'm going to treat removing this redirect as a separate issue.
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.
I've assigned myself to that work. But this PR should probably go ahead and proceed as-is, w/ passing CI.
969fc97 to
5020597
Compare
469bc7d to
1bd73e3
Compare
b917450 to
b34991a
Compare
b34991a to
5978ceb
Compare
5978ceb to
f8dc033
Compare
fbe316c to
230ee44
Compare
c008485 to
72f15ec
Compare
3ec2422 to
72f15ec
Compare
gerrod3
left a comment
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.
Two small fixes then lgtm!
72f15ec to
d59fcd0
Compare
This commit removes support for manifests storing their data inside an artifact instead of using the recently introduced Manifest.data text field. closes pulp#1621
d59fcd0 to
c8d72c7
Compare
This commit removes support for manifests storing their data inside an artifact instead of using the recently introduced Manifest.data text field.
closes #1621