diff --git a/src/bors/handlers/retry.rs b/src/bors/handlers/retry.rs index 1f9e82a5..5c6e6a41 100644 --- a/src/bors/handlers/retry.rs +++ b/src/bors/handlers/retry.rs @@ -4,6 +4,7 @@ use crate::PgDbClient; use crate::bors::handlers::{PullRequestData, deny_request, has_permission}; use crate::bors::merge_queue::MergeQueueSender; use crate::bors::{Comment, RepositoryState}; +use crate::database::QueueStatus; use crate::github::{GithubUser, PullRequestNumber}; use crate::permissions::PermissionType; @@ -21,7 +22,7 @@ pub(super) async fn command_retry( let pr_model = pr.db; - if pr_model.is_stalled() { + if matches!(pr_model.queue_status(), QueueStatus::Stalled(_, _)) { db.clear_auto_build(pr_model).await?; merge_queue_tx.notify().await?; } else { diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index 11229f6f..fe14f556 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -6,7 +6,9 @@ use crate::bors::handlers::{BuildType, is_bors_observed_branch}; use crate::bors::handlers::{get_build_type, hide_try_build_started_comments}; use crate::bors::merge_queue::MergeQueueSender; use crate::bors::{FailedWorkflowRun, RepositoryState, WorkflowRun}; -use crate::database::{BuildModel, BuildStatus, PullRequestModel, WorkflowModel, WorkflowStatus}; +use crate::database::{ + BuildModel, BuildStatus, PullRequestModel, QueueStatus, WorkflowModel, WorkflowStatus, +}; use crate::github::api::client::GithubRepositoryClient; use crate::github::{CommitSha, LabelTrigger}; use octocrab::models::CheckRunId; @@ -407,16 +409,14 @@ pub async fn maybe_cancel_auto_build( pr: &PullRequestModel, reason: AutoBuildCancelReason, ) -> anyhow::Result> { - let Some(auto_build) = &pr.auto_build else { - return Ok(None); + let auto_build = match pr.queue_status() { + QueueStatus::Pending(_, build) => build, + _ => return Ok(None), }; - if auto_build.status != BuildStatus::Pending { - return Ok(None); - } tracing::info!("Cancelling auto build {auto_build:?}"); - match cancel_build(client, db, auto_build, CheckRunConclusion::Cancelled).await { + match cancel_build(client, db, &auto_build, CheckRunConclusion::Cancelled).await { Ok(workflows) => { tracing::info!("Auto build cancelled"); let workflow_urls = workflows.into_iter().map(|w| w.url).collect(); diff --git a/src/bors/merge_queue.rs b/src/bors/merge_queue.rs index b61c2ec7..66201c35 100644 --- a/src/bors/merge_queue.rs +++ b/src/bors/merge_queue.rs @@ -12,10 +12,13 @@ use crate::bors::comment::{ merge_conflict_comment, }; use crate::bors::{PullRequestStatus, RepositoryState}; -use crate::database::{BuildStatus, MergeableState, OctocrabMergeableState, PullRequestModel}; +use crate::database::{ + ApprovalInfo, BuildModel, BuildStatus, MergeableState, OctocrabMergeableState, + PullRequestModel, QueueStatus, +}; use crate::github::api::client::CheckRunOutput; use crate::github::api::operations::{BranchUpdateError, ForcePush}; -use crate::github::{CommitSha, PullRequest}; +use crate::github::{CommitSha, PullRequest, PullRequestNumber}; use crate::github::{MergeResult, attempt_merge}; use crate::utils::sort_queue::sort_queue_prs; @@ -130,107 +133,24 @@ async fn process_repository(repo: &RepositoryState, ctx: &BorsContext) -> anyhow for pr in prs { let pr_num = pr.number; - let Some(auto_build) = &pr.auto_build else { - // No build exists for this PR - start a new auto build. - match start_auto_build(repo, ctx, &pr).await { - Ok(()) => { - tracing::info!("Starting auto build for PR {pr_num}"); - break; - } - Err(StartAutoBuildError::MergeConflict) => { - let gh_pr = repo.client.get_pull_request(pr.number).await?; - - tracing::debug!( - "Failed to start auto build for PR {pr_num} due to merge conflict" - ); - - ctx.db - .update_pr_mergeable_state(&pr, MergeableState::Unknown) - .await?; - repo.client - .post_comment(pr.number, merge_conflict_comment(&gh_pr.head.name)) - .await?; - continue; - } - Err(StartAutoBuildError::SanityCheckFailed(error)) => { - tracing::info!("Sanity check failed for PR {pr_num}: {error:?}"); - break; - } - Err(StartAutoBuildError::GitHubError(error)) => { - tracing::debug!( - "Failed to start auto build for PR {pr_num} due to a GitHub error: {error:?}" - ); - break; - } - Err(StartAutoBuildError::DatabaseError(error)) => { - tracing::debug!( - "Failed to start auto build for PR {pr_num} due to database error: {error:?}" - ); - break; - } - } - }; - - 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, - ); - - if let Err(error) = repo - .client - .set_branch_to_sha(&pr.base_branch, &commit_sha, ForcePush::No) - .await - { - tracing::error!( - "Failed to fast-forward base branch for PR {pr_num}: {error:?}" - ); - - let comment = match &error { - BranchUpdateError::Conflict(branch_name) => auto_build_push_failed_comment( - &format!("this PR has conflicts with the `{branch_name}` branch"), - ), - BranchUpdateError::ValidationFailed(branch_name) => { - auto_build_push_failed_comment(&format!( - "the tested commit was behind the `{branch_name}` branch" - )) - } - error => auto_build_push_failed_comment(&error.to_string()), - }; - - ctx.db - .update_build_status(auto_build, BuildStatus::Failure) - .await?; - - repo.client.post_comment(pr_num, comment).await?; - } else { - tracing::info!("Auto build succeeded and merged for PR {pr_num}"); - - ctx.db - .set_pr_status(&pr.repository, pr.number, PullRequestStatus::Merged) - .await?; - - repo.client.post_comment(pr.number, comment).await?; - } - - // Break to give GitHub time to update the base branch. + match pr.queue_status() { + QueueStatus::NotApproved => unreachable!(), + QueueStatus::Stalled(..) => unreachable!(), + QueueStatus::Pending(..) => { + // Build in progress - stop queue. We can only have one PR being built + // at a time. + tracing::info!("PR {pr_num} has a pending build - blocking queue"); 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"); + QueueStatus::ReadyForMerge(approval_info, auto_build) => { + handle_successful_build(repo, ctx, &pr, &auto_build, &approval_info, pr_num) + .await?; break; } - BuildStatus::Failure | BuildStatus::Cancelled | BuildStatus::Timeouted => { - unreachable!("Failed auto builds should be filtered out by SQL query"); + QueueStatus::Approved(..) => { + if handle_start_auto_build(repo, ctx, &pr, pr_num).await? { + break; + } } } } @@ -238,6 +158,101 @@ async fn process_repository(repo: &RepositoryState, ctx: &BorsContext) -> anyhow Ok(()) } +/// Handle a successful auto build by pointing the base branch to the merged commit. +async fn handle_successful_build( + repo: &RepositoryState, + ctx: &BorsContext, + pr: &PullRequestModel, + auto_build: &BuildModel, + approval_info: &ApprovalInfo, + pr_num: PullRequestNumber, +) -> anyhow::Result<()> { + let commit_sha = CommitSha(auto_build.commit_sha.clone()); + let workflows = ctx.db.get_workflows_for_build(auto_build).await?; + let comment = auto_build_succeeded_comment( + &workflows, + &approval_info.approver, + &commit_sha, + &pr.base_branch, + ); + + if let Err(error) = repo + .client + .set_branch_to_sha(&pr.base_branch, &commit_sha, ForcePush::No) + .await + { + tracing::error!("Failed to fast-forward base branch for PR {pr_num}: {error:?}"); + + let error_comment = match &error { + BranchUpdateError::Conflict(branch_name) => auto_build_push_failed_comment(&format!( + "this PR has conflicts with the `{branch_name}` branch" + )), + BranchUpdateError::ValidationFailed(branch_name) => auto_build_push_failed_comment( + &format!("the tested commit was behind the `{branch_name}` branch"), + ), + error => auto_build_push_failed_comment(&error.to_string()), + }; + + ctx.db + .update_build_status(auto_build, BuildStatus::Failure) + .await?; + repo.client.post_comment(pr_num, error_comment).await?; + } else { + tracing::info!("Auto build succeeded and merged for PR {pr_num}"); + ctx.db + .set_pr_status(&pr.repository, pr.number, PullRequestStatus::Merged) + .await?; + repo.client.post_comment(pr.number, comment).await?; + } + + Ok(()) +} + +/// Handle starting a new auto build for an approved PR. +/// Returns true if the queue should break, false to continue. +async fn handle_start_auto_build( + repo: &RepositoryState, + ctx: &BorsContext, + pr: &PullRequestModel, + pr_num: PullRequestNumber, +) -> anyhow::Result { + let Err(error) = start_auto_build(repo, ctx, pr).await else { + tracing::info!("Starting auto build for PR {pr_num}"); + return Ok(true); + }; + + match error { + StartAutoBuildError::MergeConflict => { + let gh_pr = repo.client.get_pull_request(pr.number).await?; + tracing::debug!("Failed to start auto build for PR {pr_num} due to merge conflict"); + + ctx.db + .update_pr_mergeable_state(pr, MergeableState::Unknown) + .await?; + repo.client + .post_comment(pr.number, merge_conflict_comment(&gh_pr.head.name)) + .await?; + Ok(false) + } + StartAutoBuildError::SanityCheckFailed(error) => { + tracing::info!("Sanity check failed for PR {pr_num}: {error:?}"); + Ok(true) + } + StartAutoBuildError::GitHubError(error) => { + tracing::debug!( + "Failed to start auto build for PR {pr_num} due to a GitHub error: {error:?}" + ); + Ok(true) + } + StartAutoBuildError::DatabaseError(error) => { + tracing::debug!( + "Failed to start auto build for PR {pr_num} due to database error: {error:?}" + ); + Ok(true) + } + } +} + #[must_use] pub enum StartAutoBuildError { /// Merge conflict between PR head and base branch. diff --git a/src/database/mod.rs b/src/database/mod.rs index ee99acec..189f1c26 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -181,6 +181,20 @@ pub struct ApprovalInfo { pub sha: String, } +#[derive(Debug, Clone, PartialEq)] +pub enum QueueStatus { + /// Approved with running auto build. + Pending(ApprovalInfo, BuildModel), + /// Approved with failed auto build. + Stalled(ApprovalInfo, BuildModel), + /// Approved with no auto build started yet or a failed auto build was reset + /// with `@bors retry`. + Approved(ApprovalInfo), + /// Approved with passing CI. + ReadyForMerge(ApprovalInfo, BuildModel), + NotApproved, +} + /// Represents the approval status of a pull request. #[derive(Debug, Clone, PartialEq)] pub enum ApprovalStatus { @@ -271,7 +285,7 @@ impl FromStr for DelegatedPermission { } /// Status of a GitHub build. -#[derive(Debug, PartialEq, sqlx::Type)] +#[derive(Debug, Clone, PartialEq, sqlx::Type)] #[sqlx(type_name = "TEXT")] #[sqlx(rename_all = "lowercase")] pub enum BuildStatus { @@ -309,7 +323,7 @@ impl Display for BuildStatus { } /// Represents a single (merged) commit. -#[derive(Debug, sqlx::Type)] +#[derive(Debug, Clone, PartialEq, sqlx::Type)] #[sqlx(type_name = "build")] pub struct BuildModel { pub id: PrimaryKey, @@ -382,13 +396,25 @@ impl PullRequestModel { } } - /// Approved but with a failed auto build. - pub fn is_stalled(&self) -> bool { - self.is_approved() - && self - .auto_build - .as_ref() - .is_some_and(|build| build.status.is_failure()) + /// Get the merge queue status of this pull request. + pub fn queue_status(&self) -> QueueStatus { + match &self.approval_status { + ApprovalStatus::NotApproved => QueueStatus::NotApproved, + ApprovalStatus::Approved(approval_info) => match &self.auto_build { + Some(build) => match build.status { + BuildStatus::Pending => { + QueueStatus::Pending(approval_info.clone(), build.clone()) + } + BuildStatus::Success => { + QueueStatus::ReadyForMerge(approval_info.clone(), build.clone()) + } + BuildStatus::Failure | BuildStatus::Cancelled | BuildStatus::Timeouted => { + QueueStatus::Stalled(approval_info.clone(), build.clone()) + } + }, + None => QueueStatus::Approved(approval_info.clone()), + }, + } } } diff --git a/src/github/server.rs b/src/github/server.rs index ed93c6b4..7968afcb 100644 --- a/src/github/server.rs +++ b/src/github/server.rs @@ -8,6 +8,7 @@ use crate::bors::{ BorsContext, CommandPrefix, RepositoryState, RollupMode, handle_bors_global_event, handle_bors_repository_event, }; +use crate::database::QueueStatus; use crate::github::webhook::GitHubWebhook; use crate::github::webhook::WebhookSecret; use crate::templates::{ @@ -150,16 +151,23 @@ async fn queue_handler( let prs = state.db.get_nonclosed_pull_requests(&repo.name).await?; - // TODO: add failed count - let (approved_count, rolled_up_count) = prs.iter().fold((0, 0), |(approved, rolled_up), pr| { - let is_approved = if pr.is_approved() { 1 } else { 0 }; - let is_rolled_up = if matches!(pr.rollup, Some(RollupMode::Always)) { - 1 - } else { - 0 - }; - (approved + is_approved, rolled_up + is_rolled_up) - }); + let (in_queue_count, failed_count, rolled_up_count) = + prs.iter() + .fold((0, 0, 0), |(in_queue, failed, rolled_up), pr| { + let (in_queue_inc, failed_inc) = match pr.queue_status() { + QueueStatus::Approved(..) => (1, 0), + QueueStatus::ReadyForMerge(..) => (1, 0), + QueueStatus::Pending(..) => (1, 0), + QueueStatus::Stalled(..) => (0, 1), + QueueStatus::NotApproved => (0, 0), + }; + + ( + in_queue + in_queue_inc, + failed + failed_inc, + rolled_up + usize::from(matches!(pr.rollup, Some(RollupMode::Always))), + ) + }); Ok(HtmlTemplate(QueueTemplate { repo_name: repo.name.name().to_string(), @@ -167,7 +175,8 @@ async fn queue_handler( tree_state: repo.tree_state, stats: PullRequestStats { total_count: prs.len(), - approved_count, + in_queue_count, + failed_count, rolled_up_count, }, prs, diff --git a/src/templates.rs b/src/templates.rs index b0904007..9e2c1be8 100644 --- a/src/templates.rs +++ b/src/templates.rs @@ -1,4 +1,4 @@ -use crate::database::{MergeableState::*, PullRequestModel, TreeState}; +use crate::database::{MergeableState::*, PullRequestModel, QueueStatus::*, TreeState}; use askama::Template; use axum::response::{Html, IntoResponse, Response}; use http::StatusCode; @@ -35,7 +35,8 @@ pub struct RepositoryView { pub struct PullRequestStats { pub total_count: usize, - pub approved_count: usize, + pub in_queue_count: usize, + pub failed_count: usize, pub rolled_up_count: usize, } diff --git a/templates/queue.html b/templates/queue.html index c837acd3..0f0b2a65 100644 --- a/templates/queue.html +++ b/templates/queue.html @@ -28,8 +28,8 @@

Help page

- {{ stats.total_count }} total, {{ stats.approved_count }} approved, - {{ stats.rolled_up_count }} rolled up + {{ stats.total_count }} total, {{ stats.in_queue_count }} in queue, + {{ stats.failed_count }} failed, {{ stats.rolled_up_count }} rolled up

@@ -55,9 +55,17 @@

{% if let Some(try_build) = pr.try_build %} {{ try_build.status }} (try) {% else %} - {% if pr.is_approved() %} + {% match pr.queue_status() %} + {% when Approved(_) %} approved - {% endif %} + {% when ReadyForMerge(_, _) %} + ready for merge + {% when Pending(_, _) %} + pending + {% when Stalled(_, _) %} + stalled + {% when NotApproved %} + {% endmatch %} {% endif %}