search

tazjin / nixery

Container registry which transparently builds images using the Nix package manager. Canonical repository is https://cs.tvl.fyi/depot/-/tree/tools/nixery
https://nixery.dev/
Apache License 2.0
1.82k stars 68 forks source link

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

Closed jpetazzo closed 3 years ago

jpetazzo commented 3 years ago

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 commented 3 years ago

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.

jpetazzo commented 3 years ago

Thanks for the feedback! I'll take a stab at it hopefully next week. :)

On Wed, Apr 14, 2021 at 2:23 PM Vincent Ambo @.***> wrote:

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.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/nixery/pull/115#issuecomment-819476469, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABJ3WMNAEAVZKPNB22HO2LTIWCMTANCNFSM423OYIJQ .

-- @jpetazzo https://twitter.com/jpetazzo http://jpetazzo.github.io/

jpetazzo commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 :)