From ae7933c79ee6dc3190255282705ca030fd3d00a0 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Sun, 30 Jun 2024 18:39:43 +0100 Subject: [PATCH] fix: don't retry validation when non-retryable error Closes kemitix/git-next#90 --- crates/git/src/validation/positions.rs | 79 ++++++++---------- crates/git/src/validation/tests.rs | 50 ++++-------- crates/repo-actor/README.md | 3 +- .../repo-actor/src/handlers/validate_repo.rs | 22 +++-- .../src/tests/handlers/validate_repo.rs | 80 ++++++++++++++++++- 5 files changed, 141 insertions(+), 93 deletions(-) diff --git a/crates/git/src/validation/positions.rs b/crates/git/src/validation/positions.rs index 6db32e7..f0dfc82 100644 --- a/crates/git/src/validation/positions.rs +++ b/crates/git/src/validation/positions.rs @@ -23,32 +23,29 @@ pub fn validate_positions( let dev_branch = repo_config.branches().dev(); // Collect Commit Histories for `main`, `next` and `dev` branches open_repository.fetch()?; - let commit_histories = - get_commit_histories(open_repository, &repo_config).map_err(Error::CommitLog)?; + let commit_histories = get_commit_histories(open_repository, &repo_config)?; // branch tips - let main = commit_histories - .main - .first() - .cloned() - .ok_or_else(|| Error::BranchHasNoCommits(main_branch.clone()))?; - let next = commit_histories - .next - .first() - .cloned() - .ok_or_else(|| Error::BranchHasNoCommits(next_branch.clone()))?; + let main = + commit_histories.main.first().cloned().ok_or_else(|| { + Error::NonRetryable(format!("Branch has no commits: {}", main_branch)) + })?; + let next = + commit_histories.next.first().cloned().ok_or_else(|| { + Error::NonRetryable(format!("Branch has no commits: {}", next_branch)) + })?; let dev = commit_histories .dev .first() .cloned() - .ok_or_else(|| Error::BranchHasNoCommits(dev_branch.clone()))?; + .ok_or_else(|| Error::NonRetryable(format!("Branch has no commits: {}", dev_branch)))?; // Validations: // Dev must be on main branch, else the USER must rebase it if is_not_based_on(&commit_histories.dev, &main) { tracing::warn!("Dev '{dev_branch}' not based on main '{main_branch}' - user must rebase",); - return Err(Error::DevBranchNotBasedOn { - dev: dev_branch, - other: main_branch, - }); + return Err(Error::NonRetryable(format!( + "Branch '{}' not based on '{}'", + dev_branch, main_branch + ))); } // verify that next is on main or at most one commit on top of main, else reset it back to main if is_not_based_on( @@ -93,13 +90,13 @@ fn reset_next_to_main( &git::push::Force::From(next.clone().into()), ) .map_err(|err| { - tracing::warn!(?err, "Failed to reset next to main"); - Error::FailedToResetBranch { - branch: next_branch.clone(), - commit: next.clone(), - } + Error::NonRetryable(format!( + "Failed to reset branch '{next_branch}' to commit '{next}': {err}" + )) })?; - Err(Error::NextBranchResetRequired(next_branch.clone())) + Err(Error::Retryable(format!( + "Branch {next_branch} has been reset" + ))) } fn is_not_based_on(commits: &[crate::commit::Commit], needle: &crate::Commit) -> bool { @@ -120,27 +117,19 @@ fn get_commit_histories( #[derive(Debug, thiserror::Error)] pub enum Error { - #[error("fetch: {0}")] - Fetch(#[from] git::fetch::Error), + #[error("{0} - will retry")] + Retryable(String), - #[error("commit log: {0}")] - CommitLog(#[from] git::commit::log::Error), - - #[error("failed to reset branch '{branch}' to {commit}")] - FailedToResetBranch { - branch: config::BranchName, - commit: git::Commit, - }, - - #[error("next branch '{0}' needs to be reset")] - NextBranchResetRequired(config::BranchName), - - #[error("branch '{0}' has no commits")] - BranchHasNoCommits(config::BranchName), - - #[error("dev branch '{dev}' not based on branch '{other}' ")] - DevBranchNotBasedOn { - dev: config::BranchName, - other: config::BranchName, - }, + #[error("{0} - not retrying")] + NonRetryable(String), +} +impl From for Error { + fn from(value: git::fetch::Error) -> Self { + Self::Retryable(value.to_string()) + } +} +impl From for Error { + fn from(value: git::commit::log::Error) -> Self { + Self::Retryable(value.to_string()) + } } diff --git a/crates/git/src/validation/tests.rs b/crates/git/src/validation/tests.rs index 46096cb..a736316 100644 --- a/crates/git/src/validation/tests.rs +++ b/crates/git/src/validation/tests.rs @@ -80,7 +80,7 @@ mod positions { assert!(matches!( err, - git::validation::positions::Error::Fetch(git::fetch::Error::TestFailureExpected) + git::validation::positions::Error::Retryable(_) )); } @@ -113,17 +113,13 @@ mod positions { "open repo" ); let repo_details = given::repo_details(&fs); - let main_branch = repo_config.branches().main(); let result = validate_positions(&*open_repository, &repo_details, repo_config); println!("{result:?}"); assert!(matches!( result, - Err(git::validation::positions::Error::CommitLog(git::commit::log::Error::Gix { - branch, - error - })) if branch == main_branch && error == "foo" + Err(git::validation::positions::Error::Retryable(_)) )); } @@ -156,17 +152,13 @@ mod positions { "open repo" ); let repo_details = given::repo_details(&fs); - let next_branch = repo_config.branches().next(); let result = validate_positions(&*open_repository, &repo_details, repo_config); println!("{result:?}"); assert!(matches!( result, - Err(git::validation::positions::Error::CommitLog(git::commit::log::Error::Gix { - branch, - error - })) if branch == next_branch && error == "foo" + Err(git::validation::positions::Error::Retryable(_)) )); } @@ -199,17 +191,13 @@ mod positions { "open repo" ); let repo_details = given::repo_details(&fs); - let dev_branch = repo_config.branches().dev(); let result = validate_positions(&*open_repository, &repo_details, repo_config); println!("{result:?}"); assert!(matches!( result, - Err(git::validation::positions::Error::CommitLog(git::commit::log::Error::Gix { - branch, - error - })) if branch == dev_branch && error == "foo" + Err(git::validation::positions::Error::Retryable(_)) )); } @@ -251,16 +239,14 @@ mod positions { //when let_assert!( - Err(err) = - validate_positions(&*open_repository, &repo_details, repo_config.clone()), + Err(err) = validate_positions(&*open_repository, &repo_details, repo_config), "validate" ); //then assert!(matches!( err, - git::validation::positions::Error::DevBranchNotBasedOn { dev, other } - if dev == repo_config.branches().dev() && other == repo_config.branches().main() + git::validation::positions::Error::NonRetryable(_) )); } @@ -338,8 +324,7 @@ mod positions { //when let_assert!( - Err(err) = - validate_positions(&*open_repository, &repo_details, repo_config.clone()), + Err(err) = validate_positions(&*open_repository, &repo_details, repo_config), "validate" ); @@ -348,8 +333,7 @@ mod positions { // NOTE: assertions for correct push are in on_push above assert!(matches!( err, - git::validation::positions::Error::NextBranchResetRequired(branch) - if branch == repo_config.branches().next() + git::validation::positions::Error::Retryable(_) )); } @@ -417,14 +401,12 @@ mod positions { //then println!("Got: {err:?}"); let_assert!( - Ok(sha_next) = - then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()), + Ok(_) = then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()), "load next branch sha" ); assert!(matches!( err, - git::validation::positions::Error::FailedToResetBranch{branch, commit} - if branch == repo_config.branches().next() && commit.sha() == &sha_next + git::validation::positions::Error::NonRetryable(_) )); } @@ -499,8 +481,7 @@ mod positions { //when let_assert!( - Err(err) = - validate_positions(&*open_repository, &repo_details, repo_config.clone()), + Err(err) = validate_positions(&*open_repository, &repo_details, repo_config), "validate" ); @@ -509,8 +490,7 @@ mod positions { // NOTE: assertions for correct push are in on_push above assert!(matches!( err, - git::validation::positions::Error::NextBranchResetRequired(branch) - if branch == repo_config.branches().next() + git::validation::positions::Error::Retryable(_) )); } @@ -576,14 +556,12 @@ mod positions { //then println!("Got: {err:?}"); let_assert!( - Ok(sha_next) = - then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()), + Ok(_) = then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()), "load next branch sha" ); assert!(matches!( err, - git::validation::positions::Error::FailedToResetBranch{branch, commit} - if branch == repo_config.branches().next() && commit.sha() == &sha_next + git::validation::positions::Error::NonRetryable(_) )); } diff --git a/crates/repo-actor/README.md b/crates/repo-actor/README.md index 217bae1..a4f5032 100644 --- a/crates/repo-actor/README.md +++ b/crates/repo-actor/README.md @@ -10,7 +10,8 @@ LoadConfigFromRepo --> ReceiveRepoConfig ValidateRepo --> CheckCIStatus :on next ahead of main ValidateRepo --> AdvanceNext :on dev ahead of next ValidateRepo --> [*] :on dev == next == main -ValidateRepo --> ValidateRepo :on invalid +ValidateRepo --> [*] :on non-retryable error +ValidateRepo --> ValidateRepo :on retryable error CheckCIStatus --> ReceiveCIStatus diff --git a/crates/repo-actor/src/handlers/validate_repo.rs b/crates/repo-actor/src/handlers/validate_repo.rs index 2fb3763..1a7aac4 100644 --- a/crates/repo-actor/src/handlers/validate_repo.rs +++ b/crates/repo-actor/src/handlers/validate_repo.rs @@ -38,11 +38,15 @@ impl Handler for actor::RepoActor { // Repository positions let Some(ref open_repository) = self.open_repository else { + actor::logger(&self.log, "no open repository"); return; }; + actor::logger(&self.log, "have open repository"); let Some(repo_config) = self.repo_details.repo_config.clone() else { + actor::logger(&self.log, "no repo config"); return; }; + actor::logger(&self.log, "have repo config"); match git::validation::positions::validate_positions( &**open_repository, @@ -71,29 +75,31 @@ impl Handler for actor::RepoActor { // do nothing } } - Err(err) => { - actor::logger( - &self.log, - format!("invalid positions: {err:?} will sleep then retry"), - ); - tracing::warn!("Error: {err:?}"); + Err(git::validation::positions::Error::Retryable(message)) => { + actor::logger(&self.log, message); let addr = ctx.address(); let message_token = self.message_token; let sleep_duration = self.sleep_duration; let log = self.log.clone(); async move { tracing::debug!("sleeping before retrying..."); + actor::logger(&log, "before sleep"); tokio::time::sleep(sleep_duration).await; - actor::do_send( + actor::logger(&log, "after sleep"); + let _ = actor::send( addr, actor::messages::ValidateRepo::new(message_token), &log, - ); + ) + .await; } .in_current_span() .into_actor(self) .wait(ctx); } + Err(git::validation::positions::Error::NonRetryable(message)) => { + actor::logger(&self.log, message); + } } tracing::debug!("Handler: ValidateRepo: finish"); diff --git a/crates/repo-actor/src/tests/handlers/validate_repo.rs b/crates/repo-actor/src/tests/handlers/validate_repo.rs index c058cd7..72040e8 100644 --- a/crates/repo-actor/src/tests/handlers/validate_repo.rs +++ b/crates/repo-actor/src/tests/handlers/validate_repo.rs @@ -43,7 +43,7 @@ async fn repo_with_next_not_an_ancestor_of_dev_should_be_reset() -> TestResult { System::current().stop(); //then - log.require_message_containing("NextBranchResetRequired")?; + log.require_message_containing(format!("Branch {} has been reset", branches.next()))?; Ok(()) } @@ -89,7 +89,7 @@ async fn repo_with_next_not_on_or_near_main_should_be_reset() -> TestResult { System::current().stop(); //then - log.require_message_containing("NextBranchResetRequired")?; + log.require_message_containing(format!("Branch {} has been reset", branches.next()))?; Ok(()) } @@ -135,7 +135,7 @@ async fn repo_with_next_not_based_on_main_should_be_reset() -> TestResult { System::current().stop(); //then - log.require_message_containing("NextBranchResetRequired")?; + log.require_message_containing(format!("Branch {} has been reset", branches.next()))?; Ok(()) } @@ -352,3 +352,77 @@ async fn should_reject_message_with_expired_token() -> TestResult { log.no_message_contains("accepted token")?; Ok(()) } + +#[test_log::test(actix::test)] +async fn should_send_validate_repo_when_retryable_error() -> TestResult { + //given + let fs = given::a_filesystem(); + let (mut open_repository, repo_details) = given::an_open_repository(&fs); + open_repository.expect_fetch().return_once(|| Ok(())); + open_repository + .expect_commit_log() + .return_once(|_, _| Err(git::commit::log::Error::Lock)); + + //when + let (addr, log) = when::start_actor_with_open_repository( + Box::new(open_repository), + repo_details, + given::a_forge(), + ); + addr.send(crate::messages::ValidateRepo::new(MessageToken::default())) + .await?; + tokio::time::sleep(std::time::Duration::from_millis(1)).await; + System::current().stop(); + + //then + log.require_message_containing("accepted token: 0")?; + log.require_message_containing("send: ValidateRepo")?; + Ok(()) +} + +#[test_log::test(actix::test)] +async fn should_send_nothing_when_non_retryable_error() -> TestResult { + //given + let fs = given::a_filesystem(); + let (mut open_repository, repo_details) = given::an_open_repository(&fs); + #[allow(clippy::unwrap_used)] + let repo_config = repo_details.repo_config.clone().unwrap(); + open_repository.expect_fetch().return_once(|| Ok(())); + + // branches are all unrelated - non-retryable until each branch is updated + let branches = repo_config.branches(); + // commit_log main + let main_commit = expect::main_commit_log(&mut open_repository, branches.main()); + // next + let next_branch_log = vec![given::a_commit()]; + // dev + let dev_branch_log = vec![given::a_commit()]; + // commit_log next + open_repository + .expect_commit_log() + .times(1) + .with(eq(branches.next()), eq([main_commit.clone()])) + .return_once(move |_, _| Ok(next_branch_log)); + // commit_log dev + open_repository + .expect_commit_log() + .times(1) + .with(eq(branches.dev()), eq([main_commit])) + .return_once(|_, _| Ok(dev_branch_log)); + + //when + let (addr, log) = when::start_actor_with_open_repository( + Box::new(open_repository), + repo_details, + given::a_forge(), + ); + addr.send(crate::messages::ValidateRepo::new(MessageToken::default())) + .await?; + tokio::time::sleep(std::time::Duration::from_millis(1)).await; + System::current().stop(); + + //then + log.require_message_containing("accepted token")?; + log.no_message_contains("send:")?; + Ok(()) +}