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

feat: allow to use a non-root S3 bucket location #30

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

satlank
Copy link
Contributor

@satlank satlank commented Sep 23, 2024

This allows to direct cratery to use a 'subdirectory' to store its data on S3. This is useful to not have to dedicate a bucket to cratery (which may or may not be appropriate). E.g. instead of using my-cratery-bucket/ we can now use my-multipurpose-bucket/cratery/.

@satlank
Copy link
Contributor Author

satlank commented Sep 23, 2024

Thanks for this project! While evaluating it, came across a few things that would make life easier for me, so here is the first one :)

* `REGISTRY_S3_ACCESS_KEY`: The access key to use
* `REGISTRY_S3_SECRET_KEY`: The secret key to use
* `REGISTRY_S3_ACCESS_KEY`: The access key to use, set to empty string to search for existing credentials.
* `REGISTRY_S3_SECRET_KEY`: The secret key to use, set to empty string to search for existing credentials.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely related, but took a while to figure it out (I want to let OpenDAL deal with sorting out the credentials instead).

@@ -110,6 +110,7 @@ impl StorageConfig {
region: get_var("REGISTRY_S3_REGION")?,
access_key: get_var("REGISTRY_S3_ACCESS_KEY")?,
secret_key: get_var("REGISTRY_S3_SECRET_KEY")?,
root: get_var("REGISTRY_S3_ROOT").unwrap_or_default(),
Copy link
Contributor Author

@satlank satlank Sep 23, 2024

Choose a reason for hiding this comment

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

The unwrap_or_default is here to make setting this env variable optional, but could also require it and as for the keys an empty string would be 'do the default thing of going to the top-level'.

Copy link
Member

@woutersl woutersl left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for you for your contribution. I'm integrating this now.

@woutersl woutersl merged commit fb0ebd6 into cenotelie:main Sep 23, 2024
2 checks passed
@satlank satlank deleted the awsRoot branch September 23, 2024 16:04
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.

2 participants