Skip to content
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

Add support for Cloudflare R2 Buckets #6701

Merged
merged 18 commits into from
Oct 16, 2023
Merged

Add support for Cloudflare R2 Buckets #6701

merged 18 commits into from
Oct 16, 2023

Conversation

JonasHiltl
Copy link
Contributor

@JonasHiltl JonasHiltl commented Aug 18, 2023

Motivation and context

It is currently not possible to add a Cloudflare R2 Bucket as a Cloud Storage source, since the Secret access key of R2 has a length of 64 but the ui/serializer restricts the length to 44.

This issue addresses the same problem when using STORJ but with a max length of 128.

For me it would make the most sense to completely remove the max length, but this PR currently only fixes the R2 compatibility by increasing the max length to 64.

Checklist

  • I submit my changes into the develop branch
  • I have added a description of my changes into the CHANGELOG file
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #6701 (0990f95) into develop (b450b44) will decrease coverage by 0.04%.
Report is 13 commits behind head on develop.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #6701      +/-   ##
===========================================
- Coverage    82.59%   82.55%   -0.04%     
===========================================
  Files          360      360              
  Lines        38926    38977      +51     
  Branches      3570     3577       +7     
===========================================
+ Hits         32150    32179      +29     
- Misses        6776     6798      +22     
Components Coverage Δ
cvat-ui 77.54% <100.00%> (-0.06%) ⬇️
cvat-server 87.16% <100.00%> (+<0.01%) ⬆️

Copy link
Contributor

@Marishka17 Marishka17 left a comment

Choose a reason for hiding this comment

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

I guess it's OK for now, but we probably need to remove the restriction for a secret key at all, because if I understand correctly AWS has no restriction for a Secret Access Key (only for access key id). And we support other providers (like Oracle OCI and Cloudflare R2) only because they have compatible API with AWS S3, so we have to rely on the AWS S3 limitations.

CHANGELOG.md Outdated Show resolved Hide resolved
cvat/apps/engine/models.py Outdated Show resolved Hide resolved
@nmanovic
Copy link
Contributor

@JonasHiltl , thank you for the great contribution. I'm ready to merge the PR. We have slightly changed how we are updating CHANGELOG. Could you please remove the line from CHANGELOG and use scriv create --edit command (please look at the latest version of CHANGELOG.md file)?

@JonasHiltl
Copy link
Contributor Author

@nmanovic Thanks for coming back to me, I updated the Changelog entry and I'm looking forward to seeing this get merged.

@nmanovic nmanovic merged commit 021e58c into cvat-ai:develop Oct 16, 2023
33 checks passed
@cvat-bot cvat-bot bot mentioned this pull request Oct 23, 2023
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Oct 25, 2023
It is currently not possible to add a Cloudflare R2 Bucket as a Cloud
Storage source, since the Secret access key of R2 has a length of 64 but
the ui/serializer restricts the length to 44.

[This issue](cvat-ai#5512) addresses the
same problem when using STORJ but with a max length of `128`.

For me it would make the most sense to completely remove the max length,
but this PR currently only fixes the R2 compatibility by increasing the
max length to `64`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants