Merge pull request #180 from vohoanglong0107/refactor-cleanup-repository-loader

refactor: clean up RepositoryLoader

Author: Kobzol

src/bin/bors.rs

@@ -6,8 +6,8 @@ use std::time::Duration;
 
 use anyhow::Context;
 use bors::{
-    create_app, create_bors_process, create_github_client, BorsContext, BorsGlobalEvent,
-    CommandParser, PgDbClient, RepositoryLoader, ServerState, TeamApiClient, WebhookSecret,
+    create_app, create_bors_process, create_github_client, load_repositories, BorsContext,
+    BorsGlobalEvent, CommandParser, PgDbClient, ServerState, TeamApiClient, WebhookSecret,
 };
 use clap::Parser;
 use sqlx::postgres::PgConnectOptions;
@@ -88,9 +88,7 @@ fn try_main(opts: Opts) -> anyhow::Result<()> {
             "https://api.github.com".to_string(),
             opts.private_key.into(),
         )?;
-        let repos = RepositoryLoader::new(client.clone())
-            .load_repositories(&team_api)
-            .await?;
+        let repos = load_repositories(&client, &team_api).await?;
         Ok::<_, anyhow::Error>((client, repos))
     })?;
 

src/bors/comment.rs

@@ -47,3 +47,11 @@ Build commit: {} (`{}`)"#,
         }),
     }
 }
+
+pub fn try_build_in_progress_comment() -> Comment {
+    Comment::new(":exclamation: A try build is currently in progress. You can cancel it using @bors try cancel.".to_string())
+}
+
+pub fn cant_find_last_parent_comment() -> Comment {
+    Comment::new(":exclamation: There was no previous build. Please set an explicit parent or remove the `parent=last` argument to use the default parent.".to_string())
+}

src/bors/handlers/mod.rs

@@ -1,6 +1,7 @@
 use std::sync::Arc;
 
 use anyhow::Context;
+use octocrab::Octocrab;
 use tracing::Instrument;
 
 use crate::bors::command::{BorsCommand, CommandParseError};
@@ -15,8 +16,8 @@ use crate::bors::handlers::trybuild::{command_try_build, command_try_cancel, TRY
 use crate::bors::handlers::workflow::{
     handle_check_suite_completed, handle_workflow_completed, handle_workflow_started,
 };
-use crate::bors::{BorsContext, Comment, RepositoryLoader, RepositoryState};
-use crate::{PgDbClient, TeamApiClient};
+use crate::bors::{BorsContext, Comment, RepositoryState};
+use crate::{load_repositories, PgDbClient, TeamApiClient};
 
 #[cfg(test)]
 use crate::tests::util::TestSyncMarker;
@@ -141,14 +142,14 @@ pub static WAIT_FOR_REFRESH: TestSyncMarker = TestSyncMarker::new();
 pub async fn handle_bors_global_event(
     event: BorsGlobalEvent,
     ctx: Arc<BorsContext>,
-    repo_loader: &RepositoryLoader,
+    gh_client: &Octocrab,
     team_api_client: &TeamApiClient,
 ) -> anyhow::Result<()> {
     let db = Arc::clone(&ctx.db);
     match event {
         BorsGlobalEvent::InstallationsChanged => {
             let span = tracing::info_span!("Installations changed");
-            reload_repos(ctx, repo_loader, team_api_client)
+            reload_repos(ctx, gh_client, team_api_client)
                 .instrument(span)
                 .await?;
         }
@@ -275,10 +276,10 @@ async fn handle_comment(
 
 async fn reload_repos(
     ctx: Arc<BorsContext>,
-    repo_loader: &RepositoryLoader,
+    gh_client: &Octocrab,
     team_api_client: &TeamApiClient,
 ) -> anyhow::Result<()> {
-    let reloaded_repos = repo_loader.load_repositories(team_api_client).await?;
+    let reloaded_repos = load_repositories(gh_client, team_api_client).await?;
     let mut repositories = ctx.repositories.write().unwrap();
     for repo in repositories.values() {
         if !reloaded_repos.contains_key(repo.repository()) {

src/bors/handlers/trybuild.rs

@@ -3,6 +3,8 @@ use std::sync::Arc;
 use anyhow::{anyhow, Context};
 
 use crate::bors::command::Parent;
+use crate::bors::comment::cant_find_last_parent_comment;
+use crate::bors::comment::try_build_in_progress_comment;
 use crate::bors::handlers::labels::handle_label_trigger;
 use crate::bors::Comment;
 use crate::bors::RepositoryState;
@@ -47,37 +49,17 @@ pub(super) async fn command_try_build(
         .await
         .context("Cannot find or create PR")?;
 
-    let last_parent = if let Some(ref build) = pr_model.try_build {
-        if build.status == BuildStatus::Pending {
-            tracing::warn!("Try build already in progress");
-            repo.client
-                .post_comment(pr.number, Comment::new(":exclamation: A try build is currently in progress. You can cancel it using @bors try cancel.".to_string()))
-                .await?;
-            return Ok(());
-        }
-        Some(CommitSha(build.parent.clone()))
-    } else {
-        None
-    };
-
-    let base_sha = match parent.clone() {
-        Some(parent) => match parent {
-            Parent::Last => match last_parent {
-                None => {
-                    repo.client
-                        .post_comment(pr.number, Comment::new(":exclamation: There was no previous build. Please set an explicit parent or remove the `parent=last` argument to use the default parent.".to_string()))
-                        .await?;
-                    return Ok(());
-                }
-                Some(last_parent) => last_parent,
-            },
-            Parent::CommitSha(parent) => parent,
-        },
-        None => repo
+    let base_sha = match get_base_sha(&pr_model, parent) {
+        Ok(Some(base_sha)) => base_sha,
+        Ok(None) => repo
             .client
             .get_branch_sha(&pr.base.name)
             .await
-            .with_context(|| format!("Cannot get SHA for branch {}", pr.base.name))?,
+            .context(format!("Cannot get SHA for branch {}", pr.base.name))?,
+        Err(comment) => {
+            repo.client.post_comment(pr.number, comment).await?;
+            return Ok(());
+        }
     };
 
     tracing::debug!("Attempting to merge with base SHA {base_sha}");
@@ -140,6 +122,33 @@ pub(super) async fn command_try_build(
     }
 }
 
+fn get_base_sha(
+    pr_model: &PullRequestModel,
+    parent: Option<Parent>,
+) -> Result<Option<CommitSha>, Comment> {
+    let last_parent = if let Some(ref build) = pr_model.try_build {
+        if build.status == BuildStatus::Pending {
+            tracing::warn!("Try build already in progress");
+            return Err(try_build_in_progress_comment());
+        } else {
+            Some(CommitSha(build.parent.clone()))
+        }
+    } else {
+        None
+    };
+
+    match parent.clone() {
+        Some(parent) => match parent {
+            Parent::Last => match last_parent {
+                None => Err(cant_find_last_parent_comment()),
+                Some(last_parent) => Ok(Some(last_parent)),
+            },
+            Parent::CommitSha(parent) => Ok(Some(parent)),
+        },
+        None => Ok(None),
+    }
+}
+
 pub(super) async fn command_try_cancel(
     repo: Arc<RepositoryState>,
     db: Arc<PgDbClient>,

src/bors/mod.rs

@@ -1,7 +1,4 @@
-use std::collections::HashMap;
-
 use arc_swap::ArcSwap;
-use octocrab::Octocrab;
 
 pub use command::CommandParser;
 pub use comment::Comment;
@@ -12,36 +9,15 @@ pub use handlers::{handle_bors_global_event, handle_bors_repository_event};
 
 use crate::config::RepositoryConfig;
 use crate::github::api::client::GithubRepositoryClient;
-use crate::github::api::load_repositories;
 use crate::github::GithubRepoName;
 use crate::permissions::UserPermissions;
-use crate::TeamApiClient;
 
 mod command;
 pub mod comment;
 mod context;
 pub mod event;
 mod handlers;
 
-/// Loads repositories through a global GitHub client.
-pub struct RepositoryLoader {
-    client: Octocrab,
-}
-
-impl RepositoryLoader {
-    pub fn new(client: Octocrab) -> Self {
-        Self { client }
-    }
-
-    /// Load state of repositories.
-    pub async fn load_repositories(
-        &self,
-        team_api_client: &TeamApiClient,
-    ) -> anyhow::Result<HashMap<GithubRepoName, anyhow::Result<RepositoryState>>> {
-        load_repositories(&self.client, team_api_client).await
-    }
-}
-
 #[derive(Clone, Debug)]
 pub enum CheckSuiteStatus {
     Pending,

src/github/api/client.rs

@@ -286,12 +286,12 @@ impl GithubRepositoryClient {
 
 #[cfg(test)]
 mod tests {
+    use crate::github::api::load_repositories;
     use crate::github::GithubRepoName;
     use crate::permissions::PermissionType;
     use crate::tests::mocks::Permissions;
     use crate::tests::mocks::{ExternalHttpMock, Repo};
     use crate::tests::mocks::{User, World};
-    use crate::RepositoryLoader;
     use octocrab::models::UserId;
 
     #[tokio::test]
@@ -312,10 +312,7 @@ mod tests {
         .await;
         let client = mock.github_client();
         let team_api_client = mock.team_api_client();
-        let mut repos = RepositoryLoader::new(client)
-            .load_repositories(&team_api_client)
-            .await
-            .unwrap();
+        let mut repos = load_repositories(&client, &team_api_client).await.unwrap();
         assert_eq!(repos.len(), 2);
 
         let repo = repos

src/github/api/mod.rs

@@ -10,6 +10,7 @@ use secrecy::{ExposeSecret, SecretString};
 use client::GithubRepositoryClient;
 
 use crate::bors::RepositoryState;
+use crate::config::RepositoryConfig;
 use crate::github::GithubRepoName;
 use crate::permissions::TeamApiClient;
 
@@ -36,6 +37,9 @@ pub fn create_github_client(
 }
 
 /// Loads repositories that are connected to the given GitHub App client.
+/// The anyhow::Result<RepositoryState> is intended, because we wanted to have
+/// a hard error when the repos fail to load when the bot starts, but only log
+/// a warning when we reload the state during the bot's execution.
 pub async fn load_repositories(
     client: &Octocrab,
     team_api_client: &TeamApiClient,
@@ -152,21 +156,24 @@ async fn create_repo_state(
         .await
         .with_context(|| format!("Could not load permissions for repository {name}"))?;
 
-    let config = match client.load_config().await {
-        Ok(config) => {
-            tracing::info!("Loaded repository config for {name}: {config:#?}");
-            config
-        }
-        Err(error) => {
-            return Err(anyhow::anyhow!(
-                "Could not load repository config for {name}: {error:?}"
-            ));
-        }
-    };
+    let config = load_config(&client).await?;
 
     Ok(RepositoryState {
         client,
         config: ArcSwap::new(Arc::new(config)),
         permissions: ArcSwap::new(Arc::new(permissions)),
     })
 }
+
+async fn load_config(client: &GithubRepositoryClient) -> anyhow::Result<RepositoryConfig> {
+    let name = client.repository();
+    match client.load_config().await {
+        Ok(config) => {
+            tracing::info!("Loaded repository config for {name}: {config:#?}");
+            Ok(config)
+        }
+        Err(error) => Err(anyhow::anyhow!(
+            "Could not load repository config for {name}: {error:?}"
+        )),
+    }
+}

src/github/server.rs

@@ -2,7 +2,7 @@ use crate::bors::event::BorsEvent;
 use crate::bors::{handle_bors_global_event, handle_bors_repository_event, BorsContext};
 use crate::github::webhook::GitHubWebhook;
 use crate::github::webhook::WebhookSecret;
-use crate::{BorsGlobalEvent, BorsRepositoryEvent, RepositoryLoader, TeamApiClient};
+use crate::{BorsGlobalEvent, BorsRepositoryEvent, TeamApiClient};
 
 use anyhow::Error;
 use axum::extract::State;
@@ -92,7 +92,6 @@ pub fn create_bors_process(
 ) {
     let (repository_tx, repository_rx) = mpsc::channel::<BorsRepositoryEvent>(1024);
     let (global_tx, global_rx) = mpsc::channel::<BorsGlobalEvent>(1024);
-    let repo_loader = RepositoryLoader::new(gh_client);
 
     let service = async move {
         let ctx = Arc::new(ctx);
@@ -105,7 +104,7 @@ pub fn create_bors_process(
         {
             tokio::join!(
                 consume_repository_events(ctx.clone(), repository_rx),
-                consume_global_events(ctx.clone(), global_rx, repo_loader, team_api)
+                consume_global_events(ctx.clone(), global_rx, gh_client, team_api)
             );
         }
         // In real execution, the bot runs forever. If there is something that finishes
@@ -117,7 +116,7 @@ pub fn create_bors_process(
                 _ = consume_repository_events(ctx.clone(), repository_rx) => {
                     tracing::error!("Repository event handling process has ended");
                 }
-                _ = consume_global_events(ctx.clone(), global_rx, repo_loader, team_api) => {
+                _ = consume_global_events(ctx.clone(), global_rx, gh_client, team_api) => {
                     tracing::error!("Global event handling process has ended");
                 }
             }
@@ -147,15 +146,15 @@ async fn consume_repository_events(
 async fn consume_global_events(
     ctx: Arc<BorsContext>,
     mut global_rx: mpsc::Receiver<BorsGlobalEvent>,
-    loader: RepositoryLoader,
+    gh_client: Octocrab,
     team_api: TeamApiClient,
 ) {
     while let Some(event) = global_rx.recv().await {
         let ctx = ctx.clone();
 
         let span = tracing::info_span!("GlobalEvent");
         tracing::debug!("Received global event: {event:#?}");
-        if let Err(error) = handle_bors_global_event(event, ctx, &loader, &team_api)
+        if let Err(error) = handle_bors_global_event(event, ctx, &gh_client, &team_api)
             .instrument(span.clone())
             .await
         {

src/lib.rs

@@ -8,13 +8,11 @@ mod github;
 mod permissions;
 mod utils;
 
-pub use bors::{
-    event::BorsGlobalEvent, event::BorsRepositoryEvent, BorsContext, CommandParser,
-    RepositoryLoader,
-};
+pub use bors::{event::BorsGlobalEvent, event::BorsRepositoryEvent, BorsContext, CommandParser};
 pub use database::PgDbClient;
 pub use github::{
     api::create_github_client,
+    api::load_repositories,
     server::{create_app, create_bors_process, ServerState},
     WebhookSecret,
 };