fix: don't retry validation when non-retryable error
All checks were successful
Rust / build (push) Successful in 1m29s
ci/woodpecker/push/cron-docker-builder Pipeline was successful
ci/woodpecker/push/push-next Pipeline was successful
ci/woodpecker/push/tag-created Pipeline was successful

Closes kemitix/git-next#90
This commit is contained in:
Paul Campbell 2024-06-30 18:39:43 +01:00
parent c9efbb9936
commit ae7933c79e
5 changed files with 141 additions and 93 deletions

View file

@ -23,32 +23,29 @@ pub fn validate_positions(
let dev_branch = repo_config.branches().dev(); let dev_branch = repo_config.branches().dev();
// Collect Commit Histories for `main`, `next` and `dev` branches // Collect Commit Histories for `main`, `next` and `dev` branches
open_repository.fetch()?; open_repository.fetch()?;
let commit_histories = let commit_histories = get_commit_histories(open_repository, &repo_config)?;
get_commit_histories(open_repository, &repo_config).map_err(Error::CommitLog)?;
// branch tips // branch tips
let main = commit_histories let main =
.main commit_histories.main.first().cloned().ok_or_else(|| {
.first() Error::NonRetryable(format!("Branch has no commits: {}", main_branch))
.cloned() })?;
.ok_or_else(|| Error::BranchHasNoCommits(main_branch.clone()))?; let next =
let next = commit_histories commit_histories.next.first().cloned().ok_or_else(|| {
.next Error::NonRetryable(format!("Branch has no commits: {}", next_branch))
.first() })?;
.cloned()
.ok_or_else(|| Error::BranchHasNoCommits(next_branch.clone()))?;
let dev = commit_histories let dev = commit_histories
.dev .dev
.first() .first()
.cloned() .cloned()
.ok_or_else(|| Error::BranchHasNoCommits(dev_branch.clone()))?; .ok_or_else(|| Error::NonRetryable(format!("Branch has no commits: {}", dev_branch)))?;
// Validations: // Validations:
// Dev must be on main branch, else the USER must rebase it // Dev must be on main branch, else the USER must rebase it
if is_not_based_on(&commit_histories.dev, &main) { if is_not_based_on(&commit_histories.dev, &main) {
tracing::warn!("Dev '{dev_branch}' not based on main '{main_branch}' - user must rebase",); tracing::warn!("Dev '{dev_branch}' not based on main '{main_branch}' - user must rebase",);
return Err(Error::DevBranchNotBasedOn { return Err(Error::NonRetryable(format!(
dev: dev_branch, "Branch '{}' not based on '{}'",
other: main_branch, 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 // 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( if is_not_based_on(
@ -93,13 +90,13 @@ fn reset_next_to_main(
&git::push::Force::From(next.clone().into()), &git::push::Force::From(next.clone().into()),
) )
.map_err(|err| { .map_err(|err| {
tracing::warn!(?err, "Failed to reset next to main"); Error::NonRetryable(format!(
Error::FailedToResetBranch { "Failed to reset branch '{next_branch}' to commit '{next}': {err}"
branch: next_branch.clone(), ))
commit: next.clone(),
}
})?; })?;
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 { 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)] #[derive(Debug, thiserror::Error)]
pub enum Error { pub enum Error {
#[error("fetch: {0}")] #[error("{0} - will retry")]
Fetch(#[from] git::fetch::Error), Retryable(String),
#[error("commit log: {0}")] #[error("{0} - not retrying")]
CommitLog(#[from] git::commit::log::Error), NonRetryable(String),
}
#[error("failed to reset branch '{branch}' to {commit}")] impl From<git::fetch::Error> for Error {
FailedToResetBranch { fn from(value: git::fetch::Error) -> Self {
branch: config::BranchName, Self::Retryable(value.to_string())
commit: git::Commit, }
}, }
impl From<git::commit::log::Error> for Error {
#[error("next branch '{0}' needs to be reset")] fn from(value: git::commit::log::Error) -> Self {
NextBranchResetRequired(config::BranchName), Self::Retryable(value.to_string())
}
#[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,
},
} }

View file

@ -80,7 +80,7 @@ mod positions {
assert!(matches!( assert!(matches!(
err, err,
git::validation::positions::Error::Fetch(git::fetch::Error::TestFailureExpected) git::validation::positions::Error::Retryable(_)
)); ));
} }
@ -113,17 +113,13 @@ mod positions {
"open repo" "open repo"
); );
let repo_details = given::repo_details(&fs); let repo_details = given::repo_details(&fs);
let main_branch = repo_config.branches().main();
let result = validate_positions(&*open_repository, &repo_details, repo_config); let result = validate_positions(&*open_repository, &repo_details, repo_config);
println!("{result:?}"); println!("{result:?}");
assert!(matches!( assert!(matches!(
result, result,
Err(git::validation::positions::Error::CommitLog(git::commit::log::Error::Gix { Err(git::validation::positions::Error::Retryable(_))
branch,
error
})) if branch == main_branch && error == "foo"
)); ));
} }
@ -156,17 +152,13 @@ mod positions {
"open repo" "open repo"
); );
let repo_details = given::repo_details(&fs); let repo_details = given::repo_details(&fs);
let next_branch = repo_config.branches().next();
let result = validate_positions(&*open_repository, &repo_details, repo_config); let result = validate_positions(&*open_repository, &repo_details, repo_config);
println!("{result:?}"); println!("{result:?}");
assert!(matches!( assert!(matches!(
result, result,
Err(git::validation::positions::Error::CommitLog(git::commit::log::Error::Gix { Err(git::validation::positions::Error::Retryable(_))
branch,
error
})) if branch == next_branch && error == "foo"
)); ));
} }
@ -199,17 +191,13 @@ mod positions {
"open repo" "open repo"
); );
let repo_details = given::repo_details(&fs); let repo_details = given::repo_details(&fs);
let dev_branch = repo_config.branches().dev();
let result = validate_positions(&*open_repository, &repo_details, repo_config); let result = validate_positions(&*open_repository, &repo_details, repo_config);
println!("{result:?}"); println!("{result:?}");
assert!(matches!( assert!(matches!(
result, result,
Err(git::validation::positions::Error::CommitLog(git::commit::log::Error::Gix { Err(git::validation::positions::Error::Retryable(_))
branch,
error
})) if branch == dev_branch && error == "foo"
)); ));
} }
@ -251,16 +239,14 @@ mod positions {
//when //when
let_assert!( let_assert!(
Err(err) = Err(err) = validate_positions(&*open_repository, &repo_details, repo_config),
validate_positions(&*open_repository, &repo_details, repo_config.clone()),
"validate" "validate"
); );
//then //then
assert!(matches!( assert!(matches!(
err, err,
git::validation::positions::Error::DevBranchNotBasedOn { dev, other } git::validation::positions::Error::NonRetryable(_)
if dev == repo_config.branches().dev() && other == repo_config.branches().main()
)); ));
} }
@ -338,8 +324,7 @@ mod positions {
//when //when
let_assert!( let_assert!(
Err(err) = Err(err) = validate_positions(&*open_repository, &repo_details, repo_config),
validate_positions(&*open_repository, &repo_details, repo_config.clone()),
"validate" "validate"
); );
@ -348,8 +333,7 @@ mod positions {
// NOTE: assertions for correct push are in on_push above // NOTE: assertions for correct push are in on_push above
assert!(matches!( assert!(matches!(
err, err,
git::validation::positions::Error::NextBranchResetRequired(branch) git::validation::positions::Error::Retryable(_)
if branch == repo_config.branches().next()
)); ));
} }
@ -417,14 +401,12 @@ mod positions {
//then //then
println!("Got: {err:?}"); println!("Got: {err:?}");
let_assert!( let_assert!(
Ok(sha_next) = Ok(_) = then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()),
then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()),
"load next branch sha" "load next branch sha"
); );
assert!(matches!( assert!(matches!(
err, err,
git::validation::positions::Error::FailedToResetBranch{branch, commit} git::validation::positions::Error::NonRetryable(_)
if branch == repo_config.branches().next() && commit.sha() == &sha_next
)); ));
} }
@ -499,8 +481,7 @@ mod positions {
//when //when
let_assert!( let_assert!(
Err(err) = Err(err) = validate_positions(&*open_repository, &repo_details, repo_config),
validate_positions(&*open_repository, &repo_details, repo_config.clone()),
"validate" "validate"
); );
@ -509,8 +490,7 @@ mod positions {
// NOTE: assertions for correct push are in on_push above // NOTE: assertions for correct push are in on_push above
assert!(matches!( assert!(matches!(
err, err,
git::validation::positions::Error::NextBranchResetRequired(branch) git::validation::positions::Error::Retryable(_)
if branch == repo_config.branches().next()
)); ));
} }
@ -576,14 +556,12 @@ mod positions {
//then //then
println!("Got: {err:?}"); println!("Got: {err:?}");
let_assert!( let_assert!(
Ok(sha_next) = Ok(_) = then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()),
then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()),
"load next branch sha" "load next branch sha"
); );
assert!(matches!( assert!(matches!(
err, err,
git::validation::positions::Error::FailedToResetBranch{branch, commit} git::validation::positions::Error::NonRetryable(_)
if branch == repo_config.branches().next() && commit.sha() == &sha_next
)); ));
} }

View file

@ -10,7 +10,8 @@ LoadConfigFromRepo --> ReceiveRepoConfig
ValidateRepo --> CheckCIStatus :on next ahead of main ValidateRepo --> CheckCIStatus :on next ahead of main
ValidateRepo --> AdvanceNext :on dev ahead of next ValidateRepo --> AdvanceNext :on dev ahead of next
ValidateRepo --> [*] :on dev == next == main ValidateRepo --> [*] :on dev == next == main
ValidateRepo --> ValidateRepo :on invalid ValidateRepo --> [*] :on non-retryable error
ValidateRepo --> ValidateRepo :on retryable error
CheckCIStatus --> ReceiveCIStatus CheckCIStatus --> ReceiveCIStatus

View file

@ -38,11 +38,15 @@ impl Handler<actor::messages::ValidateRepo> for actor::RepoActor {
// Repository positions // Repository positions
let Some(ref open_repository) = self.open_repository else { let Some(ref open_repository) = self.open_repository else {
actor::logger(&self.log, "no open repository");
return; return;
}; };
actor::logger(&self.log, "have open repository");
let Some(repo_config) = self.repo_details.repo_config.clone() else { let Some(repo_config) = self.repo_details.repo_config.clone() else {
actor::logger(&self.log, "no repo config");
return; return;
}; };
actor::logger(&self.log, "have repo config");
match git::validation::positions::validate_positions( match git::validation::positions::validate_positions(
&**open_repository, &**open_repository,
@ -71,29 +75,31 @@ impl Handler<actor::messages::ValidateRepo> for actor::RepoActor {
// do nothing // do nothing
} }
} }
Err(err) => { Err(git::validation::positions::Error::Retryable(message)) => {
actor::logger( actor::logger(&self.log, message);
&self.log,
format!("invalid positions: {err:?} will sleep then retry"),
);
tracing::warn!("Error: {err:?}");
let addr = ctx.address(); let addr = ctx.address();
let message_token = self.message_token; let message_token = self.message_token;
let sleep_duration = self.sleep_duration; let sleep_duration = self.sleep_duration;
let log = self.log.clone(); let log = self.log.clone();
async move { async move {
tracing::debug!("sleeping before retrying..."); tracing::debug!("sleeping before retrying...");
actor::logger(&log, "before sleep");
tokio::time::sleep(sleep_duration).await; tokio::time::sleep(sleep_duration).await;
actor::do_send( actor::logger(&log, "after sleep");
let _ = actor::send(
addr, addr,
actor::messages::ValidateRepo::new(message_token), actor::messages::ValidateRepo::new(message_token),
&log, &log,
); )
.await;
} }
.in_current_span() .in_current_span()
.into_actor(self) .into_actor(self)
.wait(ctx); .wait(ctx);
} }
Err(git::validation::positions::Error::NonRetryable(message)) => {
actor::logger(&self.log, message);
}
} }
tracing::debug!("Handler: ValidateRepo: finish"); tracing::debug!("Handler: ValidateRepo: finish");

View file

@ -43,7 +43,7 @@ async fn repo_with_next_not_an_ancestor_of_dev_should_be_reset() -> TestResult {
System::current().stop(); System::current().stop();
//then //then
log.require_message_containing("NextBranchResetRequired")?; log.require_message_containing(format!("Branch {} has been reset", branches.next()))?;
Ok(()) Ok(())
} }
@ -89,7 +89,7 @@ async fn repo_with_next_not_on_or_near_main_should_be_reset() -> TestResult {
System::current().stop(); System::current().stop();
//then //then
log.require_message_containing("NextBranchResetRequired")?; log.require_message_containing(format!("Branch {} has been reset", branches.next()))?;
Ok(()) Ok(())
} }
@ -135,7 +135,7 @@ async fn repo_with_next_not_based_on_main_should_be_reset() -> TestResult {
System::current().stop(); System::current().stop();
//then //then
log.require_message_containing("NextBranchResetRequired")?; log.require_message_containing(format!("Branch {} has been reset", branches.next()))?;
Ok(()) Ok(())
} }
@ -352,3 +352,77 @@ async fn should_reject_message_with_expired_token() -> TestResult {
log.no_message_contains("accepted token")?; log.no_message_contains("accepted token")?;
Ok(()) 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(())
}