Don't include x-amz-acl header in zarr upload

[jjnesbitt]

Closes #1709

This should fix zarr upload in the wake of dandi/dandi-infrastructure#245 being merged.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.82%. Comparing base (2827711) to head (ca7a84f).
⚠️ Report is 128 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1710   +/-   ##
=======================================
  Coverage   74.82%   74.82%           
=======================================
  Files          84       84           
  Lines       11693    11693           
=======================================
  Hits         8749     8749           
  Misses       2944     2944           

Flag	Coverage Δ
unittests	74.82% <100.00%> (ø)

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]

@kabilar could you please test this branch (zarr-upload-no-acl) on your case?

[yarikoptic]

looking into making it compatible with prior installations (e.g. EMBER)

note, I updated branch to get rid of typing check errors

[yarikoptic]

echoing discussion in

• Add tagging to zarr upload if embargoed #1685 (comment)

was that anywhere in

dandi/files/zarr.py-        signed_headers = ";".join(
dandi/files/zarr.py:            urllib.parse.parse_qs(upload_url).get("X-Amz-SignedHeaders", [])
dandi/files/zarr.py-        ).split(";")

so we could make it conditional and keep working for older deployments like EMBER with "fixed" client?

[kabilar]

Thanks, testing now.

[kabilar]

Upload has initiated. This has resolved the issue. Thank you.

[jjnesbitt]

looking into making it compatible with prior installations (e.g. EMBER)

I believe EMBER plans on merging the infra changes on their end fairly soon, although I could be wrong. cc @NEStock

If that's the case we could just coordinate with them, without much downtime.

[NEStock]

looking into making it compatible with prior installations (e.g. EMBER)

I believe EMBER plans on merging the infra changes on their end fairly soon, although I could be wrong. cc @NEStock

If that's the case we could just coordinate with them, without much downtime.

@jjnesbitt Yes, we are hoping to update EMBER fork's of dandi-archive & -infrastructure soon. Am I understanding correctly that if this MR is merged into dandi-cli before we update EMBER-DANDI to some commit of dandi-infrastructure, we risk incompatibility?
If so, can you link what version (commit) of dandi-infrastructure (and dandi-archive if applicable) is required?

[jjnesbitt]

@jjnesbitt Yes, we are hoping to update EMBER fork's of dandi-archive & -infrastructure soon. Am I understanding correctly that if this MR is merged into dandi-cli before we update EMBER-DANDI to some commit of dandi-infrastructure, we risk incompatibility?

Correct, although I don't think we'd merge this in an incompatible way. We'd either try to make this update in a backwards compatible way, or wait until the ember project is caught up, so we can keep the current implementation. My preference is the latter.

If so, can you link what version (commit) of dandi-infrastructure (and dandi-archive if applicable) is required?

For dandi-infrastructure, up to this commit is required.
For dandi-archive, up to this commit is required.

[NEStock]

Correct, although I don't think we'd merge this in an incompatible way. We'd either try to make this update in a backwards compatible way, or wait until the ember project is caught up, so we can keep the current implementation. My preference is the latter.

Sounds good, thank you! We are planning to do our next sync early next week - will coordinate further via Slack.

For dandi-infrastructure, up to this commit is required.
For dandi-archive, up to this commit is required.

Thanks!

[yarikoptic]

We'd either try to make this update in a backwards compatible way

do you see an easy way for this @jjnesbitt ?

[jjnesbitt]

We'd either try to make this update in a backwards compatible way

do you see an easy way for this @jjnesbitt ?

If the x-amz-acl header is visible in the pre-signed URL then it's possible, I'd have to investigate a bit, since the main DANDI instance no longer uses this.

[yarikoptic]

@NEStock -- did you have a chance to upgrade ?

[NEStock]

@NEStock -- did you have a chance to upgrade ?

Yes I believe we are updated as needed. @yarikoptic

• aplbrain/dandi-infrastructure is currently fully up to date with dandi-infrastructure
• aplbrain/dandi-archive is updated at least to the linked commit (but still a few commits behind dandi-archive)

[github-actions]

🚀 PR was released in 0.73.0 🚀