Make SCCACHE_S3_NO_CREDENTIALS require a value of true
#1724
Conversation
src/config.rs
Outdated
| @@ -506,7 +506,10 @@ fn config_from_env() -> Result<EnvConfig> { | |||
| // ======= AWS ======= | |||
| let s3 = env::var("SCCACHE_BUCKET").ok().map(|bucket| { | |||
| let region = env::var("SCCACHE_REGION").ok(); | |||
| let no_credentials = env::var("SCCACHE_S3_NO_CREDENTIALS").ok().is_some(); | |||
| let no_credentials = env::var("SCCACHE_S3_NO_CREDENTIALS") | |||
There was a problem hiding this comment.
I think we should be more aggressive here.
if SCCACHE_S3_NO_CREDENTIALS is set, it can be only true or false (or 1 or 0).
Otherwise, we don't start the program.
There was a problem hiding this comment.
@sylvestre, I just pushed some changes. I'm not great with rust. Can you tell me if my approach is right? It took me a while to figure out the Option / Result handling.
I'll update the tests in a follow-up commit.
| @@ -1066,7 +1069,7 @@ fn config_overrides() { | |||
| #[test] | |||
| #[serial] | |||
| fn test_s3_no_credentials() { | |||
| env::set_var("SCCACHE_S3_NO_CREDENTIALS", "1"); | |||
| env::set_var("SCCACHE_S3_NO_CREDENTIALS", "true"); | |||
There was a problem hiding this comment.
we would need more (and separate) tests to cover this
There was a problem hiding this comment.
Updated in the latest commits. I mostly just copy and pasted the existing test functions.
Let me know if there's a better convention than that.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1724 +/- ##
==========================================
+ Coverage 29.14% 29.26% +0.12%
==========================================
Files 49 49
Lines 17456 17496 +40
Branches 8471 8481 +10
==========================================
+ Hits 5087 5120 +33
+ Misses 7297 7291 -6
- Partials 5072 5085 +13
... and 8 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
@sylvestre, just checking in. any thoughts on this PR so far? |
ajschmidt8
force-pushed
the
no-credentials
branch
from
841c356
to
3337715
Compare
ajschmidt8
force-pushed
the
no-credentials
branch
from
3337715
to
6352388
Compare
Closes #1723.
This PR updates the
SCCACHE_S3_NO_CREDENTIALSenvironment variable to require a value oftruein order to enable this feature.Note: This is a breaking change and should probably be called out loudly in any future release notes.