Skip to content

Set Content-Type when uploading certain Zarr entries#1576

Merged
yarikoptic merged 2 commits intomasterfrom
content-type
Feb 13, 2025
Merged

Set Content-Type when uploading certain Zarr entries#1576
yarikoptic merged 2 commits intomasterfrom
content-type

Conversation

@jwodder
Copy link
Contributor

@jwodder jwodder commented Feb 12, 2025

No description provided.

@jwodder jwodder added minor Increment the minor version when merged blocked Blocked by some needed development/fix cmd-upload zarr labels Feb 12, 2025
@jwodder jwodder changed the title Set Content-Type when upload Zarr entries Set Content-Type when uploading Zarr entries Feb 12, 2025
@codecov
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.66%. Comparing base (45ac3d2) to head (b502a48).
Report is 87 commits behind head on master.

Files with missing lines Patch % Lines
dandi/files/zarr.py 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1576      +/-   ##
==========================================
+ Coverage   88.53%   88.66%   +0.13%     
==========================================
  Files          78       78              
  Lines       10829    10855      +26     
==========================================
+ Hits         9587     9625      +38     
+ Misses       1242     1230      -12     
Flag Coverage Δ
unittests 88.66% <92.85%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yarikoptic
Copy link
Member

given the discussion in dandi-archive PR, could you please just try uploading some zarr to dandi-staging e.g. to https://gui-staging.dandiarchive.org/dandiset/214256 and see if resultant downloads have correct content encoding and rendered by browser if you visit those urls

@jwodder
Copy link
Contributor Author

jwodder commented Feb 13, 2025

@yarikoptic I uploaded a Zarr to that Dandiset, with Content-Type set in the PUT request, and downloading its .zarray file afterwards via curl -fisSL 'https://api-staging.dandiarchive.org/api/zarr/8c9d4d53-4084-4fc5-9720-0c809a5903ef/files/?prefix=.zarray&download=true' shows "Content-Type: application/json".

rendered by browser if you visit those urls

What URL? If you mean via dandidav, it doesn't cover staging.

@yarikoptic
Copy link
Member

pretty much this URL on S3 for the asset you tried: https://dandi-api-staging-dandisets.s3.amazonaws.com/zarr/8c9d4d53-4084-4fc5-9720-0c809a5903ef/.zarray and it is all good! It is different from e.g. https://dandi-api-staging-dandisets.s3.amazonaws.com/zarr/1421096f-3b20-4027-a800-c3fee29b130a/.zarray for a zarr uploaded a year ago and thus without content-types -- browser just offers to download it. So I think we are all good as -- it works!

@jwodder jwodder changed the title Set Content-Type when uploading Zarr entries Set Content-Type when uploading certain Zarr entries Feb 13, 2025
@jwodder jwodder removed the blocked Blocked by some needed development/fix label Feb 13, 2025
@jwodder jwodder marked this pull request as ready for review February 13, 2025 16:03
@jwodder jwodder requested a review from yarikoptic February 13, 2025 16:03
@yarikoptic
Copy link
Member

Looks great, thank you @jwodder, let's proceed

@yarikoptic yarikoptic merged commit dec6cb7 into master Feb 13, 2025
26 checks passed
@yarikoptic yarikoptic deleted the content-type branch February 13, 2025 19:29
@github-actions
Copy link

github-actions bot commented Mar 3, 2025

🚀 PR was released in 0.67.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd-upload minor Increment the minor version when merged released zarr

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants