Merge pull request #347 from Kobzol/checks-reenable

Re-enable PR checks

Author: Kobzol

.sqlx/query-33cf1b803a0b658f197ab26d87e2c6c4fe42128cb481a234372626f35392e809.json

@@ -3,6 +3,7 @@ use std::time::Duration;
 
 use anyhow::Context;
 use chrono::{DateTime, Utc};
+use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus};
 use std::collections::BTreeMap;
 
 use crate::bors::Comment;
@@ -28,6 +29,22 @@ pub async fn cancel_timed_out_builds(
 
             db.update_build_status(&build, BuildStatus::Cancelled)
                 .await?;
+
+            if let Some(check_run_id) = build.check_run_id {
+                if let Err(error) = repo
+                    .client
+                    .update_check_run(
+                        check_run_id as u64,
+                        CheckRunStatus::Completed,
+                        Some(CheckRunConclusion::TimedOut),
+                        None,
+                    )
+                    .await
+                {
+                    tracing::error!("Could not update check run {check_run_id}: {error:?}");
+                }
+            }
+
             if let Some(pr) = db.find_pr_by_build(&build).await? {
                 if let Err(error) = cancel_build_workflows(&repo.client, db, &build).await {
                     tracing::error!(
@@ -181,11 +198,13 @@ fn elapsed_time(date: DateTime<Utc>) -> Duration {
 mod tests {
     use crate::bors::PullRequestStatus;
     use crate::bors::handlers::refresh::MOCK_TIME;
+    use crate::bors::handlers::trybuild::TRY_BUILD_CHECK_RUN_NAME;
     use crate::database::{MergeableState, OctocrabMergeableState};
     use crate::tests::mocks::{
         BorsBuilder, GitHubState, default_pr_number, default_repo_name, run_test,
     };
     use chrono::Utc;
+    use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus};
     use std::future::Future;
     use std::time::Duration;
     use tokio::runtime::RuntimeFlavor;
@@ -267,6 +286,33 @@ timeout = 3600
             .await;
     }
 
+    #[sqlx::test]
+    async fn refresh_cancel_build_updates_check_run(pool: sqlx::PgPool) {
+        BorsBuilder::new(pool)
+            .github(gh_state_with_long_timeout())
+            .run_test(|mut tester| async move {
+                tester.post_comment("@bors try").await?;
+                tester.expect_comments(1).await;
+
+                with_mocked_time(Duration::from_secs(4000), async {
+                    tester.cancel_timed_out_builds().await;
+                })
+                .await;
+                tester.expect_comments(1).await;
+
+                tester.expect_check_run(
+                    &tester.default_pr().await.get_gh_pr().head_sha,
+                    TRY_BUILD_CHECK_RUN_NAME,
+                    "Bors try build",
+                    CheckRunStatus::Completed,
+                    Some(CheckRunConclusion::TimedOut),
+                );
+
+                Ok(tester)
+            })
+            .await;
+    }
+
     #[sqlx::test]
     async fn refresh_cancel_workflow_after_timeout(pool: sqlx::PgPool) {
         let gh = BorsBuilder::new(pool)

src/bors/handlers/refresh.rs

@@ -1,7 +1,5 @@
 use std::sync::Arc;
 
-use anyhow::{Context, anyhow};
-
 use crate::PgDbClient;
 use crate::bors::RepositoryState;
 use crate::bors::command::Parent;
@@ -20,6 +18,10 @@ use crate::github::api::operations::ForcePush;
 use crate::github::{CommitSha, GithubUser, LabelTrigger, MergeError, PullRequestNumber};
 use crate::permissions::PermissionType;
 use crate::utils::text::suppress_github_mentions;
+use anyhow::{Context, anyhow};
+use octocrab::params::checks::CheckRunOutput;
+use octocrab::params::checks::CheckRunStatus;
+use tracing::log;
 
 use super::has_permission;
 use super::{PullRequestData, deny_request};
@@ -33,6 +35,9 @@ pub(super) const TRY_MERGE_BRANCH_NAME: &str = "automation/bors/try-merge";
 // This branch should run CI checks.
 pub(super) const TRY_BRANCH_NAME: &str = "automation/bors/try";
 
+// The name of the check run seen in the GitHub UI.
+pub(super) const TRY_BUILD_CHECK_RUN_NAME: &str = "Bors try build";
+
 /// Performs a so-called try build - merges the PR branch into a special branch designed
 /// for running CI checks.
 ///
@@ -87,10 +92,41 @@ pub(super) async fn command_try_build(
     {
         MergeResult::Success(merge_sha) => {
             // If the merge was succesful, run CI with merged commit
-            run_try_build(&repo.client, &db, &pr.db, merge_sha.clone(), base_sha).await?;
+            let build_id =
+                run_try_build(&repo.client, &db, &pr.db, merge_sha.clone(), base_sha).await?;
 
             handle_label_trigger(repo, pr.number(), LabelTrigger::TryBuildStarted).await?;
 
+            // Create a check run to track the try build status in GitHub's UI.
+            // This gets added to the PR's head SHA so GitHub shows UI in the checks tab and
+            // the bottom of the PR.
+            match repo
+                .client
+                .create_check_run(
+                    TRY_BUILD_CHECK_RUN_NAME,
+                    &pr.github.head.sha,
+                    CheckRunStatus::InProgress,
+                    CheckRunOutput {
+                        title: "Bors try build".to_string(),
+                        summary: "".to_string(),
+                        text: None,
+                        annotations: vec![],
+                        images: vec![],
+                    },
+                    &build_id.to_string(),
+                )
+                .await
+            {
+                Ok(check_run) => {
+                    db.update_build_check_run_id(build_id, check_run.id.into_inner() as i64)
+                        .await?;
+                }
+                Err(error) => {
+                    // Check runs aren't critical, don't block progress if they fail
+                    log::error!("Cannot create check run: {error:?}");
+                }
+            }
+
             repo.client
                 .post_comment(
                     pr.number(),
@@ -176,17 +212,18 @@ async fn run_try_build(
     pr: &PullRequestModel,
     commit_sha: CommitSha,
     parent_sha: CommitSha,
-) -> anyhow::Result<()> {
+) -> anyhow::Result<i32> {
     client
         .set_branch_to_sha(TRY_BRANCH_NAME, &commit_sha, ForcePush::Yes)
         .await
         .map_err(|error| anyhow!("Cannot set try branch to main branch: {error:?}"))?;
 
-    db.attach_try_build(pr, TRY_BRANCH_NAME.to_string(), commit_sha, parent_sha)
+    let build_id = db
+        .attach_try_build(pr, TRY_BRANCH_NAME.to_string(), commit_sha, parent_sha)
         .await?;
 
     tracing::info!("Try build started");
-    Ok(())
+    Ok(build_id)
 }
 
 enum MergeResult {
@@ -307,13 +344,16 @@ fn auto_merge_commit_message(
 
 #[cfg(test)]
 mod tests {
-    use crate::bors::handlers::trybuild::{TRY_BRANCH_NAME, TRY_MERGE_BRANCH_NAME};
+    use crate::bors::handlers::trybuild::{
+        TRY_BRANCH_NAME, TRY_BUILD_CHECK_RUN_NAME, TRY_MERGE_BRANCH_NAME,
+    };
     use crate::database::operations::get_all_workflows;
     use crate::github::CommitSha;
     use crate::tests::mocks::{
         BorsBuilder, Comment, GitHubState, User, Workflow, WorkflowEvent, default_pr_number,
         default_repo_name, run_test,
     };
+    use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus};
 
     #[sqlx::test]
     async fn try_success(pool: sqlx::PgPool) {
@@ -861,4 +901,70 @@ try_failed = ["+foo", "+bar", "-baz"]
             })
             .await;
     }
+
+    #[sqlx::test]
+    async fn try_build_creates_check_run(pool: sqlx::PgPool) {
+        run_test(pool.clone(), |mut tester| async {
+            tester.create_branch(TRY_BRANCH_NAME).expect_suites(1);
+            tester.post_comment("@bors try").await?;
+            tester.expect_comments(1).await;
+
+            tester.expect_check_run(
+                &tester.default_pr().await.get_gh_pr().head_sha,
+                TRY_BUILD_CHECK_RUN_NAME,
+                "Bors try build",
+                CheckRunStatus::InProgress,
+                None,
+            );
+
+            Ok(tester)
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn try_build_updates_check_run_on_success(pool: sqlx::PgPool) {
+        run_test(pool.clone(), |mut tester| async {
+            tester.create_branch(TRY_BRANCH_NAME).expect_suites(1);
+            tester.post_comment("@bors try").await?;
+            tester.expect_comments(1).await;
+
+            tester.workflow_success(tester.try_branch()).await?;
+            tester.expect_comments(1).await;
+
+            tester.expect_check_run(
+                &tester.default_pr().await.get_gh_pr().head_sha,
+                TRY_BUILD_CHECK_RUN_NAME,
+                "Bors try build",
+                CheckRunStatus::Completed,
+                Some(CheckRunConclusion::Success),
+            );
+
+            Ok(tester)
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn try_build_updates_check_run_on_failure(pool: sqlx::PgPool) {
+        run_test(pool.clone(), |mut tester| async {
+            tester.create_branch(TRY_BRANCH_NAME).expect_suites(1);
+            tester.post_comment("@bors try").await?;
+            tester.expect_comments(1).await;
+
+            tester.workflow_failure(tester.try_branch()).await?;
+            tester.expect_comments(1).await;
+
+            tester.expect_check_run(
+                &tester.default_pr().await.get_gh_pr().head_sha,
+                TRY_BUILD_CHECK_RUN_NAME,
+                "Bors try build",
+                CheckRunStatus::Completed,
+                Some(CheckRunConclusion::Failure),
+            );
+
+            Ok(tester)
+        })
+        .await;
+    }
 }

src/bors/handlers/trybuild.rs

@@ -1,6 +1,9 @@
 use std::sync::Arc;
 use std::time::Duration;
 
+use octocrab::params::checks::CheckRunConclusion;
+use octocrab::params::checks::CheckRunStatus;
+
 use crate::PgDbClient;
 use crate::bors::CheckSuiteStatus;
 use crate::bors::RepositoryState;
@@ -193,6 +196,18 @@ async fn try_complete_build(
 
     handle_label_trigger(repo, pr.number, trigger).await?;
 
+    if let Some(check_run_id) = build.check_run_id {
+        let (status, conclusion) = if has_failure {
+            (CheckRunStatus::Completed, Some(CheckRunConclusion::Failure))
+        } else {
+            (CheckRunStatus::Completed, Some(CheckRunConclusion::Success))
+        };
+
+        repo.client
+            .update_check_run(check_run_id as u64, status, conclusion, None)
+            .await?;
+    }
+
     let message = if !has_failure {
         tracing::info!("Workflow succeeded");
         try_build_succeeded_comment(&workflows, payload.commit_sha, &build)

src/bors/handlers/workflow.rs

@@ -15,9 +15,9 @@ use super::operations::{
     get_prs_with_unknown_mergeable_state, get_pull_request, get_repository, get_repository_by_name,
     get_running_builds, get_workflow_urls_for_build, get_workflows_for_build,
     insert_repo_if_not_exists, set_pr_assignees, set_pr_priority, set_pr_rollup, set_pr_status,
-    unapprove_pull_request, undelegate_pull_request, update_build_status,
-    update_mergeable_states_by_base_branch, update_pr_mergeable_state, update_pr_try_build_id,
-    update_workflow_status, upsert_pull_request, upsert_repository,
+    unapprove_pull_request, undelegate_pull_request, update_build_check_run_id,
+    update_build_status, update_mergeable_states_by_base_branch, update_pr_mergeable_state,
+    update_pr_try_build_id, update_workflow_status, upsert_pull_request, upsert_repository,
 };
 use super::{ApprovalInfo, DelegatedPermission, MergeableState, RunId, UpsertPullRequestParams};
 
@@ -181,13 +181,13 @@ impl PgDbClient {
         branch: String,
         commit_sha: CommitSha,
         parent: CommitSha,
-    ) -> anyhow::Result<()> {
+    ) -> anyhow::Result<i32> {
         let mut tx = self.pool.begin().await?;
         let build_id =
             create_build(&mut *tx, &pr.repository, &branch, &commit_sha, &parent).await?;
         update_pr_try_build_id(&mut *tx, pr.id, build_id).await?;
         tx.commit().await?;
-        Ok(())
+        Ok(build_id)
     }
 
     pub async fn find_build(
@@ -214,6 +214,14 @@ impl PgDbClient {
         update_build_status(&self.pool, build.id, status).await
     }
 
+    pub async fn update_build_check_run_id(
+        &self,
+        build_id: i32,
+        check_run_id: i64,
+    ) -> anyhow::Result<()> {
+        update_build_check_run_id(&self.pool, build_id, check_run_id).await
+    }
+
     pub async fn create_workflow(
         &self,
         build: &BuildModel,

src/database/client.rs

@@ -315,6 +315,7 @@ pub struct BuildModel {
     /// The base commit SHA that this build is merged with (e.g., main branch HEAD).
     pub parent: String,
     pub created_at: DateTime<Utc>,
+    /// The ID of the check run associated with the build.
     pub check_run_id: Option<i64>,
 }
 

src/database/mod.rs

@@ -946,6 +946,24 @@ pub(crate) async fn upsert_repository(
     .await
 }
 
+pub(crate) async fn update_build_check_run_id(
+    executor: impl PgExecutor<'_>,
+    build_id: i32,
+    check_run_id: i64,
+) -> anyhow::Result<()> {
+    measure_db_query("update_build_check_run_id", || async {
+        sqlx::query!(
+            "UPDATE build SET check_run_id = $1 WHERE id = $2",
+            check_run_id,
+            build_id
+        )
+        .execute(executor)
+        .await?;
+        Ok(())
+    })
+    .await
+}
+
 /// Removes the auto build associated with a PR and deletes the build record (if one exists).
 pub(crate) async fn delete_auto_build(
     executor: impl PgExecutor<'_>,