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: Exit while cache is not configured correctly #1515

Merged
merged 9 commits into from Dec 30, 2022

Conversation

Xuanwo
Copy link
Collaborator

@Xuanwo Xuanwo commented Dec 29, 2022

Signed-off-by: Xuanwo github@xuanwo.io

This PR is the first step of #1512.

We will exit while the cache is not configured correctly. For example, s3 is configured with an invalid endpoint.

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2022

Codecov Report

Base: 30.15% // Head: 30.12% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (b10bc06) compared to base (66fff86).
Patch coverage: 27.94% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1515      +/-   ##
==========================================
- Coverage   30.15%   30.12%   -0.04%     
==========================================
  Files          47       47              
  Lines       16665    16643      -22     
  Branches     7918     7908      -10     
==========================================
- Hits         5026     5014      -12     
+ Misses       6326     6315      -11     
- Partials     5313     5314       +1     
Impacted Files Coverage Δ
src/cache/gcs.rs 12.24% <0.00%> (-0.53%) ⬇️
src/server.rs 32.45% <25.00%> (+0.25%) ⬆️
src/cache/cache.rs 38.65% <26.31%> (+2.22%) ⬆️
tests/sccache_cargo.rs 32.94% <33.33%> (+0.38%) ⬆️
src/config.rs 35.55% <38.46%> (-0.43%) ⬇️
src/dist/cache.rs 20.67% <0.00%> (-1.88%) ⬇️
src/mock_command.rs 51.31% <0.00%> (-1.13%) ⬇️
src/compiler/rust.rs 33.33% <0.00%> (-0.27%) ⬇️
... and 6 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Xuanwo <github@xuanwo.io>
tests/sccache_cargo.rs Outdated Show resolved Hide resolved
@Xuanwo Xuanwo marked this pull request as draft December 29, 2022 10:31
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo marked this pull request as ready for review December 29, 2022 10:37
@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Dec 29, 2022

Fixed! @sylvestre PTAL.

tests/sccache_cargo.rs Outdated Show resolved Hide resolved
Xuanwo and others added 2 commits December 29, 2022 18:45
Co-authored-by: Sylvestre Ledru <sledru@mozilla.com>
Signed-off-by: Xuanwo <github@xuanwo.io>
@@ -373,30 +382,31 @@ impl Storage for opendal::Operator {
}

/// Normalize key `abcdef` into `a/b/c/abcdef`
#[allow(dead_code)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it be removed altogether?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function will be used if feature like s3, azure enabled. Adding a dead_code is the most simple way to address clippy errors under --no-default-feature.

src/cache/cache.rs Outdated Show resolved Hide resolved
@@ -666,7 +665,7 @@ fn config_file(env_var: &str, leaf: &str) -> PathBuf {

#[derive(Debug, Default, PartialEq, Eq)]
pub struct Config {
pub caches: Vec<CacheType>,
pub cache: Option<CacheType>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this used anywhere before? Did multiple backends ever work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did multiple backends ever work?

No, they never works at the same time.

The only case is users configure different backends at the same time, and some of them are wrong. For example, users configure s3, azure, redis at the same time, and s3, azure is invalid, only redis works.

In old logic, the errors returned by s3 and azure will be ignore, and using redis finally. In this PR, we will only choose the first valid cache type and returns error happened which makes the behavior more predictable.

In the future, I will return errors if users configured different backends at the same time.

Copy link
Collaborator

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

It's unclear whether the changes of many backends to one is diserable or not. If that checks out, looks good to me otherwise

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
@sylvestre sylvestre merged commit eca88ce into mozilla:main Dec 30, 2022
@Xuanwo Xuanwo deleted the exit-while-misconfig branch January 1, 2023 05:03
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.

None yet

4 participants