From 4978400ece7c37ed51328da0667b2abb1b528fc7 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Sun, 21 Jul 2024 09:32:08 +0100 Subject: [PATCH] refactor: use Option<&T> over &Option --- .../repo-actor/src/handlers/advance_main.rs | 25 +++++++------- .../repo-actor/src/handlers/advance_next.rs | 2 +- .../src/handlers/check_ci_status.rs | 4 +-- crates/repo-actor/src/handlers/clone_repo.rs | 10 +++--- .../src/handlers/load_config_from_repo.rs | 2 +- .../src/handlers/receive_ci_status.rs | 12 ++++--- .../src/handlers/receive_repo_config.rs | 2 +- .../src/handlers/register_webhook.rs | 4 +-- .../repo-actor/src/handlers/validate_repo.rs | 33 ++++++++++--------- .../src/handlers/webhook_notification.rs | 29 +++++++++------- .../src/handlers/webhook_registered.rs | 2 +- crates/repo-actor/src/lib.rs | 12 +++---- 12 files changed, 72 insertions(+), 65 deletions(-) diff --git a/crates/repo-actor/src/handlers/advance_main.rs b/crates/repo-actor/src/handlers/advance_main.rs index 47424fb1..9efb10ce 100644 --- a/crates/repo-actor/src/handlers/advance_main.rs +++ b/crates/repo-actor/src/handlers/advance_main.rs @@ -30,21 +30,18 @@ impl Handler for actor::RepoActor { Err(err) => { tracing::warn!("advance main: {err}"); } - Ok(_) => { - let log = self.log.clone(); - match repo_config.source() { - git_next_config::RepoConfigSource::Repo => { - actor::do_send(addr, actor::messages::LoadConfigFromRepo, &log); - } - git_next_config::RepoConfigSource::Server => { - actor::do_send( - addr, - actor::messages::ValidateRepo::new(message_token), - &log, - ); - } + Ok(_) => match repo_config.source() { + git_next_config::RepoConfigSource::Repo => { + actor::do_send(addr, actor::messages::LoadConfigFromRepo, self.log.as_ref()); } - } + git_next_config::RepoConfigSource::Server => { + actor::do_send( + addr, + actor::messages::ValidateRepo::new(message_token), + self.log.as_ref(), + ); + } + }, } } } diff --git a/crates/repo-actor/src/handlers/advance_next.rs b/crates/repo-actor/src/handlers/advance_next.rs index da04615c..a8908d5e 100644 --- a/crates/repo-actor/src/handlers/advance_next.rs +++ b/crates/repo-actor/src/handlers/advance_next.rs @@ -35,7 +35,7 @@ impl Handler for actor::RepoActor { actor::do_send( addr, actor::messages::ValidateRepo::new(message_token), - &self.log, + self.log.as_ref(), ); } Err(err) => tracing::warn!("advance next: {err}"), diff --git a/crates/repo-actor/src/handlers/check_ci_status.rs b/crates/repo-actor/src/handlers/check_ci_status.rs index fc696863..bb834f4d 100644 --- a/crates/repo-actor/src/handlers/check_ci_status.rs +++ b/crates/repo-actor/src/handlers/check_ci_status.rs @@ -11,7 +11,7 @@ impl Handler for actor::RepoActor { msg: actor::messages::CheckCIStatus, ctx: &mut Self::Context, ) -> Self::Result { - actor::logger(&self.log, "start: CheckCIStatus"); + actor::logger(self.log.as_ref(), "start: CheckCIStatus"); let addr = ctx.address(); let forge = self.forge.duplicate(); let next = msg.unwrap(); @@ -24,7 +24,7 @@ impl Handler for actor::RepoActor { actor::do_send( addr, actor::messages::ReceiveCIStatus::new((next, status)), - &log, + log.as_ref(), ); } .in_current_span() diff --git a/crates/repo-actor/src/handlers/clone_repo.rs b/crates/repo-actor/src/handlers/clone_repo.rs index 7e214be4..ddc4efff 100644 --- a/crates/repo-actor/src/handlers/clone_repo.rs +++ b/crates/repo-actor/src/handlers/clone_repo.rs @@ -12,29 +12,29 @@ impl Handler for actor::RepoActor { _msg: actor::messages::CloneRepo, ctx: &mut Self::Context, ) -> Self::Result { - actor::logger(&self.log, "Handler: CloneRepo: start"); + actor::logger(self.log.as_ref(), "Handler: CloneRepo: start"); tracing::debug!("Handler: CloneRepo: start"); match git::repository::open(&*self.repository_factory, &self.repo_details) { Ok(repository) => { - actor::logger(&self.log, "open okay"); + actor::logger(self.log.as_ref(), "open okay"); tracing::debug!("open okay"); self.open_repository.replace(repository); if self.repo_details.repo_config.is_none() { actor::do_send( ctx.address(), actor::messages::LoadConfigFromRepo, - &self.log, + self.log.as_ref(), ); } else { actor::do_send( ctx.address(), actor::messages::RegisterWebhook::new(), - &self.log, + self.log.as_ref(), ); } } Err(err) => { - actor::logger(&self.log, "open failed"); + actor::logger(self.log.as_ref(), "open failed"); tracing::debug!("err: {err:?}"); tracing::warn!("Could not open repo: {err}") } diff --git a/crates/repo-actor/src/handlers/load_config_from_repo.rs b/crates/repo-actor/src/handlers/load_config_from_repo.rs index 4a64a451..d270ad5f 100644 --- a/crates/repo-actor/src/handlers/load_config_from_repo.rs +++ b/crates/repo-actor/src/handlers/load_config_from_repo.rs @@ -23,7 +23,7 @@ impl Handler for actor::RepoActor { async move { match actor::load::config_from_repository(repo_details, &*open_repository).await { Ok(repo_config) => { - actor::logger(&log, "send: LoadedConfig"); + actor::logger(log.as_ref(), "send: LoadedConfig"); addr.do_send(actor::messages::ReceiveRepoConfig::new(repo_config)) } Err(err) => { diff --git a/crates/repo-actor/src/handlers/receive_ci_status.rs b/crates/repo-actor/src/handlers/receive_ci_status.rs index b5fec18d..d5a5e811 100644 --- a/crates/repo-actor/src/handlers/receive_ci_status.rs +++ b/crates/repo-actor/src/handlers/receive_ci_status.rs @@ -12,7 +12,7 @@ impl Handler for actor::RepoActor { ctx: &mut Self::Context, ) -> Self::Result { let log = self.log.clone(); - actor::logger(&log, "start: ReceiveCIStatus"); + actor::logger(log.as_ref(), "start: ReceiveCIStatus"); let addr = ctx.address(); let (next, status) = msg.unwrap(); let message_token = self.message_token; @@ -21,14 +21,18 @@ impl Handler for actor::RepoActor { tracing::debug!(?status, ""); match status { git::forge::commit::Status::Pass => { - actor::do_send(addr, actor::messages::AdvanceMain::new(next), &self.log); + actor::do_send( + addr, + actor::messages::AdvanceMain::new(next), + self.log.as_ref(), + ); } git::forge::commit::Status::Pending => { std::thread::sleep(sleep_duration); actor::do_send( addr, actor::messages::ValidateRepo::new(message_token), - &self.log, + self.log.as_ref(), ); } git::forge::commit::Status::Fail => { @@ -38,7 +42,7 @@ impl Handler for actor::RepoActor { addr, sleep_duration, actor::messages::ValidateRepo::new(message_token), - &self.log, + self.log.as_ref(), ); } } diff --git a/crates/repo-actor/src/handlers/receive_repo_config.rs b/crates/repo-actor/src/handlers/receive_repo_config.rs index 2572d5aa..24004ee3 100644 --- a/crates/repo-actor/src/handlers/receive_repo_config.rs +++ b/crates/repo-actor/src/handlers/receive_repo_config.rs @@ -17,7 +17,7 @@ impl Handler for actor::RepoActor { actor::do_send( ctx.address(), actor::messages::RegisterWebhook::new(), - &self.log, + self.log.as_ref(), ); } } diff --git a/crates/repo-actor/src/handlers/register_webhook.rs b/crates/repo-actor/src/handlers/register_webhook.rs index 59edca2e..4ec6f195 100644 --- a/crates/repo-actor/src/handlers/register_webhook.rs +++ b/crates/repo-actor/src/handlers/register_webhook.rs @@ -25,7 +25,7 @@ impl Handler for RepoActor { actor::do_send( addr, actor::messages::WebhookRegistered::from(registered_webhook), - &log, + log.as_ref(), ); } Err(err) => { @@ -36,7 +36,7 @@ impl Handler for RepoActor { )); let log_message = format!("send: {:?}", msg); tracing::debug!(log_message); - actor::logger(&log, log_message); + actor::logger(log.as_ref(), log_message); if let Some(notify_user_recipient) = notify_user_recipient { notify_user_recipient.do_send(msg); } diff --git a/crates/repo-actor/src/handlers/validate_repo.rs b/crates/repo-actor/src/handlers/validate_repo.rs index a2bb08f9..3fc86cf3 100644 --- a/crates/repo-actor/src/handlers/validate_repo.rs +++ b/crates/repo-actor/src/handlers/validate_repo.rs @@ -14,14 +14,14 @@ impl Handler for actor::RepoActor { msg: actor::messages::ValidateRepo, ctx: &mut Self::Context, ) -> Self::Result { - actor::logger(&self.log, "start: ValidateRepo"); + actor::logger(self.log.as_ref(), "start: ValidateRepo"); // Message Token - make sure we are only triggered for the latest/current token match self.token_status(msg.unwrap()) { TokenStatus::Current => {} // do nothing TokenStatus::Expired => { actor::logger( - &self.log, + self.log.as_ref(), format!("discarded: old message token: {}", self.message_token), ); return; // message is expired @@ -29,24 +29,27 @@ impl Handler for actor::RepoActor { TokenStatus::New(message_token) => { self.message_token = message_token; actor::logger( - &self.log, + self.log.as_ref(), format!("new message token: {}", self.message_token), ); } } - actor::logger(&self.log, format!("accepted token: {}", self.message_token)); + actor::logger( + self.log.as_ref(), + format!("accepted token: {}", self.message_token), + ); // Repository positions let Some(ref open_repository) = self.open_repository else { - actor::logger(&self.log, "no open repository"); + actor::logger(self.log.as_ref(), "no open repository"); return; }; - actor::logger(&self.log, "have open repository"); + actor::logger(self.log.as_ref(), "have open repository"); let Some(repo_config) = self.repo_details.repo_config.clone() else { - actor::logger(&self.log, "no repo config"); + actor::logger(self.log.as_ref(), "no repo config"); return; }; - actor::logger(&self.log, "have repo config"); + actor::logger(self.log.as_ref(), "have repo config"); match git::validation::positions::validate_positions( &**open_repository, @@ -64,33 +67,33 @@ impl Handler for actor::RepoActor { actor::do_send( ctx.address(), actor::messages::CheckCIStatus::new(next), - &self.log, + self.log.as_ref(), ); } else if next != dev { actor::do_send( ctx.address(), actor::messages::AdvanceNext::new((next, dev_commit_history)), - &self.log, + self.log.as_ref(), ) } else { // do nothing } } Err(git::validation::positions::Error::Retryable(message)) => { - actor::logger(&self.log, message); + actor::logger(self.log.as_ref(), 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"); + actor::logger(log.as_ref(), "before sleep"); tokio::time::sleep(sleep_duration).await; - actor::logger(&log, "after sleep"); + actor::logger(log.as_ref(), "after sleep"); actor::do_send( addr, actor::messages::ValidateRepo::new(message_token), - &log, + log.as_ref(), ); } .in_current_span() @@ -99,7 +102,7 @@ impl Handler for actor::RepoActor { } Err(git::validation::positions::Error::NonRetryable(message)) | Err(git::validation::positions::Error::UserIntervention(message)) => { - actor::logger(&self.log, message); + actor::logger(self.log.as_ref(), message); } } diff --git a/crates/repo-actor/src/handlers/webhook_notification.rs b/crates/repo-actor/src/handlers/webhook_notification.rs index d9011373..4c6be742 100644 --- a/crates/repo-actor/src/handlers/webhook_notification.rs +++ b/crates/repo-actor/src/handlers/webhook_notification.rs @@ -17,23 +17,30 @@ impl Handler for actor::RepoActor { #[tracing::instrument(name = "RepoActor::WebhookMessage", skip_all, fields(token = %self.message_token, repo = %self.repo_details))] fn handle(&mut self, msg: WebhookNotification, ctx: &mut Self::Context) -> Self::Result { let Some(config) = &self.repo_details.repo_config else { - actor::logger(&self.log, "server has no repo config"); + actor::logger(self.log.as_ref(), "server has no repo config"); warn!("No repo config"); return; }; - if validate_notification(&msg, &self.webhook_auth, &*self.forge, &self.log).is_err() { + if validate_notification( + &msg, + self.webhook_auth.as_ref(), + &*self.forge, + self.log.as_ref(), + ) + .is_err() + { return; } let body = msg.body(); match self.forge.parse_webhook_body(body) { Err(err) => { - actor::logger(&self.log, "message parse error - not a push"); + actor::logger(self.log.as_ref(), "message parse error - not a push"); warn!(?err, "Not a 'push'"); return; } Ok(push) => match push.branch(config.branches()) { None => { - actor::logger(&self.log, "unknown branch"); + actor::logger(self.log.as_ref(), "unknown branch"); warn!( ?push, "Unrecognised branch, we should be filtering to only the ones we want" @@ -45,7 +52,7 @@ impl Handler for actor::RepoActor { push, config.branches().main(), &mut self.last_main_commit, - &self.log, + self.log.as_ref(), ) .is_err() { @@ -57,7 +64,7 @@ impl Handler for actor::RepoActor { push, config.branches().next(), &mut self.last_next_commit, - &self.log, + self.log.as_ref(), ) .is_err() { @@ -69,7 +76,7 @@ impl Handler for actor::RepoActor { push, config.branches().dev(), &mut self.last_dev_commit, - &self.log, + self.log.as_ref(), ) .is_err() { @@ -86,16 +93,16 @@ impl Handler for actor::RepoActor { actor::do_send( ctx.address(), actor::messages::ValidateRepo::new(message_token), - &self.log, + self.log.as_ref(), ); } } fn validate_notification( msg: &WebhookNotification, - webhook_auth: &Option, + webhook_auth: Option<&WebhookAuth>, forge: &dyn ForgeLike, - log: &Option, + log: Option<&RepoActorLog>, ) -> Result<(), ()> { let Some(expected_authorization) = webhook_auth else { actor::logger(log, "server has no auth token"); @@ -122,7 +129,7 @@ fn handle_push( push: Push, branch: BranchName, last_commit: &mut Option, - log: &Option, + log: Option<&RepoActorLog>, ) -> Result<(), ()> { actor::logger(log, "message is for dev branch"); let commit = git::Commit::from(push); diff --git a/crates/repo-actor/src/handlers/webhook_registered.rs b/crates/repo-actor/src/handlers/webhook_registered.rs index 398f56ed..ae718e78 100644 --- a/crates/repo-actor/src/handlers/webhook_registered.rs +++ b/crates/repo-actor/src/handlers/webhook_registered.rs @@ -16,7 +16,7 @@ impl Handler for actor::RepoActor { actor::do_send( ctx.address(), actor::messages::ValidateRepo::new(self.message_token), - &self.log, + self.log.as_ref(), ); } } diff --git a/crates/repo-actor/src/lib.rs b/crates/repo-actor/src/lib.rs index 1d29adfb..4e0557fe 100644 --- a/crates/repo-actor/src/lib.rs +++ b/crates/repo-actor/src/lib.rs @@ -118,12 +118,8 @@ impl Actor for RepoActor { } } -pub fn delay_send( - addr: Addr, - delay: Duration, - msg: M, - log: &Option, -) where +pub fn delay_send(addr: Addr, delay: Duration, msg: M, log: Option<&RepoActorLog>) +where M: actix::Message + Send + 'static + std::fmt::Debug, RepoActor: actix::Handler, ::Result: Send, @@ -135,7 +131,7 @@ pub fn delay_send( do_send(addr, msg, log) } -pub fn do_send(_addr: Addr, msg: M, log: &Option) +pub fn do_send(_addr: Addr, msg: M, log: Option<&RepoActorLog>) where M: actix::Message + Send + 'static + std::fmt::Debug, RepoActor: actix::Handler, @@ -148,7 +144,7 @@ where _addr.do_send(msg) } -pub fn logger(log: &Option, message: impl Into) { +pub fn logger(log: Option<&RepoActorLog>, message: impl Into) { if let Some(log) = log { let message: String = message.into(); tracing::debug!(message);