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 to specify credentials for S3 explicitly #2210

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Saruniks
Copy link
Contributor

@Saruniks Saruniks commented Jun 20, 2024

Closes #2164

No breaking changes, because if environment variables (SCCACHE_AWS_ACCESS_KEY_ID, SCCACHE_AWS_SECRET_ACCESS_KEY) or file configuration for credentials are not specified, sccache will try to load credentials from environment (including AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY)

src/cache/s3.rs Outdated
builder.access_key_id(access_key_id);
}

if let Some(secret_access_key) = secret_access_key {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should load password from env only with the login for preventing usage password from another account

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 sure that I understood what you meant.
You mean we should have checks to bail if only one of these are specified? None of them or both should be specified in order to work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can do it in this case.

@Saruniks Saruniks force-pushed the explicit-credentials-for-s3-configuration branch from d5f4f48 to fae1fa3 Compare July 17, 2024 14:27
@Saruniks Saruniks marked this pull request as draft July 17, 2024 14:50
@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 40.86%. Comparing base (0cc0c62) to head (0923a45).
Report is 66 commits behind head on main.

Files Patch % Lines
src/config.rs 51.72% 2 Missing and 12 partials ⚠️
src/cache/s3.rs 40.00% 3 Missing ⚠️
src/cache/cache.rs 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2210      +/-   ##
==========================================
+ Coverage   30.91%   40.86%   +9.94%     
==========================================
  Files          53       55       +2     
  Lines       20112    20707     +595     
  Branches     9755     9827      +72     
==========================================
+ Hits         6217     8461    +2244     
- Misses       7922     8102     +180     
+ Partials     5973     4144    -1829     

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

@Saruniks Saruniks marked this pull request as ready for review July 17, 2024 18:13
@sylvestre sylvestre force-pushed the explicit-credentials-for-s3-configuration branch from 987eb66 to 0923a45 Compare July 18, 2024 14:33
@Saruniks Saruniks force-pushed the explicit-credentials-for-s3-configuration branch from 0923a45 to 987eb66 Compare August 3, 2024 11:12
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.

Rename request for AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY
3 participants