From 528bf7d8e28a420b071f2011e0e0f1101cd80905 Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Fri, 18 Jul 2025 17:23:15 +0100 Subject: [PATCH 1/3] Start auto builds --- ...1134df3ac7818af0c085ac432eb52874a62a2.json | 15 + src/bors/comment.rs | 7 + src/bors/merge_queue.rs | 293 +++++++++++++++++- src/bors/mod.rs | 3 + src/database/client.rs | 17 +- src/database/operations.rs | 18 ++ src/tests/mocks/bors.rs | 32 +- src/tests/mocks/mod.rs | 2 +- 8 files changed, 380 insertions(+), 7 deletions(-) create mode 100644 .sqlx/query-3ae0ba7ce98f0a68b47df52a9181134df3ac7818af0c085ac432eb52874a62a2.json diff --git a/.sqlx/query-3ae0ba7ce98f0a68b47df52a9181134df3ac7818af0c085ac432eb52874a62a2.json b/.sqlx/query-3ae0ba7ce98f0a68b47df52a9181134df3ac7818af0c085ac432eb52874a62a2.json new file mode 100644 index 00000000..d4166e22 --- /dev/null +++ b/.sqlx/query-3ae0ba7ce98f0a68b47df52a9181134df3ac7818af0c085ac432eb52874a62a2.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET auto_build_id = $1 WHERE id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4", + "Int4" + ] + }, + "nullable": [] + }, + "hash": "3ae0ba7ce98f0a68b47df52a9181134df3ac7818af0c085ac432eb52874a62a2" +} diff --git a/src/bors/comment.rs b/src/bors/comment.rs index 0951cf1c..3ff61d63 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -303,3 +303,10 @@ fn list_workflows_status(workflows: &[WorkflowModel]) -> String { .collect::>() .join("\n") } + +pub fn auto_build_started_comment(head_sha: &CommitSha, merge_sha: &CommitSha) -> Comment { + Comment::new(format!( + ":hourglass: Testing commit {} with merge {}...", + head_sha, merge_sha + )) +} diff --git a/src/bors/merge_queue.rs b/src/bors/merge_queue.rs index 88ccf341..52b96029 100644 --- a/src/bors/merge_queue.rs +++ b/src/bors/merge_queue.rs @@ -1,3 +1,5 @@ +use anyhow::anyhow; +use octocrab::params::checks::{CheckRunOutput, CheckRunStatus}; use std::future::Future; use std::sync::Arc; use tokio::sync::mpsc; @@ -5,8 +7,18 @@ use tracing::Instrument; use crate::BorsContext; use crate::bors::RepositoryState; +use crate::bors::comment::auto_build_started_comment; +use crate::database::{BuildStatus, PullRequestModel}; +use crate::github::api::client::GithubRepositoryClient; +use crate::github::api::operations::ForcePush; +use crate::github::{CommitSha, MergeError}; use crate::utils::sort_queue::sort_queue_prs; +enum MergeResult { + Success(CommitSha), + Conflict, +} + #[derive(Debug)] enum MergeQueueEvent { Trigger, @@ -31,10 +43,17 @@ impl MergeQueueSender { } } +/// Branch used for performing merge operations. +/// This branch should not run CI checks. +pub(super) const AUTO_MERGE_BRANCH_NAME: &str = "automation/bors/auto-merge"; + /// Branch where CI checks run for auto builds. /// This branch should run CI checks. pub(super) const AUTO_BRANCH_NAME: &str = "automation/bors/auto"; +// The name of the check run seen in the GitHub UI. +pub(super) const AUTO_BUILD_CHECK_RUN_NAME: &str = "Bors auto build"; + pub async fn merge_queue_tick(ctx: Arc) -> anyhow::Result<()> { let repos: Vec> = ctx.repositories.read().unwrap().values().cloned().collect(); @@ -61,14 +80,173 @@ pub async fn merge_queue_tick(ctx: Arc) -> anyhow::Result<()> { // then pending builds (which block the queue to prevent starting simultaneous auto-builds). let prs = sort_queue_prs(prs); - for _ in prs { - // Process PRs... + for pr in prs { + let pr_num = pr.number; + + if let Some(auto_build) = &pr.auto_build { + match auto_build.status { + // Build successful - point the base branch to the merged commit. + BuildStatus::Success => { + // Break to give GitHub time to update the base branch. + break; + } + // Build in progress - stop queue. We can only have one PR being built + // at a time. + BuildStatus::Pending => { + tracing::info!("PR {pr_num} has a pending build - blocking queue"); + break; + } + BuildStatus::Failure | BuildStatus::Cancelled | BuildStatus::Timeouted => { + unreachable!("Failed auto builds should be filtered out by SQL query"); + } + } + } + + // No build exists for this PR - start a new auto build. + match start_auto_build(&repo, &ctx, pr).await { + Ok(true) => { + tracing::info!("Starting auto build for PR {pr_num}"); + break; + } + Ok(false) => { + tracing::debug!("Failed to start auto build for PR {pr_num}"); + continue; + } + Err(error) => { + // Unexpected error - the PR will remain in the "queue" for a retry. + tracing::error!("Error starting auto build for PR {pr_num}: {:?}", error); + continue; + } + } } } + #[cfg(test)] + crate::bors::WAIT_FOR_MERGE_QUEUE.mark(); + Ok(()) } +/// Starts a new auto build for a pull request. +async fn start_auto_build( + repo: &Arc, + ctx: &Arc, + pr: PullRequestModel, +) -> anyhow::Result { + let client = &repo.client; + + let gh_pr = client.get_pull_request(pr.number).await?; + let base_sha = client.get_branch_sha(&pr.base_branch).await?; + + let auto_merge_commit_message = format!( + "Auto merge of #{} - {}, r={}\n\n{}\n\n{}", + pr.number, + gh_pr.head_label, + pr.approver().unwrap_or(""), + pr.title, + gh_pr.message + ); + + // 1. Merge PR head with base branch on `AUTO_MERGE_BRANCH_NAME` + match attempt_merge( + &repo.client, + &gh_pr.head.sha, + &base_sha, + &auto_merge_commit_message, + ) + .await? + { + MergeResult::Success(merge_sha) => { + // 2. Push merge commit to `AUTO_BRANCH_NAME` where CI runs + client + .set_branch_to_sha(AUTO_BRANCH_NAME, &merge_sha, ForcePush::Yes) + .await?; + + // 3. Record the build in the database + let build_id = ctx + .db + .attach_auto_build( + &pr, + AUTO_BRANCH_NAME.to_string(), + merge_sha.clone(), + base_sha, + ) + .await?; + + // 4. Set GitHub check run to pending on PR head + match client + .create_check_run( + AUTO_BUILD_CHECK_RUN_NAME, + &gh_pr.head.sha, + CheckRunStatus::InProgress, + CheckRunOutput { + title: AUTO_BUILD_CHECK_RUN_NAME.to_string(), + summary: "".to_string(), + text: None, + annotations: vec![], + images: vec![], + }, + &build_id.to_string(), + ) + .await + { + Ok(check_run) => { + tracing::info!( + "Created check run {} for build {build_id}", + check_run.id.into_inner() + ); + ctx.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 + tracing::error!("Cannot create check run: {error:?}"); + } + } + + // 5. Post status comment + let comment = auto_build_started_comment(&gh_pr.head.sha, &merge_sha); + client.post_comment(pr.number, comment).await?; + + Ok(true) + } + MergeResult::Conflict => Ok(false), + } +} + +/// Attempts to merge the given head SHA with base SHA via `AUTO_MERGE_BRANCH_NAME`. +async fn attempt_merge( + client: &GithubRepositoryClient, + head_sha: &CommitSha, + base_sha: &CommitSha, + merge_message: &str, +) -> anyhow::Result { + tracing::debug!("Attempting to merge with base SHA {base_sha}"); + + // Reset auto merge branch to point to base branch + client + .set_branch_to_sha(AUTO_MERGE_BRANCH_NAME, base_sha, ForcePush::Yes) + .await + .map_err(|error| anyhow!("Cannot set auto merge branch to {}: {error:?}", base_sha.0))?; + + // then merge PR head commit into auto merge branch. + match client + .merge_branches(AUTO_MERGE_BRANCH_NAME, head_sha, merge_message) + .await + { + Ok(merge_sha) => { + tracing::debug!("Merge successful, SHA: {merge_sha}"); + Ok(MergeResult::Success(merge_sha)) + } + Err(MergeError::Conflict) => { + tracing::warn!("Merge conflict"); + Ok(MergeResult::Conflict) + } + Err(error) => Err(error.into()), + } +} + pub fn start_merge_queue(ctx: Arc) -> (MergeQueueSender, impl Future) { let (tx, mut rx) = mpsc::channel::(10); let sender = MergeQueueSender { inner: tx }; @@ -103,3 +281,114 @@ pub fn start_merge_queue(ctx: Arc) -> (MergeQueueSender, impl Futur (sender, fut) } + +#[cfg(test)] +mod tests { + + use octocrab::params::checks::CheckRunStatus; + use sqlx::PgPool; + + use crate::{ + bors::merge_queue::{AUTO_BRANCH_NAME, AUTO_BUILD_CHECK_RUN_NAME}, + database::{WorkflowStatus, operations::get_all_workflows}, + github::CommitSha, + tests::{ + BorsTester, + mocks::{BorsBuilder, GitHubState, WorkflowEvent, default_repo_name}, + }, + }; + + fn gh_state_with_merge_queue() -> GitHubState { + GitHubState::default().with_default_config( + r#" + merge_queue_enabled = true + "#, + ) + } + + pub async fn run_merge_queue_test anyhow::Result<()>>( + pool: PgPool, + f: F, + ) -> GitHubState { + BorsBuilder::new(pool) + .github(gh_state_with_merge_queue()) + .run_test(f) + .await + } + + async fn start_auto_build(tester: &mut BorsTester) -> anyhow::Result<()> { + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + tester.process_merge_queue().await; + tester.expect_comments(1).await; + Ok(()) + } + + #[sqlx::test] + async fn auto_workflow_started(pool: sqlx::PgPool) { + run_merge_queue_test(pool.clone(), async |mut tester| { + start_auto_build(&mut tester).await?; + tester + .workflow_event(WorkflowEvent::started(tester.auto_branch())) + .await?; + Ok(()) + }) + .await; + + let suite = get_all_workflows(&pool).await.unwrap().pop().unwrap(); + assert_eq!(suite.status, WorkflowStatus::Pending); + } + + #[sqlx::test] + async fn auto_workflow_check_run_created(pool: sqlx::PgPool) { + run_merge_queue_test(pool, async |mut tester| { + start_auto_build(&mut tester).await?; + tester.expect_check_run( + &tester.default_pr().await.get_gh_pr().head_sha, + AUTO_BUILD_CHECK_RUN_NAME, + AUTO_BUILD_CHECK_RUN_NAME, + CheckRunStatus::InProgress, + None, + ); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_started_comment(pool: sqlx::PgPool) { + run_merge_queue_test(pool, async |tester| { + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + tester.process_merge_queue().await; + insta::assert_snapshot!( + tester.get_comment().await?, + @":hourglass: Testing commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0..." + ); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_insert_into_db(pool: sqlx::PgPool) { + run_merge_queue_test(pool, async |mut tester| { + start_auto_build(&mut tester).await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + assert!( + tester + .db() + .find_build( + &default_repo_name(), + AUTO_BRANCH_NAME.to_string(), + CommitSha(tester.auto_branch().get_sha().to_string()), + ) + .await? + .is_some() + ); + Ok(()) + }) + .await; + } +} diff --git a/src/bors/mod.rs b/src/bors/mod.rs index 165006cc..978635ab 100644 --- a/src/bors/mod.rs +++ b/src/bors/mod.rs @@ -41,6 +41,9 @@ pub static WAIT_FOR_PR_STATUS_REFRESH: TestSyncMarker = TestSyncMarker::new(); #[cfg(test)] pub static WAIT_FOR_WORKFLOW_STARTED: TestSyncMarker = TestSyncMarker::new(); +#[cfg(test)] +pub static WAIT_FOR_MERGE_QUEUE: TestSyncMarker = TestSyncMarker::new(); + /// Corresponds to a single execution of a workflow. #[derive(Clone, Debug)] pub struct WorkflowRun { diff --git a/src/database/client.rs b/src/database/client.rs index 3c3d7e98..890ad89c 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -1,7 +1,7 @@ use sqlx::PgPool; use crate::bors::{PullRequestStatus, RollupMode}; -use crate::database::operations::get_merge_queue_prs; +use crate::database::operations::{get_merge_queue_prs, update_pr_auto_build_id}; use crate::database::{ BuildModel, BuildStatus, PullRequestModel, RepoModel, TreeState, WorkflowModel, WorkflowStatus, WorkflowType, @@ -191,6 +191,21 @@ impl PgDbClient { Ok(build_id) } + pub async fn attach_auto_build( + &self, + pr: &PullRequestModel, + branch: String, + commit_sha: CommitSha, + parent: CommitSha, + ) -> anyhow::Result { + let mut tx = self.pool.begin().await?; + let build_id = + create_build(&mut *tx, &pr.repository, &branch, &commit_sha, &parent).await?; + update_pr_auto_build_id(&mut *tx, pr.id, build_id).await?; + tx.commit().await?; + Ok(build_id) + } + pub async fn find_build( &self, repo: &GithubRepoName, diff --git a/src/database/operations.rs b/src/database/operations.rs index ce10d361..1095c415 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -515,6 +515,24 @@ pub(crate) async fn update_pr_try_build_id( .await } +pub(crate) async fn update_pr_auto_build_id( + executor: impl PgExecutor<'_>, + pr_id: i32, + auto_build_id: i32, +) -> anyhow::Result<()> { + measure_db_query("update_pr_auto_build_id", || async { + sqlx::query!( + "UPDATE pull_request SET auto_build_id = $1 WHERE id = $2", + auto_build_id, + pr_id + ) + .execute(executor) + .await?; + Ok(()) + }) + .await +} + pub(crate) async fn create_build( executor: impl PgExecutor<'_>, repo: &GithubRepoName, diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs index f885425a..34c37f4d 100644 --- a/src/tests/mocks/bors.rs +++ b/src/tests/mocks/bors.rs @@ -19,7 +19,7 @@ use super::repository::PullRequest; use crate::bors::merge_queue::MergeQueueSender; use crate::bors::mergeable_queue::MergeableQueueSender; use crate::bors::{ - CommandPrefix, RollupMode, WAIT_FOR_CANCEL_TIMED_OUT_BUILDS_REFRESH, + CommandPrefix, RollupMode, WAIT_FOR_CANCEL_TIMED_OUT_BUILDS_REFRESH, WAIT_FOR_MERGE_QUEUE, WAIT_FOR_MERGEABILITY_STATUS_REFRESH, WAIT_FOR_PR_STATUS_REFRESH, WAIT_FOR_WORKFLOW_STARTED, }; use crate::database::{ @@ -41,7 +41,7 @@ use crate::tests::mocks::{ use crate::tests::util::TestSyncMarker; use crate::tests::webhook::{TEST_WEBHOOK_SECRET, create_webhook_request}; use crate::{ - BorsContext, BorsGlobalEvent, CommandParser, PgDbClient, ServerState, WebhookSecret, + BorsContext, BorsGlobalEvent, CommandParser, PgDbClient, ServerState, TreeState, WebhookSecret, create_app, create_bors_process, }; @@ -147,7 +147,13 @@ impl BorsTester { let mut repos = HashMap::default(); for (name, repo) in loaded_repos { let repo = repo.unwrap(); - repos.insert(name, Arc::new(repo)); + repos.insert(name.clone(), Arc::new(repo)); + } + + for (name, _) in &repos { + if let Err(error) = db.insert_repo_if_not_exists(name, TreeState::Open).await { + tracing::warn!("Failed to insert repository {name} in test: {error:?}"); + } } let ctx = BorsContext::new( @@ -298,6 +304,10 @@ impl BorsTester { self.get_branch("automation/bors/try") } + pub fn auto_branch(&self) -> Branch { + self.get_branch("automation/bors/auto") + } + /// Wait until the next bot comment is received on the default repo and the default PR. pub async fn get_comment(&mut self) -> anyhow::Result { Ok(self @@ -361,6 +371,22 @@ impl BorsTester { .unwrap(); } + pub async fn process_merge_queue(&self) { + // Wait until the merge queue processing is fully handled + wait_for_marker( + async || { + self.global_tx + .send(BorsGlobalEvent::ProcessMergeQueue) + .await + .unwrap(); + Ok(()) + }, + &WAIT_FOR_MERGE_QUEUE, + ) + .await + .unwrap(); + } + /// Performs a single started/success/failure workflow event. pub async fn workflow_event(&mut self, event: WorkflowEvent) -> anyhow::Result<()> { // Update the status of the workflow in the GitHub state mock diff --git a/src/tests/mocks/mod.rs b/src/tests/mocks/mod.rs index f709f2d2..2e23455a 100644 --- a/src/tests/mocks/mod.rs +++ b/src/tests/mocks/mod.rs @@ -29,7 +29,7 @@ pub use workflow::WorkflowJob; pub use workflow::WorkflowRunData; mod app; -mod bors; +pub mod bors; mod comment; mod github; mod permissions; From 19bf9f514d09374f2081ca642b71e00332467664 Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Sun, 20 Jul 2025 13:53:58 +0100 Subject: [PATCH 2/3] Complete auto build - Post comment - Update check run - Update label --- rust-bors.example.toml | 2 + src/bors/comment.rs | 17 ++ src/bors/handlers/pr_events.rs | 24 ++- src/bors/handlers/review.rs | 10 +- src/bors/handlers/trybuild.rs | 51 +++--- src/bors/handlers/workflow.rs | 267 +++++++++++++++++++++++++++++--- src/bors/merge_queue.rs | 212 ++++++++++++++++++++++++- src/config.rs | 26 +++- src/github/labels.rs | 2 + src/tests/mocks/bors.rs | 31 +++- src/tests/mocks/comment.rs | 6 +- src/tests/mocks/pull_request.rs | 60 +++---- src/tests/mocks/repository.rs | 14 +- src/utils/sort_queue.rs | 4 +- 14 files changed, 614 insertions(+), 112 deletions(-) diff --git a/rust-bors.example.toml b/rust-bors.example.toml index dc3247f4..73db61fd 100644 --- a/rust-bors.example.toml +++ b/rust-bors.example.toml @@ -19,6 +19,8 @@ approve = ["+approved"] try = ["+foo", "-bar"] try_succeed = ["+foobar", "+foo", "+baz"] try_failed = [] +auto_build_succeeded = ["+foo", "+bar"] +auto_build_failed = ["+foo", "+bar"] # Labels that will block approval when present on a PR # (Optional) diff --git a/src/bors/comment.rs b/src/bors/comment.rs index 3ff61d63..680f06ba 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -310,3 +310,20 @@ pub fn auto_build_started_comment(head_sha: &CommitSha, merge_sha: &CommitSha) - head_sha, merge_sha )) } + +pub fn auto_build_succeeded_comment( + workflows: &[WorkflowModel], + approved_by: &str, + merge_sha: &CommitSha, + base_ref: &str, +) -> Comment { + let urls = workflows + .iter() + .map(|w| format!("[{}]({})", w.name, w.url)) + .collect::>() + .join(", "); + + Comment::new(format!( + ":sunny: Test successful - {urls}\nApproved by: `{approved_by}`\nPushing {merge_sha} to `{base_ref}`...", + )) +} diff --git a/src/bors/handlers/pr_events.rs b/src/bors/handlers/pr_events.rs index 15fe8675..c948d0f6 100644 --- a/src/bors/handlers/pr_events.rs +++ b/src/bors/handlers/pr_events.rs @@ -659,18 +659,22 @@ mod tests { #[sqlx::test] async fn enqueue_prs_on_push_to_branch(pool: sqlx::PgPool) { run_test(pool, async |tester| { - tester.open_pr(default_repo_name(), false).await?; + let pr = tester.open_pr(default_repo_name(), false).await?; tester.push_to_branch(default_branch_name()).await?; tester - .wait_for_default_pr(|pr| pr.mergeable_state == MergeableState::Unknown) + .wait_for_pr(default_repo_name(), pr.number.0, |pr| { + pr.mergeable_state == MergeableState::Unknown + }) .await?; tester .default_repo() .lock() - .get_pr_mut(default_pr_number()) + .get_pr_mut(pr.number.0) .mergeable_state = OctocrabMergeableState::Dirty; tester - .wait_for_default_pr(|pr| pr.mergeable_state == MergeableState::HasConflicts) + .wait_for_pr(default_repo_name(), pr.number.0, |pr| { + pr.mergeable_state == MergeableState::HasConflicts + }) .await?; Ok(()) }) @@ -680,17 +684,21 @@ mod tests { #[sqlx::test] async fn enqueue_prs_on_pr_opened(pool: sqlx::PgPool) { run_test(pool, async |tester| { - tester.open_pr(default_repo_name(), false).await?; + let pr = tester.open_pr(default_repo_name(), false).await?; tester - .wait_for_default_pr(|pr| pr.mergeable_state == MergeableState::Unknown) + .wait_for_pr(default_repo_name(), pr.number.0, |pr| { + pr.mergeable_state == MergeableState::Unknown + }) .await?; tester .default_repo() .lock() - .get_pr_mut(default_pr_number()) + .get_pr_mut(pr.number.0) .mergeable_state = OctocrabMergeableState::Dirty; tester - .wait_for_default_pr(|pr| pr.mergeable_state == MergeableState::HasConflicts) + .wait_for_pr(default_repo_name(), pr.number.0, |pr| { + pr.mergeable_state == MergeableState::HasConflicts + }) .await?; Ok(()) }) diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 621aaf35..3509eb2c 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -717,13 +717,9 @@ mod tests { gh.check_sha_history( default_repo_name(), TRY_MERGE_BRANCH_NAME, - &["main-sha1", "merge-main-sha1-pr-1-sha-0"], - ); - gh.check_sha_history( - default_repo_name(), - TRY_BRANCH_NAME, - &["merge-main-sha1-pr-1-sha-0"], + &["main-sha1", "merge-0-pr-1"], ); + gh.check_sha_history(default_repo_name(), TRY_BRANCH_NAME, &["merge-0-pr-1"]); } #[sqlx::test] @@ -1130,7 +1126,7 @@ mod tests { tester.post_comment("@bors try").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0… + :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index 8750c0f0..ed6c355c 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -422,9 +422,9 @@ mod tests { tester.get_comment().await?, @r#" :sunny: Try build successful ([Workflow1](https://github.com/workflows/Workflow1/1)) - Build commit: merge-main-sha1-pr-1-sha-0 (`merge-main-sha1-pr-1-sha-0`, parent: `main-sha1`) + Build commit: merge-0-pr-1 (`merge-0-pr-1`, parent: `main-sha1`) - + "# ); Ok(()) @@ -507,7 +507,7 @@ mod tests { insta::assert_snapshot!( tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0… + :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. " @@ -524,7 +524,7 @@ mod tests { insta::assert_snapshot!( tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0… + :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. " @@ -631,13 +631,9 @@ try-job: Bar gh.check_sha_history( default_repo_name(), TRY_MERGE_BRANCH_NAME, - &["main-sha1", "merge-main-sha1-pr-1-sha-0"], - ); - gh.check_sha_history( - default_repo_name(), - TRY_BRANCH_NAME, - &["merge-main-sha1-pr-1-sha-0"], + &["main-sha1", "merge-0-pr-1"], ); + gh.check_sha_history(default_repo_name(), TRY_BRANCH_NAME, &["merge-0-pr-1"]); } #[sqlx::test] @@ -653,10 +649,7 @@ try-job: Bar gh.check_sha_history( default_repo_name(), TRY_MERGE_BRANCH_NAME, - &[ - "ea9c1b050cc8b420c2c211d2177811e564a4dc60", - "merge-ea9c1b050cc8b420c2c211d2177811e564a4dc60-pr-1-sha-0", - ], + &["ea9c1b050cc8b420c2c211d2177811e564a4dc60", "merge-0-pr-1"], ); } @@ -671,7 +664,7 @@ try-job: Bar tester.expect_comments(1).await; tester.post_comment("@bors try parent=last").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-ea9c1b050cc8b420c2c211d2177811e564a4dc60-pr-1-sha-1… + :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… To cancel the try build, run the command `@bors try cancel`. "); @@ -683,9 +676,9 @@ try-job: Bar TRY_MERGE_BRANCH_NAME, &[ "ea9c1b050cc8b420c2c211d2177811e564a4dc60", - "merge-ea9c1b050cc8b420c2c211d2177811e564a4dc60-pr-1-sha-0", + "merge-0-pr-1", "ea9c1b050cc8b420c2c211d2177811e564a4dc60", - "merge-ea9c1b050cc8b420c2c211d2177811e564a4dc60-pr-1-sha-1", + "merge-1-pr-1", ], ); } @@ -773,7 +766,7 @@ try-job: Bar tester.expect_comments(1).await; tester.post_comment("@bors try").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-1… + :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… To cancel the try build, run the command `@bors try cancel`. "); @@ -795,7 +788,7 @@ try-job: Bar ) .await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0… + :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); @@ -816,7 +809,7 @@ try-job: Bar tester.post_comment("@bors try").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-1… + :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… (The previously running try build was automatically cancelled.) @@ -840,18 +833,22 @@ try-job: Bar tester.post_comment("@bors try").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-1… + :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… To cancel the try build, run the command `@bors try cancel`. "); tester - .workflow_full_success(WorkflowRunData::from(tester.try_branch()).with_run_id(2).with_check_suite_id(2)) + .workflow_full_success( + WorkflowRunData::from(tester.try_branch()) + .with_run_id(2) + .with_check_suite_id(2), + ) .await?; insta::assert_snapshot!(tester.get_comment().await?, @r#" :sunny: Try build successful ([Workflow1](https://github.com/workflows/Workflow1/2)) - Build commit: merge-main-sha1-pr-1-sha-1 (`merge-main-sha1-pr-1-sha-1`, parent: `main-sha1`) + Build commit: merge-1-pr-1 (`merge-1-pr-1`, parent: `main-sha1`) - + "#); Ok(()) }) @@ -988,7 +985,7 @@ try = ["+foo", "+bar", "-baz"] .run_test(async |tester| { tester.post_comment("@bors try").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0… + :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); @@ -1013,7 +1010,7 @@ try_succeed = ["+foo", "+bar", "-baz"] .run_test(async |tester| { tester.post_comment("@bors try").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0… + :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); @@ -1043,7 +1040,7 @@ try_failed = ["+foo", "+bar", "-baz"] .run_test(async |tester| { tester.post_comment("@bors try").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - :hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0… + :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index bc05ae48..9ac084bc 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -1,6 +1,7 @@ use crate::PgDbClient; use crate::bors::comment::{build_failed_comment, try_build_succeeded_comment}; use crate::bors::event::{WorkflowRunCompleted, WorkflowRunStarted}; +use crate::bors::handlers::TRY_BRANCH_NAME; use crate::bors::handlers::is_bors_observed_branch; use crate::bors::handlers::labels::handle_label_trigger; use crate::bors::merge_queue::AUTO_BRANCH_NAME; @@ -184,21 +185,41 @@ async fn maybe_complete_build( let has_failure = db_workflow_runs .iter() .any(|check| matches!(check.status, WorkflowStatus::Failure)); + let build_succeeded = !has_failure; + let pr_num = pr.number; + let branch = payload.branch.as_str(); - let (status, trigger) = if has_failure { - (BuildStatus::Failure, LabelTrigger::TryBuildFailed) + let status = if build_succeeded { + BuildStatus::Success } else { - (BuildStatus::Success, LabelTrigger::TryBuildSucceeded) + BuildStatus::Failure + }; + let trigger = match branch { + TRY_BRANCH_NAME => { + if build_succeeded { + LabelTrigger::TryBuildSucceeded + } else { + LabelTrigger::TryBuildFailed + } + } + AUTO_BRANCH_NAME => { + if build_succeeded { + LabelTrigger::AutoBuildSucceeded + } else { + LabelTrigger::AutoBuildFailed + } + } + _ => unreachable!("Branch should be bors observed branch"), }; - db.update_build_status(&build, status).await?; - handle_label_trigger(repo, pr.number, trigger).await?; + db.update_build_status(&build, status).await?; + handle_label_trigger(repo, pr_num, trigger).await?; if let Some(check_run_id) = build.check_run_id { - let (status, conclusion) = if has_failure { - (CheckRunStatus::Completed, Some(CheckRunConclusion::Failure)) - } else { + let (status, conclusion) = if build_succeeded { (CheckRunStatus::Completed, Some(CheckRunConclusion::Success)) + } else { + (CheckRunStatus::Completed, Some(CheckRunConclusion::Failure)) }; if let Err(error) = repo @@ -210,12 +231,29 @@ async fn maybe_complete_build( } } + // Trigger merge queue when an auto build completes + if payload.branch == AUTO_BRANCH_NAME { + merge_queue_tx.trigger().await?; + } + db_workflow_runs.sort_by(|a, b| a.name.cmp(&b.name)); - let message = if !has_failure { - tracing::info!("Build succeeded"); - try_build_succeeded_comment(&db_workflow_runs, payload.commit_sha, &build) + let comment_opt = if build_succeeded { + tracing::info!("Build succeeded for PR {pr_num}"); + + if branch == TRY_BRANCH_NAME { + Some(try_build_succeeded_comment( + &db_workflow_runs, + payload.commit_sha, + &build, + )) + } else { + // Merge queue will post the build succeeded comment + None + } } else { + tracing::info!("Build failed for PR {pr_num}"); + // Download failed jobs let mut workflow_runs: Vec = vec![]; for workflow_run in db_workflow_runs { @@ -235,14 +273,11 @@ async fn maybe_complete_build( }) } - tracing::info!("Build failed"); - build_failed_comment(repo.repository(), workflow_runs) + Some(build_failed_comment(repo.repository(), workflow_runs)) }; - repo.client.post_comment(pr.number, message).await?; - // Trigger merge queue when an auto build completes - if payload.branch == AUTO_BRANCH_NAME { - merge_queue_tx.trigger().await?; + if let Some(comment) = comment_opt { + repo.client.post_comment(pr_num, comment).await?; } Ok(()) @@ -273,10 +308,24 @@ async fn get_failed_jobs( #[cfg(test)] mod tests { - use crate::database::WorkflowStatus; + use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus}; + + use crate::bors::merge_queue::AUTO_BUILD_CHECK_RUN_NAME; use crate::database::operations::get_all_workflows; + use crate::database::{BuildStatus, WorkflowStatus}; use crate::tests::BorsTester; - use crate::tests::mocks::{Branch, WorkflowEvent, WorkflowRunData, run_test}; + use crate::tests::mocks::{ + BorsBuilder, Branch, GitHubState, WorkflowEvent, WorkflowRunData, default_pr_number, + run_test, + }; + + fn gh_state_with_merge_queue() -> GitHubState { + GitHubState::default().with_default_config( + r#" + merge_queue_enabled = true + "#, + ) + } #[sqlx::test] async fn workflow_started_unknown_build(pool: sqlx::PgPool) { @@ -353,7 +402,10 @@ mod tests { let w2 = WorkflowRunData::from(tester.try_branch()).with_run_id(2); // Let the GH mock know about the existence of the second workflow - tester.default_repo().lock().update_workflow_run(w2.clone(), WorkflowStatus::Pending); + tester + .default_repo() + .lock() + .update_workflow_run(w2.clone(), WorkflowStatus::Pending); // Finish w1 while w2 is not yet in the DB tester.workflow_full_success(w1).await?; tester.workflow_full_success(w2).await?; @@ -364,9 +416,9 @@ mod tests { :sunny: Try build successful - [Workflow1](https://github.com/workflows/Workflow1/1) :white_check_mark: - [Workflow1](https://github.com/workflows/Workflow1/2) :white_check_mark: - Build commit: merge-main-sha1-pr-1-sha-0 (`merge-main-sha1-pr-1-sha-0`, parent: `main-sha1`) + Build commit: merge-0-pr-1 (`merge-0-pr-1`, parent: `main-sha1`) - + "# ); Ok(()) @@ -394,14 +446,14 @@ mod tests { :sunny: Try build successful - [Workflow1](https://github.com/workflows/Workflow1/1) :white_check_mark: - [Workflow1](https://github.com/workflows/Workflow1/2) :white_check_mark: - Build commit: merge-main-sha1-pr-1-sha-0 (`merge-main-sha1-pr-1-sha-0`, parent: `main-sha1`) + Build commit: merge-0-pr-1 (`merge-0-pr-1`, parent: `main-sha1`) - + "# ); Ok(()) }) - .await; + .await; } // Finish the build early when we encounter the first failure @@ -425,4 +477,169 @@ mod tests { }) .await; } + + #[sqlx::test] + async fn auto_build_success_updates_check_run(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .github(gh_state_with_merge_queue()) + .run_test(async |tester| { + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + tester.process_merge_queue().await; + tester.expect_comments(1).await; + + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + tester.expect_check_run( + &tester.default_pr().await.get_gh_pr().head_sha, + AUTO_BUILD_CHECK_RUN_NAME, + AUTO_BUILD_CHECK_RUN_NAME, + CheckRunStatus::Completed, + Some(CheckRunConclusion::Success), + ); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_success_labels(pool: sqlx::PgPool) { + let github = GitHubState::default().with_default_config( + r#" +merge_queue_enabled = true + +[labels] +auto_build_succeeded = ["+foo", "+bar", "-baz"] + "#, + ); + + BorsBuilder::new(pool) + .github(github) + .run_test(async |tester| { + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + tester.process_merge_queue().await; + tester.expect_comments(1).await; + + let repo = tester.default_repo(); + repo.lock() + .get_pr(default_pr_number()) + .check_added_labels(&[]); + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + + let pr = repo.lock().get_pr(default_pr_number()).clone(); + pr.check_added_labels(&["foo", "bar"]); + pr.check_removed_labels(&["baz"]); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_failed_labels(pool: sqlx::PgPool) { + let github = GitHubState::default().with_default_config( + r#" +merge_queue_enabled = true + +[labels] +auto_build_failed = ["+foo", "+bar", "-baz"] + "#, + ); + + BorsBuilder::new(pool) + .github(github) + .run_test(async |tester| { + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + tester.process_merge_queue().await; + tester.expect_comments(1).await; + + let repo = tester.default_repo(); + repo.lock() + .get_pr(default_pr_number()) + .check_added_labels(&[]); + + tester.workflow_full_failure(tester.auto_branch()).await?; + tester.expect_comments(1).await; + + let pr = repo.lock().get_pr(default_pr_number()).clone(); + pr.check_added_labels(&["foo", "bar"]); + pr.check_removed_labels(&["baz"]); + + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_fails_in_db(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .github(gh_state_with_merge_queue()) + .run_test(async |tester| { + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + tester.process_merge_queue().await; + tester.expect_comments(1).await; + + tester.workflow_full_failure(tester.auto_branch()).await?; + tester.expect_comments(1).await; + + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_failed_comment(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .github(gh_state_with_merge_queue()) + .run_test(async |tester| { + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + tester.process_merge_queue().await; + tester.expect_comments(1).await; + + let repo = tester.default_repo(); + repo.lock() + .get_pr(default_pr_number()) + .check_added_labels(&[]); + + tester.workflow_full_failure(tester.auto_branch()).await?; + tester.wait_for_default_pr(|pr| { + pr.auto_build.as_ref().unwrap().status == BuildStatus::Failure + }).await?; + insta::assert_snapshot!( + tester.get_comment().await?, + @":broken_heart: Test failed ([Workflow1](https://github.com/workflows/Workflow1/1))" + ); + + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_failure_updates_check_run(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .github(gh_state_with_merge_queue()) + .run_test(async |tester| { + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + tester.process_merge_queue().await; + tester.expect_comments(1).await; + + tester.workflow_full_failure(tester.auto_branch()).await?; + tester.expect_comments(1).await; + tester.expect_check_run( + &tester.default_pr().await.get_gh_pr().head_sha, + AUTO_BUILD_CHECK_RUN_NAME, + AUTO_BUILD_CHECK_RUN_NAME, + CheckRunStatus::Completed, + Some(CheckRunConclusion::Failure), + ); + Ok(()) + }) + .await; + } } diff --git a/src/bors/merge_queue.rs b/src/bors/merge_queue.rs index 52b96029..a8ffebe8 100644 --- a/src/bors/merge_queue.rs +++ b/src/bors/merge_queue.rs @@ -6,8 +6,8 @@ use tokio::sync::mpsc; use tracing::Instrument; use crate::BorsContext; -use crate::bors::RepositoryState; -use crate::bors::comment::auto_build_started_comment; +use crate::bors::comment::{auto_build_started_comment, auto_build_succeeded_comment}; +use crate::bors::{PullRequestStatus, RepositoryState}; use crate::database::{BuildStatus, PullRequestModel}; use crate::github::api::client::GithubRepositoryClient; use crate::github::api::operations::ForcePush; @@ -84,9 +84,39 @@ pub async fn merge_queue_tick(ctx: Arc) -> anyhow::Result<()> { let pr_num = pr.number; if let Some(auto_build) = &pr.auto_build { + let commit_sha = CommitSha(auto_build.commit_sha.clone()); + match auto_build.status { // Build successful - point the base branch to the merged commit. BuildStatus::Success => { + let workflows = ctx.db.get_workflows_for_build(auto_build).await?; + let comment = auto_build_succeeded_comment( + &workflows, + pr.approver().unwrap_or(""), + &commit_sha, + &pr.base_branch, + ); + repo.client.post_comment(pr.number, comment).await?; + + match repo + .client + .set_branch_to_sha(&pr.base_branch, &commit_sha, ForcePush::No) + .await + { + Ok(()) => { + tracing::info!("Auto build succeeded and merged for PR {pr_num}"); + + ctx.db + .set_pr_status( + &pr.repository, + pr.number, + PullRequestStatus::Merged, + ) + .await?; + } + Err(_) => {} + }; + // Break to give GitHub time to update the base branch. break; } @@ -289,12 +319,15 @@ mod tests { use sqlx::PgPool; use crate::{ - bors::merge_queue::{AUTO_BRANCH_NAME, AUTO_BUILD_CHECK_RUN_NAME}, - database::{WorkflowStatus, operations::get_all_workflows}, + bors::{ + PullRequestStatus, + merge_queue::{AUTO_BRANCH_NAME, AUTO_BUILD_CHECK_RUN_NAME, AUTO_MERGE_BRANCH_NAME}, + }, + database::{BuildStatus, WorkflowStatus, operations::get_all_workflows}, github::CommitSha, tests::{ BorsTester, - mocks::{BorsBuilder, GitHubState, WorkflowEvent, default_repo_name}, + mocks::{BorsBuilder, Comment, GitHubState, WorkflowEvent, default_repo_name}, }, }; @@ -363,7 +396,7 @@ mod tests { tester.process_merge_queue().await; insta::assert_snapshot!( tester.get_comment().await?, - @":hourglass: Testing commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0..." + @":hourglass: Testing commit pr-1-sha with merge merge-0-pr-1..." ); Ok(()) }) @@ -391,4 +424,171 @@ mod tests { }) .await; } + + #[sqlx::test] + async fn auto_build_success_comment(pool: sqlx::PgPool) { + run_merge_queue_test(pool, async |tester| { + start_auto_build(tester).await?; + tester.workflow_full_success(tester.auto_branch()).await?; + insta::assert_snapshot!( + tester.get_comment().await?, + @r" + :sunny: Test successful - [Workflow1](https://github.com/workflows/Workflow1/1) + Approved by: `default-user` + Pushing merge-0-pr-1 to `main`... + " + ); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_succeeds_and_merges_in_db(pool: sqlx::PgPool) { + run_merge_queue_test(pool, async |mut tester| { + start_auto_build(&mut tester).await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + tester + .wait_for_default_pr(|pr| { + pr.auto_build.as_ref().unwrap().status == BuildStatus::Success + }) + .await?; + tester + .wait_for_default_pr(|pr| pr.pr_status == PullRequestStatus::Merged) + .await?; + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_branch_history(pool: sqlx::PgPool) { + let gh = run_merge_queue_test(pool, async |mut tester| { + start_auto_build(&mut tester).await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + Ok(()) + }) + .await; + gh.check_sha_history(default_repo_name(), "main", &["main-sha1", "merge-0-pr-1"]); + gh.check_sha_history( + default_repo_name(), + AUTO_MERGE_BRANCH_NAME, + &["main-sha1", "merge-0-pr-1"], + ); + gh.check_sha_history(default_repo_name(), AUTO_BRANCH_NAME, &["merge-0-pr-1"]); + } + + #[sqlx::test] + async fn merge_queue_sequential_order(pool: sqlx::PgPool) { + let gh = run_merge_queue_test(pool, async |tester| { + let pr2 = tester.open_pr(default_repo_name(), false).await?; + let pr3 = tester.open_pr(default_repo_name(), false).await?; + + tester.post_comment("@bors r+").await?; + tester + .post_comment(Comment::pr(pr2.number.0, "@bors r+")) + .await?; + tester + .post_comment(Comment::pr(pr3.number.0, "@bors r+")) + .await?; + + tester.expect_comments(1).await; + tester + .expect_comment_on_pr(default_repo_name(), pr2.number.0) + .await?; + tester + .expect_comment_on_pr(default_repo_name(), pr3.number.0) + .await?; + + tester.process_merge_queue().await; + tester.expect_comments(1).await; + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + + tester.process_merge_queue().await; + tester + .expect_comment_on_pr(default_repo_name(), pr2.number.0) + .await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester + .expect_comment_on_pr(default_repo_name(), pr2.number.0) + .await?; + + tester.process_merge_queue().await; + tester + .expect_comment_on_pr(default_repo_name(), pr3.number.0) + .await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester + .expect_comment_on_pr(default_repo_name(), pr3.number.0) + .await?; + + Ok(()) + }) + .await; + + gh.check_sha_history( + default_repo_name(), + "main", + &["main-sha1", "merge-0-pr-1", "merge-1-pr-2", "merge-2-pr-3"], + ); + } + + #[sqlx::test] + async fn merge_queue_priority_order(pool: sqlx::PgPool) { + let gh = run_merge_queue_test(pool, async |tester| { + let pr2 = tester.open_pr(default_repo_name(), false).await?; + let pr3 = tester.open_pr(default_repo_name(), false).await?; + + tester.post_comment("@bors r+").await?; + tester + .post_comment(Comment::pr(pr2.number.0, "@bors r+")) + .await?; + tester + .post_comment(Comment::pr(pr3.number.0, "@bors r+ p=3")) + .await?; + + tester.expect_comments(1).await; + tester + .expect_comment_on_pr(default_repo_name(), pr2.number.0) + .await?; + tester + .expect_comment_on_pr(default_repo_name(), pr3.number.0) + .await?; + + tester.process_merge_queue().await; + tester.expect_comments(1).await; + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + + tester.process_merge_queue().await; + tester + .expect_comment_on_pr(default_repo_name(), pr3.number.0) + .await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester + .expect_comment_on_pr(default_repo_name(), pr3.number.0) + .await?; + + tester.process_merge_queue().await; + tester + .expect_comment_on_pr(default_repo_name(), pr2.number.0) + .await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester + .expect_comment_on_pr(default_repo_name(), pr2.number.0) + .await?; + + Ok(()) + }) + .await; + + gh.check_sha_history( + default_repo_name(), + "main", + &["main-sha1", "merge-0-pr-1", "merge-1-pr-3", "merge-2-pr-2"], + ); + } } diff --git a/src/config.rs b/src/config.rs index 1e584627..d2d5b802 100644 --- a/src/config.rs +++ b/src/config.rs @@ -73,6 +73,8 @@ where Try, TrySucceed, TryFailed, + AutoBuildSucceeded, + AutoBuildFailed, } impl From for LabelTrigger { @@ -83,6 +85,8 @@ where Trigger::Try => LabelTrigger::TryBuildStarted, Trigger::TrySucceed => LabelTrigger::TryBuildSucceeded, Trigger::TryFailed => LabelTrigger::TryBuildFailed, + Trigger::AutoBuildSucceeded => LabelTrigger::AutoBuildSucceeded, + Trigger::AutoBuildFailed => LabelTrigger::AutoBuildFailed, } } } @@ -210,9 +214,11 @@ approve = ["+approved"] try = ["+foo", "-bar"] try_succeed = ["+foobar", "+foo", "+baz"] try_failed = [] +auto_build_succeeded = ["+foobar", "-foo"] +auto_build_failed = ["+bar", "+baz"] "#; let config = load_config(content); - insta::assert_debug_snapshot!(config.labels.into_iter().collect::>(), @r###" + insta::assert_debug_snapshot!(config.labels.into_iter().collect::>(), @r#" { Approved: [ Add( @@ -244,8 +250,24 @@ try_failed = [] ), ], TryBuildFailed: [], + AutoBuildSucceeded: [ + Add( + "foobar", + ), + Remove( + "foo", + ), + ], + AutoBuildFailed: [ + Add( + "bar", + ), + Add( + "baz", + ), + ], } - "###); + "#); } #[test] diff --git a/src/github/labels.rs b/src/github/labels.rs index 337bb33b..fba4458f 100644 --- a/src/github/labels.rs +++ b/src/github/labels.rs @@ -6,6 +6,8 @@ pub enum LabelTrigger { TryBuildStarted, TryBuildSucceeded, TryBuildFailed, + AutoBuildSucceeded, + AutoBuildFailed, } #[derive(Debug, Eq, PartialEq)] diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs index 34c37f4d..edec1af2 100644 --- a/src/tests/mocks/bors.rs +++ b/src/tests/mocks/bors.rs @@ -318,6 +318,20 @@ impl BorsTester { .content) } + /// Wait until the next bot comment is received on the specified PR and consume it. + pub async fn expect_comment_on_pr( + &mut self, + repo: GithubRepoName, + pr: u64, + ) -> anyhow::Result { + Ok(self + .http_mock + .gh_server + .get_comment(repo, pr) + .await? + .content) + } + //-- Generation of GitHub events --// pub async fn post_comment>(&mut self, comment: C) -> anyhow::Result<()> { self.webhook_comment(comment.into()).await @@ -460,10 +474,23 @@ impl BorsTester { let number = { let repo = self.github.get_repo(&repo_name); let repo = repo.lock(); - repo.pull_requests.keys().max().copied().unwrap_or(1) + repo.pull_requests.keys().max().copied().unwrap_or(0) + 1 }; - let pr = PullRequest::new(repo_name, number, User::default_pr_author(), is_draft); + let pr = PullRequest::new( + repo_name.clone(), + number, + User::default_pr_author(), + is_draft, + ); + + // Add the PR to the repository + { + let repo = self.github.get_repo(&repo_name); + let mut repo = repo.lock(); + repo.pull_requests.insert(number, pr.clone()); + } + self.send_webhook( "pull_request", GitHubPullRequestEventPayload::new(pr.clone(), "opened", None), diff --git a/src/tests/mocks/comment.rs b/src/tests/mocks/comment.rs index 8bbf0ad9..e7ccd0d7 100644 --- a/src/tests/mocks/comment.rs +++ b/src/tests/mocks/comment.rs @@ -8,7 +8,7 @@ use url::Url; use crate::github::GithubRepoName; use crate::tests::mocks::repository::GitHubRepository; use crate::tests::mocks::user::{GitHubUser, User}; -use crate::tests::mocks::{Repo, default_pr_number}; +use crate::tests::mocks::{Repo, default_pr_number, default_repo_name}; #[derive(Clone, Debug)] pub struct Comment { @@ -30,6 +30,10 @@ impl Comment { } } + pub fn pr(pr: u64, content: &str) -> Self { + Self::new(default_repo_name(), pr, content) + } + pub fn with_author(self, author: User) -> Self { Self { author, ..self } } diff --git a/src/tests/mocks/pull_request.rs b/src/tests/mocks/pull_request.rs index a689da3f..cdecb959 100644 --- a/src/tests/mocks/pull_request.rs +++ b/src/tests/mocks/pull_request.rs @@ -52,41 +52,44 @@ pub async fn mock_pull_requests( .mount(mock_server) .await; + let repo_clone = repo.clone(); + dynamic_mock_req( + move |_req: &Request, [pr_number]: [&str; 1]| { + let pr_number: u64 = pr_number.parse().unwrap(); + let pull_request_error = repo_clone.lock().pull_request_error; + if pull_request_error { + ResponseTemplate::new(500) + } else if let Some(pr) = repo_clone.lock().pull_requests.get(&pr_number) { + ResponseTemplate::new(200).set_body_json(GitHubPullRequest::from(pr.clone())) + } else { + ResponseTemplate::new(404) + } + }, + "GET", + format!("^/repos/{repo_name}/pulls/([0-9]+)$"), + ) + .mount(mock_server) + .await; + + mock_pr_comments(repo.clone(), comments_tx.clone(), mock_server).await; + let prs = repo.lock().pull_requests.clone(); for &pr_number in prs.keys() { - let repo_clone = repo.clone(); - Mock::given(method("GET")) - .and(path(format!("/repos/{repo_name}/pulls/{pr_number}"))) - .respond_with(move |_: &Request| { - let pull_request_error = repo_clone.lock().pull_request_error; - if pull_request_error { - ResponseTemplate::new(500) - } else if let Some(pr) = repo_clone.lock().pull_requests.get(&pr_number) { - ResponseTemplate::new(200).set_body_json(GitHubPullRequest::from(pr.clone())) - } else { - ResponseTemplate::new(404) - } - }) - .mount(mock_server) - .await; - - mock_pr_comments(repo.clone(), pr_number, comments_tx.clone(), mock_server).await; mock_pr_labels(repo.clone(), repo_name.clone(), pr_number, mock_server).await; } } async fn mock_pr_comments( repo: Arc>, - pr_number: u64, comments_tx: Sender, mock_server: &MockServer, ) { let repo_name = repo.lock().name.clone(); - Mock::given(method("POST")) - .and(path(format!( - "/repos/{repo_name}/issues/{pr_number}/comments", - ))) - .respond_with(move |req: &Request| { + let repo_name_clone = repo_name.clone(); + dynamic_mock_req( + move |req: &Request, [pr_number]: [&str; 1]| { + let pr_number: u64 = pr_number.parse().unwrap(); + #[derive(Deserialize)] struct CommentCreatePayload { body: String, @@ -97,7 +100,7 @@ async fn mock_pr_comments( let pr = repo.pull_requests.get_mut(&pr_number).unwrap(); let comment_id = pr.next_comment_id(); - let comment = Comment::new(repo_name.clone(), pr_number, &comment_payload.body) + let comment = Comment::new(repo_name_clone.clone(), pr_number, &comment_payload.body) .with_author(User::bors_bot()) .with_id(comment_id); @@ -106,9 +109,12 @@ async fn mock_pr_comments( // `tx.send()`. comments_tx.try_send(comment.clone()).unwrap(); ResponseTemplate::new(201).set_body_json(GitHubComment::from(comment)) - }) - .mount(mock_server) - .await; + }, + "POST", + format!("^/repos/{repo_name}/issues/([0-9]+)/comments$"), + ) + .mount(mock_server) + .await; } async fn mock_pr_labels( diff --git a/src/tests/mocks/repository.rs b/src/tests/mocks/repository.rs index 0e24bdcc..a3316016 100644 --- a/src/tests/mocks/repository.rs +++ b/src/tests/mocks/repository.rs @@ -547,7 +547,7 @@ async fn mock_merge_branch(repo: Arc>, mock_server: &MockServer) { let head_sha = match head { None => { // head is a SHA - data.head + data.head.clone() } Some(branch) => { // head is a branch @@ -562,10 +562,14 @@ async fn mock_merge_branch(repo: Arc>, mock_server: &MockServer) { return ResponseTemplate::new(409); } - let merge_sha = format!( - "merge-{}-{head_sha}-{}", - base_branch.sha, base_branch.merge_counter - ); + let source_info = if head_sha.starts_with("pr-") && head_sha.ends_with("-sha") { + head_sha.strip_suffix("-sha").unwrap() + } else { + // Use the original head reference (branch name or SHA) + &data.head + }; + + let merge_sha = format!("merge-{}-{source_info}", base_branch.merge_counter); base_branch.merge_counter += 1; base_branch.set_to_sha(&merge_sha); repo.set_commit_message(&merge_sha, &data.commit_message); diff --git a/src/utils/sort_queue.rs b/src/utils/sort_queue.rs index 58c8cd5b..ae138373 100644 --- a/src/utils/sort_queue.rs +++ b/src/utils/sort_queue.rs @@ -46,8 +46,8 @@ fn get_queue_blocking_priority(pr: &PullRequestModel) -> u32 { fn get_status_priority(pr: &PullRequestModel) -> u32 { match &pr.auto_build { Some(build) => match build.status { - BuildStatus::Success => 1, - BuildStatus::Pending => 1, // Same as success since queue blocking is handled separately + BuildStatus::Success => 0, + BuildStatus::Pending => 1, BuildStatus::Failure => 3, BuildStatus::Cancelled | BuildStatus::Timeouted => 2, }, From d18150cb008902e17734107436ab1479c563eaac Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Sun, 20 Jul 2025 14:05:22 +0100 Subject: [PATCH 3/3] Deal with auto build push error --- src/bors/comment.rs | 6 ++ src/bors/merge_queue.rs | 103 ++++++++++++++++++++++++++++++++-- src/tests/mocks/repository.rs | 7 +++ 3 files changed, 112 insertions(+), 4 deletions(-) diff --git a/src/bors/comment.rs b/src/bors/comment.rs index 680f06ba..54979dde 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -327,3 +327,9 @@ pub fn auto_build_succeeded_comment( ":sunny: Test successful - {urls}\nApproved by: `{approved_by}`\nPushing {merge_sha} to `{base_ref}`...", )) } + +pub fn auto_build_push_failed_comment(error: &str) -> Comment { + Comment::new(format!( + ":eyes: Test was successful, but fast-forwarding failed: {error}" + )) +} diff --git a/src/bors/merge_queue.rs b/src/bors/merge_queue.rs index a8ffebe8..3c6d06d0 100644 --- a/src/bors/merge_queue.rs +++ b/src/bors/merge_queue.rs @@ -1,12 +1,15 @@ use anyhow::anyhow; -use octocrab::params::checks::{CheckRunOutput, CheckRunStatus}; +use octocrab::models::CheckRunId; +use octocrab::params::checks::{CheckRunConclusion, CheckRunOutput, CheckRunStatus}; use std::future::Future; use std::sync::Arc; use tokio::sync::mpsc; use tracing::Instrument; use crate::BorsContext; -use crate::bors::comment::{auto_build_started_comment, auto_build_succeeded_comment}; +use crate::bors::comment::{ + auto_build_push_failed_comment, auto_build_started_comment, auto_build_succeeded_comment, +}; use crate::bors::{PullRequestStatus, RepositoryState}; use crate::database::{BuildStatus, PullRequestModel}; use crate::github::api::client::GithubRepositoryClient; @@ -114,7 +117,36 @@ pub async fn merge_queue_tick(ctx: Arc) -> anyhow::Result<()> { ) .await?; } - Err(_) => {} + Err(error) => { + tracing::error!( + "Failed to push PR {pr_num} to base branch: {:?}", + error + ); + + if let Some(check_run_id) = auto_build.check_run_id { + if let Err(error) = repo + .client + .update_check_run( + CheckRunId(check_run_id as u64), + CheckRunStatus::Completed, + Some(CheckRunConclusion::Failure), + None, + ) + .await + { + tracing::error!( + "Could not update check run {check_run_id}: {error:?}" + ); + } + } + + ctx.db + .update_build_status(auto_build, BuildStatus::Failure) + .await?; + + let comment = auto_build_push_failed_comment(&error.to_string()); + repo.client.post_comment(pr.number, comment).await?; + } }; // Break to give GitHub time to update the base branch. @@ -315,7 +347,7 @@ pub fn start_merge_queue(ctx: Arc) -> (MergeQueueSender, impl Futur #[cfg(test)] mod tests { - use octocrab::params::checks::CheckRunStatus; + use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus}; use sqlx::PgPool; use crate::{ @@ -462,6 +494,69 @@ mod tests { .await; } + #[sqlx::test] + async fn auto_build_push_fail_comment(pool: sqlx::PgPool) { + run_merge_queue_test(pool, async |mut tester| { + start_auto_build(&mut tester).await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + + tester.default_repo().lock().push_error = true; + + tester.process_merge_queue().await; + insta::assert_snapshot!( + tester.get_comment().await?, + @":eyes: Test was successful, but fast-forwarding failed: IO error" + ); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_push_fail_updates_check_run(pool: sqlx::PgPool) { + run_merge_queue_test(pool, async |mut tester| { + start_auto_build(&mut tester).await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + + tester.default_repo().lock().push_error = true; + + tester.process_merge_queue().await; + tester.expect_comments(1).await; + tester.expect_check_run( + &tester.default_pr().await.get_gh_pr().head_sha, + AUTO_BUILD_CHECK_RUN_NAME, + AUTO_BUILD_CHECK_RUN_NAME, + CheckRunStatus::Completed, + Some(CheckRunConclusion::Failure), + ); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn auto_build_push_fail_in_db(pool: sqlx::PgPool) { + run_merge_queue_test(pool, async |mut tester| { + start_auto_build(&mut tester).await?; + tester.workflow_full_success(tester.auto_branch()).await?; + tester.expect_comments(1).await; + + tester.default_repo().lock().push_error = true; + + tester.process_merge_queue().await; + tester.expect_comments(1).await; + tester + .wait_for_default_pr(|pr| { + pr.auto_build.as_ref().unwrap().status == BuildStatus::Failure + }) + .await?; + Ok(()) + }) + .await; + } + #[sqlx::test] async fn auto_build_branch_history(pool: sqlx::PgPool) { let gh = run_merge_queue_test(pool, async |mut tester| { diff --git a/src/tests/mocks/repository.rs b/src/tests/mocks/repository.rs index a3316016..eac20201 100644 --- a/src/tests/mocks/repository.rs +++ b/src/tests/mocks/repository.rs @@ -168,6 +168,8 @@ pub struct Repo { pub check_runs: Vec, /// Cause pull request fetch to fail. pub pull_request_error: bool, + // Cause branch push to fail. + pub push_error: bool, pub pr_push_counter: u64, } @@ -184,6 +186,7 @@ impl Repo { workflow_cancel_error: false, workflow_runs: vec![], pull_request_error: false, + push_error: false, pr_push_counter: 0, check_runs: vec![], } @@ -509,6 +512,10 @@ async fn mock_update_branch(repo: Arc>, mock_server: &MockServer) { let data: SetRefRequest = req.body_json().unwrap(); + if repo.push_error { + return ResponseTemplate::new(500).set_body_string("Push error"); + } + let sha = data.sha; match repo.get_branch_by_name(branch_name) { Some(branch) => {