Skip to content

Commit 7a230e3

Browse files
Cache at action level instead of group (#213)
#208 fixed one caching bug by changing when we update the cache. I'm unsure if that PR introduced this bug (it may have) but this PR fixes another. When ever the _same_ file path is used to cache on in subsequent actions, the cache is updated when the first succeeds, so the cache is read as valid for the second action when it should be invalid. Example config file ```yaml apiVersion: scope.github.com/v1alpha kind: ScopeDoctorGroup metadata: name: ssh description: Setup SSH keys for GitHub authentication spec: include: when-required needs: [] actions: - name: private-key-exists description: Ensures SSH private key exists check: paths: - ~/.ssh/id_ed25519 commands: - ./bin/ssh.sh check private-key fix: commands: - ./bin/ssh.sh fix private-key - name: public-key-exists description: Ensures SSH public key exists check: paths: - ~/.ssh/id_ed25519.pub commands: - ./bin/ssh.sh check public-key fix: commands: - ./bin/ssh.sh fix public-key - name: key-added description: Uploads the pub key to GitHub check: # The paths in this were previously erroneously cached too early # so the check was always passing paths: # if either of these files have changed, we need to re-check - ~/.ssh/id_ed25519 - ~/.ssh/id_ed25519.pub commands: - ./bin/ssh.sh check github fix: commands: - ./bin/ssh.sh fix github ``` This because we were keying _only_ off the group name and the fully qualified file name. This PR updates the cache to _also_ key off of the action name, so that each check has a unique key in the cache. Old cache format ```json { "checksums": { "rails-group": { "/path/to/file1.txt": "abc123", "/path/to/file2.txt": "def456" }, "ruby-group": { "/path/to/ruby-version": "xyz789" } } } ``` New cache format ```json { "checksums": { "rails-group": { "action-1": { "/path/to/file1.txt": "abc123" }, "action-2": { "/path/to/file2.txt": "def456" } }, "ruby-group": { "action-3": { "/path/to/ruby-version": "xyz789" } } } } ``` _IMPORTANT:_ This will invalidate any existing cache on a user's machine. The first time a user runs with this new version, we will invalidate all of their existing caches.
1 parent ed48782 commit 7a230e3

File tree

4 files changed

+775
-28
lines changed

4 files changed

+775
-28
lines changed

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

scope/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ assert_cmd = "2.0.16"
8787
assert_fs = "1.1.2"
8888
escargot = "0.5.12"
8989
predicates = "3.1.2"
90+
tempfile = "3.0"
9091

9192
[build-dependencies]
9293
vergen = { version = "8.3", features = ["build", "git", "git2"] }

scope/src/doctor/check.rs

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,7 @@ impl DefaultDoctorActionRun {
268268
&cache_path.base_path,
269269
&cache_path.paths,
270270
&self.model.metadata.name(),
271+
&self.action.name,
271272
self.file_cache.clone(),
272273
)
273274
.await;
@@ -592,6 +593,7 @@ impl DefaultDoctorActionRun {
592593
&paths.base_path,
593594
&paths.paths,
594595
&self.model.metadata.name(),
596+
&self.action.name,
595597
self.file_cache.clone(),
596598
)
597599
.await?;
@@ -682,6 +684,7 @@ pub trait GlobWalker: Send + Sync {
682684
base_dir: &Path,
683685
paths: &[String],
684686
cache_name: &str,
687+
action_name: &str,
685688
file_cache: Arc<dyn FileCache>,
686689
) -> Result<bool, RuntimeError>;
687690

@@ -690,6 +693,7 @@ pub trait GlobWalker: Send + Sync {
690693
base_dir: &Path,
691694
paths: &[String],
692695
cache_name: &str,
696+
action_name: &str,
693697
file_cache: Arc<dyn FileCache>,
694698
) -> Result<(), RuntimeError>;
695699
}
@@ -746,6 +750,7 @@ impl GlobWalker for DefaultGlobWalker {
746750
base_dir: &Path,
747751
paths: &[String],
748752
cache_name: &str,
753+
action_name: &str,
749754
file_cache: Arc<dyn FileCache>,
750755
) -> Result<bool, RuntimeError> {
751756
for glob_str in paths {
@@ -757,7 +762,9 @@ impl GlobWalker for DefaultGlobWalker {
757762
}
758763

759764
for path in files {
760-
let file_result = file_cache.check_file(cache_name.to_string(), &path).await?;
765+
let file_result = file_cache
766+
.check_file(cache_name, action_name, &path)
767+
.await?;
761768
debug!(target: "user", "CacheStatus for file {}: {:?}", path.display(), file_result);
762769
let check_result = file_result == FileCacheStatus::FileMatches;
763770
if !check_result {
@@ -774,13 +781,14 @@ impl GlobWalker for DefaultGlobWalker {
774781
base_dir: &Path,
775782
paths: &[String],
776783
cache_name: &str,
784+
action_name: &str,
777785
file_cache: Arc<dyn FileCache>,
778786
) -> Result<(), RuntimeError> {
779787
for glob_str in paths {
780788
let glob_path = make_absolute(base_dir, glob_str);
781789
for path in self.file_system.find_files(&glob_path)? {
782790
file_cache
783-
.update_cache_entry(cache_name.to_string(), &path)
791+
.update_cache_entry(cache_name, action_name, &path)
784792
.await?;
785793
}
786794
}
@@ -1068,11 +1076,11 @@ pub(crate) mod tests {
10681076
glob_walker
10691077
.expect_have_globs_changed()
10701078
.times(1)
1071-
.returning(|_, _, _, _| Ok(false));
1079+
.returning(|_, _, _, _, _| Ok(false));
10721080
glob_walker
10731081
.expect_update_cache()
10741082
.times(1)
1075-
.returning(|_, _, _, _| Ok(()));
1083+
.returning(|_, _, _, _, _| Ok(()));
10761084

10771085
let run = setup_test(vec![action], exec_runner, glob_walker);
10781086

@@ -1098,11 +1106,11 @@ pub(crate) mod tests {
10981106
glob_walker
10991107
.expect_have_globs_changed()
11001108
.times(1)
1101-
.returning(|_, _, _, _| Ok(false));
1109+
.returning(|_, _, _, _, _| Ok(false));
11021110
glob_walker
11031111
.expect_update_cache()
11041112
.times(1)
1105-
.returning(|_, _, _, _| Err(RuntimeError::AnyError(anyhow!("bogus error"))));
1113+
.returning(|_, _, _, _, _| Err(RuntimeError::AnyError(anyhow!("bogus error"))));
11061114

11071115
let run = setup_test(vec![action], exec_runner, glob_walker);
11081116

@@ -1128,7 +1136,7 @@ pub(crate) mod tests {
11281136
glob_walker
11291137
.expect_have_globs_changed()
11301138
.times(1)
1131-
.returning(|_, _, _, _| Ok(false));
1139+
.returning(|_, _, _, _, _| Ok(false));
11321140
glob_walker.expect_update_cache().never();
11331141

11341142
let run = setup_test(vec![action], exec_runner, glob_walker);
@@ -1170,11 +1178,11 @@ pub(crate) mod tests {
11701178
glob_walker
11711179
.expect_have_globs_changed()
11721180
.times(1)
1173-
.returning(|_, _, _, _| Ok(true));
1181+
.returning(|_, _, _, _, _| Ok(true));
11741182
glob_walker
11751183
.expect_update_cache()
11761184
.once()
1177-
.returning(|_, _, _, _| Ok(()));
1185+
.returning(|_, _, _, _, _| Ok(()));
11781186

11791187
exec_runner
11801188
.expect_run_command()
@@ -1228,11 +1236,11 @@ pub(crate) mod tests {
12281236
glob_walker
12291237
.expect_have_globs_changed()
12301238
.times(1)
1231-
.returning(|_, _, _, _| Ok(false));
1239+
.returning(|_, _, _, _, _| Ok(false));
12321240
glob_walker
12331241
.expect_update_cache()
12341242
.once()
1235-
.returning(|_, _, _, _| Ok(()));
1243+
.returning(|_, _, _, _, _| Ok(()));
12361244

12371245
exec_runner
12381246
.expect_run_command()
@@ -1267,9 +1275,10 @@ pub(crate) mod tests {
12671275
.once()
12681276
.with(
12691277
predicate::eq("file_cache".to_string()),
1278+
predicate::eq("action_name".to_string()),
12701279
predicate::eq(Path::new("/foo/bar")),
12711280
)
1272-
.returning(|_, _| Ok(()));
1281+
.returning(|_, _, _| Ok(()));
12731282

12741283
file_system
12751284
.expect_find_files()
@@ -1286,6 +1295,7 @@ pub(crate) mod tests {
12861295
Path::new("/foo/root"),
12871296
&["*.txt".to_string()],
12881297
"file_cache",
1298+
"action_name",
12891299
Arc::new(file_cache),
12901300
)
12911301
.await;
@@ -1302,9 +1312,10 @@ pub(crate) mod tests {
13021312
.once()
13031313
.with(
13041314
predicate::eq("file_cache".to_string()),
1315+
predicate::eq("action_name".to_string()),
13051316
predicate::eq(Path::new("/foo/bar")),
13061317
)
1307-
.returning(|_, _| Ok(()));
1318+
.returning(|_, _, _| Ok(()));
13081319

13091320
file_system
13101321
.expect_find_files()
@@ -1321,6 +1332,7 @@ pub(crate) mod tests {
13211332
Path::new("/foo/root"),
13221333
&["/a/abs/path/*.txt".to_string()],
13231334
"file_cache",
1335+
"action_name",
13241336
Arc::new(file_cache),
13251337
)
13261338
.await;
@@ -1340,9 +1352,10 @@ pub(crate) mod tests {
13401352
.once()
13411353
.with(
13421354
predicate::eq("group_name".to_string()),
1355+
predicate::eq("action_name".to_string()),
13431356
predicate::eq(resolved_path.clone()),
13441357
)
1345-
.returning(|_, _| Ok(()));
1358+
.returning(|_, _, _| Ok(()));
13461359

13471360
file_system
13481361
.expect_find_files()
@@ -1367,6 +1380,7 @@ pub(crate) mod tests {
13671380
Path::new("/foo/root"),
13681381
&["~/path/*.txt".to_string()],
13691382
"group_name",
1383+
"action_name",
13701384
Arc::new(file_cache),
13711385
)
13721386
.await;
@@ -1385,9 +1399,10 @@ pub(crate) mod tests {
13851399
.once()
13861400
.with(
13871401
predicate::eq("group_name".to_string()),
1402+
predicate::eq("action_name".to_string()),
13881403
predicate::eq(Path::new("/foo/path/foo.txt")),
13891404
)
1390-
.returning(|_, _| Ok(()));
1405+
.returning(|_, _, _| Ok(()));
13911406

13921407
file_system
13931408
.expect_find_files()
@@ -1407,6 +1422,7 @@ pub(crate) mod tests {
14071422
Path::new("/foo/root"),
14081423
&["../path/*.txt".to_string()],
14091424
"group_name",
1425+
"action_name",
14101426
Arc::new(file_cache),
14111427
)
14121428
.await;

0 commit comments

Comments
 (0)