Merge pull request #187 from vohoanglong0107/refactor-centralize-comments

refactor: centralize build and workflow comments

Author: Kobzol

src/bors/comment.rs

@@ -1,6 +1,9 @@
 use serde::Serialize;
 
-use crate::github::CommitSha;
+use crate::{
+    database::{WorkflowModel, WorkflowStatus},
+    github::CommitSha,
+};
 
 /// A comment that can be posted to a pull request.
 pub struct Comment {
@@ -34,13 +37,14 @@ impl Comment {
     }
 }
 
-pub fn try_build_succeeded_comment(workflow_list: String, commit_sha: CommitSha) -> Comment {
+pub fn try_build_succeeded_comment(workflows: &[WorkflowModel], commit_sha: CommitSha) -> Comment {
+    let workflows_status = list_workflows_status(workflows);
     Comment {
         text: format!(
             r#":sunny: Try build successful
 {}
 Build commit: {} (`{}`)"#,
-            workflow_list, commit_sha, commit_sha
+            workflows_status, commit_sha, commit_sha
         ),
         metadata: Some(CommentMetadata::TryBuildCompleted {
             merge_sha: commit_sha.to_string(),
@@ -55,3 +59,51 @@ pub fn try_build_in_progress_comment() -> Comment {
 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())
 }
+
+pub fn no_try_build_in_progress_comment() -> Comment {
+    Comment::new(":exclamation: There is currently no try build in progress.".to_string())
+}
+
+pub fn unclean_try_build_cancelled_comment() -> Comment {
+    Comment::new(
+        "Try build was cancelled. It was not possible to cancel some workflows.".to_string(),
+    )
+}
+
+pub fn try_build_cancelled_comment(workflow_urls: impl Iterator<Item = String>) -> Comment {
+    let mut try_build_cancelled_comment = r#"Try build cancelled.
+Cancelled workflows:"#
+        .to_string();
+    for url in workflow_urls {
+        try_build_cancelled_comment += format!("\n- {}", url).as_str();
+    }
+    Comment::new(try_build_cancelled_comment)
+}
+
+pub fn workflow_failed_comment(workflows: &[WorkflowModel]) -> Comment {
+    let workflows_status = list_workflows_status(workflows);
+    Comment::new(format!(
+        r#":broken_heart: Test failed
+{}"#,
+        workflows_status
+    ))
+}
+
+fn list_workflows_status(workflows: &[WorkflowModel]) -> String {
+    workflows
+        .iter()
+        .map(|w| {
+            format!(
+                "- [{}]({}) {}",
+                w.name,
+                w.url,
+                if w.status == WorkflowStatus::Success {
+                    ":white_check_mark:"
+                } else {
+                    ":x:"
+                }
+            )
+        })
+        .collect::<Vec<_>>()
+        .join("\n")
+}

src/bors/handlers/refresh.rs

@@ -40,7 +40,7 @@ async fn cancel_timed_out_builds(repo: &RepositoryState, db: &PgDbClient) -> any
             db.update_build_status(&build, BuildStatus::Cancelled)
                 .await?;
             if let Some(pr) = db.find_pr_by_build(&build).await? {
-                if let Err(error) = cancel_build_workflows(repo, db, &build).await {
+                if let Err(error) = cancel_build_workflows(&repo.client, db, &build).await {
                     tracing::error!(
                         "Could not cancel workflows for SHA {}: {error:?}",
                         build.commit_sha

src/bors/handlers/trybuild.rs

@@ -4,12 +4,16 @@ use anyhow::{anyhow, Context};
 
 use crate::bors::command::Parent;
 use crate::bors::comment::cant_find_last_parent_comment;
+use crate::bors::comment::no_try_build_in_progress_comment;
+use crate::bors::comment::try_build_cancelled_comment;
 use crate::bors::comment::try_build_in_progress_comment;
+use crate::bors::comment::unclean_try_build_cancelled_comment;
 use crate::bors::handlers::labels::handle_label_trigger;
 use crate::bors::Comment;
 use crate::bors::RepositoryState;
 use crate::database::RunId;
-use crate::database::{BuildModel, BuildStatus, PullRequestModel, WorkflowStatus, WorkflowType};
+use crate::database::{BuildModel, BuildStatus, PullRequestModel};
+use crate::github::api::client::GithubRepositoryClient;
 use crate::github::GithubRepoName;
 use crate::github::{
     CommitSha, GithubUser, LabelTrigger, MergeError, PullRequest, PullRequestNumber,
@@ -44,6 +48,8 @@ pub(super) async fn command_try_build(
         return Ok(());
     }
 
+    // Create pr model based on CI repo, so we can retrieve the pr later when
+    // the CI repo emits events
     let pr_model = db
         .get_or_create_pull_request(repo.client.repository(), pr.number)
         .await
@@ -62,66 +68,94 @@ pub(super) async fn command_try_build(
         }
     };
 
+    match attempt_merge(
+        &repo.client,
+        &pr.head.sha,
+        &base_sha,
+        &auto_merge_commit_message(pr, repo.client.repository(), "<try>", jobs),
+    )
+    .await?
+    {
+        MergeResult::Success(merge_sha) => {
+            // If the merge was succesful, run CI with merged commit
+            run_try_build(&repo.client, &db, pr_model, merge_sha.clone(), base_sha).await?;
+
+            handle_label_trigger(repo, pr.number, LabelTrigger::TryBuildStarted).await?;
+
+            repo.client
+                .post_comment(pr.number, trying_build_comment(&pr.head.sha, &merge_sha))
+                .await
+        }
+        MergeResult::Conflict => {
+            repo.client
+                .post_comment(pr.number, merge_conflict_comment(&pr.head.name))
+                .await
+        }
+    }
+}
+
+async fn attempt_merge(
+    client: &GithubRepositoryClient,
+    head_sha: &CommitSha,
+    base_sha: &CommitSha,
+    merge_message: &str,
+) -> anyhow::Result<MergeResult> {
     tracing::debug!("Attempting to merge with base SHA {base_sha}");
 
     // First set the try branch to our base commit (either the selected parent or the main branch).
-    repo.client
-        .set_branch_to_sha(TRY_MERGE_BRANCH_NAME, &base_sha)
+    client
+        .set_branch_to_sha(TRY_MERGE_BRANCH_NAME, base_sha)
         .await
         .map_err(|error| anyhow!("Cannot set try merge branch to {}: {error:?}", base_sha.0))?;
 
     // Then merge the PR commit into the try branch
-    match repo
-        .client
-        .merge_branches(
-            TRY_MERGE_BRANCH_NAME,
-            &pr.head.sha,
-            &auto_merge_commit_message(pr, repo.client.repository(), "<try>", jobs),
-        )
+    match client
+        .merge_branches(TRY_MERGE_BRANCH_NAME, head_sha, merge_message)
         .await
     {
         Ok(merge_sha) => {
             tracing::debug!("Merge successful, SHA: {merge_sha}");
-            // If the merge was succesful, then set the actual try branch that will run CI to the
-            // merged commit.
-            repo.client
-                .set_branch_to_sha(TRY_BRANCH_NAME, &merge_sha)
-                .await
-                .map_err(|error| anyhow!("Cannot set try branch to main branch: {error:?}"))?;
-
-            db.attach_try_build(
-                pr_model,
-                TRY_BRANCH_NAME.to_string(),
-                merge_sha.clone(),
-                base_sha.clone(),
-            )
-            .await?;
-            tracing::info!("Try build started");
-
-            handle_label_trigger(repo, pr.number, LabelTrigger::TryBuildStarted).await?;
 
-            let comment = Comment::new(format!(
-                ":hourglass: Trying commit {} with merge {}…",
-                pr.head.sha.clone(),
-                merge_sha
-            ));
-            repo.client.post_comment(pr.number, comment).await?;
-            Ok(())
+            Ok(MergeResult::Success(merge_sha))
         }
         Err(MergeError::Conflict) => {
             tracing::warn!("Merge conflict");
-            repo.client
-                .post_comment(
-                    pr.number,
-                    Comment::new(merge_conflict_message(&pr.head.name)),
-                )
-                .await?;
-            Ok(())
+
+            Ok(MergeResult::Conflict)
         }
         Err(error) => Err(error.into()),
     }
 }
 
+async fn run_try_build(
+    client: &GithubRepositoryClient,
+    db: &PgDbClient,
+    pr_model: PullRequestModel,
+    commit_sha: CommitSha,
+    parent_sha: CommitSha,
+) -> anyhow::Result<()> {
+    client
+        .set_branch_to_sha(TRY_BRANCH_NAME, &commit_sha)
+        .await
+        .map_err(|error| anyhow!("Cannot set try branch to main branch: {error:?}"))?;
+
+    db.attach_try_build(
+        pr_model,
+        TRY_BRANCH_NAME.to_string(),
+        commit_sha,
+        parent_sha,
+    )
+    .await?;
+
+    tracing::info!("Try build started");
+    Ok(())
+}
+
+enum MergeResult {
+    Success(CommitSha),
+    Conflict,
+}
+
 fn get_base_sha(
     pr_model: &PullRequestModel,
     parent: Option<Parent>,
@@ -168,17 +202,12 @@ pub(super) async fn command_try_cancel(
     let Some(build) = get_pending_build(pr) else {
         tracing::warn!("No build found");
         repo.client
-            .post_comment(
-                pr_number,
-                Comment::new(
-                    ":exclamation: There is currently no try build in progress.".to_string(),
-                ),
-            )
+            .post_comment(pr_number, no_try_build_in_progress_comment())
             .await?;
         return Ok(());
     };
 
-    match cancel_build_workflows(repo, db.as_ref(), &build).await {
+    match cancel_build_workflows(&repo.client, db.as_ref(), &build).await {
         Err(error) => {
             tracing::error!(
                 "Could not cancel workflows for SHA {}: {error:?}",
@@ -187,30 +216,21 @@ pub(super) async fn command_try_cancel(
             db.update_build_status(&build, BuildStatus::Cancelled)
                 .await?;
             repo.client
-                .post_comment(
-                    pr_number,
-                    Comment::new(
-                        "Try build was cancelled. It was not possible to cancel some workflows."
-                            .to_string(),
-                    ),
-                )
+                .post_comment(pr_number, unclean_try_build_cancelled_comment())
                 .await?
         }
         Ok(workflow_ids) => {
             db.update_build_status(&build, BuildStatus::Cancelled)
                 .await?;
             tracing::info!("Try build cancelled");
 
-            let mut try_build_cancelled_comment = r#"Try build cancelled.
-Cancelled workflows:"#
-                .to_string();
-            for id in workflow_ids {
-                let url = repo.client.get_workflow_url(id);
-                try_build_cancelled_comment += format!("\n- {}", url).as_str();
-            }
-
             repo.client
-                .post_comment(pr_number, Comment::new(try_build_cancelled_comment))
+                .post_comment(
+                    pr_number,
+                    try_build_cancelled_comment(
+                        repo.client.get_workflow_urls(workflow_ids.into_iter()),
+                    ),
+                )
                 .await?
         }
     };
@@ -219,20 +239,14 @@ Cancelled workflows:"#
 }
 
 pub async fn cancel_build_workflows(
-    repo: &RepositoryState,
+    client: &GithubRepositoryClient,
     db: &PgDbClient,
     build: &BuildModel,
 ) -> anyhow::Result<Vec<RunId>> {
-    let pending_workflows = db
-        .get_workflows_for_build(build)
-        .await?
-        .into_iter()
-        .filter(|w| w.status == WorkflowStatus::Pending && w.workflow_type == WorkflowType::Github)
-        .map(|w| w.run_id)
-        .collect::<Vec<_>>();
+    let pending_workflows = db.get_pending_workflows_for_build(build).await?;
 
     tracing::info!("Cancelling workflows {:?}", pending_workflows);
-    repo.client.cancel_workflows(&pending_workflows).await?;
+    client.cancel_workflows(&pending_workflows).await?;
     Ok(pending_workflows)
 }
 
@@ -267,8 +281,14 @@ fn auto_merge_commit_message(
     message
 }
 
-fn merge_conflict_message(branch: &str) -> String {
-    format!(
+fn trying_build_comment(head_sha: &CommitSha, merge_sha: &CommitSha) -> Comment {
+    Comment::new(format!(
+        ":hourglass: Trying commit {head_sha} with merge {merge_sha}…"
+    ))
+}
+
+fn merge_conflict_comment(branch: &str) -> Comment {
+    let message = format!(
         r#":lock: Merge conflict
 
 This pull request and the master branch diverged in a way that cannot
@@ -298,7 +318,8 @@ handled during merge and rebase. This is normal, and you should still perform st
 
 </details>
 "#
-    )
+    );
+    Comment::new(message)
 }
 
 async fn check_try_permissions(