search

transparency-dev / trillian-tessera

Go library for building tile-based transparency logs (tlogs)
Apache License 2.0
8 stars 7 forks source link

Explicitly Set content types on tiles and leaves #136

Closed phbnf closed 4 weeks ago

phbnf commented 1 month ago

If not specified, GCS automatically detects the content type, so let's be explicit and avoid unfortunate divergence outside of our control.

I haven't added tests, but since there was some divergence before https://github.com/C2SP/C2SP/pull/92, it proves that it can happen, and doesn't sound crazy to add some. I can do this in a later PR but we'd also need to modify the getObject signature, which sounds a bit cumbersome.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 35.94%. Comparing base (46ec9c2) to head (14e6273). Report is 60 commits behind head on main.

Files Patch % Lines
storage/gcp/gcp.go 20.00% 3 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #136 +/- ## ========================================== + Coverage 35.80% 35.94% +0.13% ========================================== Files 16 33 +17 Lines 1363 2824 +1461 ========================================== + Hits 488 1015 +527 - Misses 801 1698 +897 - Partials 74 111 +37 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

AlCutter commented 1 month ago

As an aside, I'm not sure what application/data is - it's not one of the IANA registered types: https://www.iana.org/assignments/media-types/media-types.xhtml#application

I'm checking with Filippo what he intended there, but I've speculatively raised C2SP/C2SP#92 against the specs to change to application/octet-stream which is a registered type.

I agree that being explicit here is >> than relying on auto-detection which may change at any time, though!

Do you want to leave this PR open for the moment until we've clarified the type?

phbnf commented 1 month ago

Ah you beat me to it I was about to speak with Martin and raise and issue after lunch if need be. My plan was to submit this to align with specs, and to change this later if the specs change.

phbnf commented 4 weeks ago

I've updated the PR title, but I'll leave the description as it is to keep history and context.

AlCutter commented 4 weeks ago

I think it'd be better to update the description to match what the PR does now - this is going to be what's associated with the squashed commit when it's merged so better to have it describe the "atomic whole" rather than a point-in-time history. The evolution is available for archeology via this PR if anyone needs the context.