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

feat(storage): Add generic support for content-types #115

Merged
merged 1 commit into from Apr 27, 2021

Conversation

jpetazzo
Copy link
Contributor

When serving a manifest, it is important to set the content-type
correctly (otherwise pulling an image is likely to give a cryptic
error message, "Error response from daemon: missing signature key").

This makes sure that we set the content-type properly for both
manifests and layers.

Let me know if that code should rather be moved to filesystem.go.

@tazjin
Copy link
Owner

tazjin commented Apr 14, 2021

Thanks! I think I might prefer having this in filesystem.go (e.g. changing the Serve function to also accept blobType or something like that). In the GCS backend this is handled by GCS itself, which is why the content-type is passed to the Persist function - there's a TODO about it in filesystem.go also.

@tazjin tazjin added the enhancement New feature or request label Apr 14, 2021
@jpetazzo
Copy link
Contributor Author

jpetazzo commented Apr 14, 2021 via email

@jpetazzo
Copy link
Contributor Author

OK, I suggested another implementation, storing the content-type in a separate file (with a .type extension).

I'm not super excited about it, but I'll let you judge if that's better :)

Another possibility would be to use extended attributes to store the content-type; but I'm worried that it might not work on all filesystems.

@tazjin
Copy link
Owner

tazjin commented Apr 27, 2021

I think I favour this change, but I'm unsure how this interacts with the redirects. Passing to the gcs implementation will write a redirect to the response writer. I'll test this out and then probably merge this PR (to get the issue fixed) and see if we can find a cleaner fix later.

@tazjin
Copy link
Owner

tazjin commented Apr 27, 2021

I rebased the PR on master, ignore the merge commit - that was me thinking Github would do the right thing, which it didn't, I've removed that again :)

When serving a manifest, it is important to set the content-type
correctly (otherwise pulling an image is likely to give a cryptic
error message, "Error response from daemon: missing signature key").

This makes sure that we set the content-type properly for both
manifests and layers.
@tazjin tazjin merged commit 7db252f into tazjin:master Apr 27, 2021
@jpetazzo jpetazzo deleted the set-content-types branch April 27, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants