Skip to content

Multipart upload support #34

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

Merged
merged 6 commits into from
May 21, 2025

Conversation

mesemus
Copy link
Contributor

@mesemus mesemus commented Jan 19, 2025

Description

This pull request provides an implementation of multipart upload from RFC 0072.
The new methods are used by code changes in invenio-records-resources PR

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Frontend

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

@mesemus
Copy link
Contributor Author

mesemus commented Jan 19, 2025

mesemus and others added 2 commits April 8, 2025 07:56
* Implementation of multipart upload, as described in RFC 0072

* See inveniosoftware/rfcs#91

Co-authored-by: Mirek Simek <[email protected]>
@mesemus mesemus force-pushed the contribution-multipart-upload branch from cee8f40 to 3b4ba12 Compare April 8, 2025 05:58
@mesemus mesemus marked this pull request as ready for review April 8, 2025 06:00
@mesemus
Copy link
Contributor Author

mesemus commented Apr 8, 2025

Question: The warnings at ext.py: typos in S3_ACCCESS_KEY_ID and S3_SECRECT_ACCESS_KEY have been in the codebase for two years, would it be ok to remove those?

@tmorrell
Copy link
Contributor

tmorrell commented Apr 8, 2025

Those typos corrections have been there for more like 6 years. I think it's high time to get rid of the warnings.

Copy link
Contributor

@tmorrell tmorrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! These look like a sensible improvement to the package.

@tmorrell tmorrell added this to v13.0.0 Apr 11, 2025
@tmorrell tmorrell moved this to 👀 In review in v13.0.0 Apr 11, 2025
Copy link
Contributor

@wgresshoff wgresshoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This look great to me.

Rationale: the API and dependencies of s3fs have changed in
versions >0.5.0. We will need to upgrade eventually, but I'd
prefer doing it in a dedicated PR.

The problem surfaced with `uv` as it uses different
version conflict resolution.
mesemus added 3 commits May 20, 2025 18:24
* due to changes in the creation of url it is necessary to set THEME_FRONTPAGE explicitly
@tmorrell
Copy link
Contributor

All comments have been addressed. I'm planning on merging and releasing this tomorrow, unless someone yells.

@tmorrell tmorrell merged commit 0458911 into inveniosoftware:master May 21, 2025
2 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to To release 🤖 in v13.0.0 May 21, 2025
@tmorrell tmorrell moved this from To release 🤖 to Released ✔ in v13.0.0 Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released ✔
Development

Successfully merging this pull request may close these issues.

5 participants