Skip to content

Make tentative approvals less obtrusive #859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kobzol merged 1 commit into from
Sep 25, 2026
Merged

Make tentative approvals less obtrusive #859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 28 additions & 117 deletions src/bors/approval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,156 +1,67 @@
use crate::bors::comment::approved_comment;
use crate::bors::handlers::PullRequestData;
use crate::bors::RepositoryState;
use crate::bors::labels::handle_label_trigger;
use crate::bors::merge_queue::MergeQueueSender;
use crate::bors::{BorsContext, Comment, RepositoryState};
use crate::database::{TreeState, WorkflowStatus};
use crate::github::LabelTrigger;
use crate::database::WorkflowStatus;
use crate::github::api::client::WorkflowSource;

/// Check if the specified approvers exist as GitHub users or teams.
pub(super) fn check_unknown_reviewers(repo: &RepositoryState, approvers: &str) -> Vec<String> {
let directory = repo.permissions.load();

approvers
.split(',')
.filter(|approver| !directory.user_exists(approver) && !directory.team_exists(approver))
.map(str::to_string)
.collect()
}
use crate::github::{LabelTrigger, PullRequest};

/// Note that can be attached to an approval comment.
pub enum ApprovalNote {
/// The pull request was force approved while its PR CI is failing.
PrCiIsFailing,
/// A previously fully approved PR was approved again *tentatively*.
/// This tentative approval was automatically upgraded to a full approval.
TentativeApprovalUpgraded,
/// The PR was approved tentatively.
TentativeApproval,
}

/// Finalize an approval and prepare the pull request for the merge queue.
/// Perform post-approve actions.
/// Should only be called if the PR is fully approved!
///
/// Clears any failed auto build so it can be retried, wakes the merge queue, posts the approval
/// comment, and applies label changes configured for approved pull requests.
#[allow(clippy::too_many_arguments)]
/// Notifies the merge queue and applies approval labels.
pub(super) async fn finalize_approval(
ctx: &BorsContext,
repo: &RepositoryState,
pr: PullRequestData<'_>,
approver: &str,
priority: Option<u32>,
pr: &PullRequest,
merge_queue_tx: &MergeQueueSender,
note: Option<ApprovalNote>,
) -> anyhow::Result<()> {
let unknown_reviewers = check_unknown_reviewers(repo, approver);
let had_failed_auto_build = pr
.db
.auto_build
.as_ref()
.map(|b| b.status.is_failure())
.unwrap_or(false);
// Re-approval should act as a retry
if had_failed_auto_build {
ctx.db.clear_auto_build(pr.db).await?;
}

merge_queue_tx.notify().await?;

let mut tree_state = ctx
.db
.repo_db(repo.repository())
.await?
.map(|r| r.tree_state.clone())
.unwrap_or(TreeState::Open);

// If the PR has high enough priority, do not post the tree closed message
if let TreeState::Closed {
priority: tree_priority,
..
} = &tree_state
&& let Some(priority) = priority
&& priority >= *tree_priority
{
tree_state = TreeState::Open;
}

repo.client
.post_comment(
pr.db.number,
approved_comment(
ctx.get_web_url(),
repo.repository(),
&pr.github.head.sha,
approver,
unknown_reviewers,
tree_state,
had_failed_auto_build,
note,
),
&ctx.db,
)
.await?;
handle_label_trigger(repo, &pr.github.clone().into(), LabelTrigger::Approved).await
handle_label_trigger(repo, &pr.clone().into(), LabelTrigger::Approved).await
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub(super) enum TentativeApprovalOutcome {
/// The tentative approval was confirmed or rejected.
Resolved,
/// Still waiting for some workflows to be finished.
#[derive(Copy, Clone)]
pub enum PrCiStatus {
/// Waiting for PR CI to finish.
Pending,
/// There was some transient error, the tentative approval should be resolved later.
Skipped,
/// PR CI has finished successfully.
Success,
/// PR CI has failed.
Failed,
}

/// Try to resolve a tentative approval from the current PR CI state.
#[allow(clippy::too_many_arguments)]
pub(super) async fn try_resolve_tentative_approval(
ctx: &BorsContext,
pub async fn get_pr_ci_status(
repo: &RepositoryState,
pr: PullRequestData<'_>,
approver: &str,
failure_comment: Comment,
priority: Option<u32>,
merge_queue_tx: &MergeQueueSender,
) -> anyhow::Result<TentativeApprovalOutcome> {
let workflow_runs = match repo
pr: &PullRequest,
) -> anyhow::Result<PrCiStatus> {
let workflow_runs = repo
.client
.get_workflow_runs_for_commit_sha(WorkflowSource::PullRequest(pr.github))
.await
{
Ok(workflow_runs) => workflow_runs,
Err(error) => {
tracing::error!(
"Failed to get pull request CI status for commit {}: {error:?}",
pr.github.head.sha
);
return Ok(TentativeApprovalOutcome::Skipped);
}
};
.get_workflow_runs_for_commit_sha(WorkflowSource::PullRequest(pr))
.await?;

if workflow_runs.is_empty() {
return Ok(TentativeApprovalOutcome::Pending);
return Ok(PrCiStatus::Pending);
}

if workflow_runs
.iter()
.any(|run| run.status == WorkflowStatus::Failure)
{
ctx.db.unapprove(pr.db).await?;
repo.client
.post_comment(pr.number(), failure_comment, &ctx.db)
.await?;
return Ok(TentativeApprovalOutcome::Resolved);
return Ok(PrCiStatus::Failed);
}

if workflow_runs
.iter()
.any(|run| run.status == WorkflowStatus::Pending)
{
return Ok(TentativeApprovalOutcome::Pending);
Ok(PrCiStatus::Pending)
} else {
Ok(PrCiStatus::Success)
}

ctx.db.confirm_tentative_approval(pr.db).await?;
finalize_approval(ctx, repo, pr, approver, priority, merge_queue_tx, None).await?;
Ok(TentativeApprovalOutcome::Resolved)
}
102 changes: 56 additions & 46 deletions src/bors/approval_queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use crate::BorsContext;
use crate::bors::approval::{TentativeApprovalOutcome, try_resolve_tentative_approval};
use crate::bors::approval::{PrCiStatus, finalize_approval, get_pr_ci_status};
use crate::bors::comment::{
tentative_approval_removed_comment, tentative_approval_timed_out_comment,
};
use crate::bors::event::WorkflowRunCompleted;
use crate::bors::handlers::PullRequestData;
use crate::bors::merge_queue::MergeQueueSender;
use crate::bors::{PullRequestStatus, RepositoryState, elapsed_time_since};
use crate::database::{ApprovalInfo, PullRequestModel};
Expand Down Expand Up @@ -84,6 +83,9 @@ pub async fn handle_approval_queue_event(
}
}
ApprovalQueueEvent::OnWorkflowCompleted(event) => {
// We have to scan all open PRs, because we sadly cannot easily get the PR number
// from a pull_request workflow event :(
// We could look it up in the DB via the HEAD SHA, but that seems like overkill for now.
let handle = async {
let repo = ctx.get_repo(&event.repository)?;
let pull_requests = ctx
Expand Down Expand Up @@ -159,23 +161,9 @@ async fn process_tentative_approval(
return Ok(());
}

match try_resolve_tentative_approval(
ctx,
repo,
PullRequestData {
github: &gh_pr,
db: pr,
},
&approval_info.approver,
tentative_approval_removed_comment(&gh_pr.head.sha),
pr.priority.map(|priority| priority as u32),
merge_queue_tx,
)
.await?
{
TentativeApprovalOutcome::Resolved => {}
TentativeApprovalOutcome::Skipped => {}
TentativeApprovalOutcome::Pending => {
let pr_ci_status = get_pr_ci_status(repo, &gh_pr).await?;
match pr_ci_status {
PrCiStatus::Pending => {
let Some(head_update_time) = repo
.client
.get_pull_request_head_update_time(&gh_pr)
Expand All @@ -190,6 +178,8 @@ async fn process_tentative_approval(
};

let timeout = repo.config.load().pr_ci_timeout;

// CI has timed out
if elapsed_time_since(head_update_time) >= timeout {
ctx.db.unapprove(pr).await?;
repo.client
Expand All @@ -201,6 +191,22 @@ async fn process_tentative_approval(
.await?;
}
}
PrCiStatus::Success => {
// CI is green! Confirm the approval
ctx.db.confirm_tentative_approval(pr).await?;
finalize_approval(repo, &gh_pr, merge_queue_tx).await?;
}
PrCiStatus::Failed => {
// CI has failed
ctx.db.unapprove(pr).await?;
repo.client
.post_comment(
pr.number,
tentative_approval_removed_comment(&gh_pr.head.sha),
&ctx.db,
)
.await?;
}
}

Ok(())
Expand All @@ -210,7 +216,7 @@ async fn process_tentative_approval(
mod tests {
use crate::bors::with_mocked_time;
use crate::database::WorkflowStatus;
use crate::tests::{BorsTester, Commit, User, WorkflowEvent, run_test};
use crate::tests::{BorsTester, Commit, GitHub, User, run_test};
use std::time::Duration;

#[sqlx::test(migrator = "crate::MIGRATOR")]
Expand All @@ -219,13 +225,12 @@ mod tests {
let workflow = ctx.pr_ci_workflow(());
ctx.approve(()).await?;

ctx.workflow_event(WorkflowEvent::success(workflow)).await?;

insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @"
:pushpin: Commit pr-1-sha has been approved by `default-user`
ctx.pr(())
.await
.expect_unapproved()
.expect_tentative_approval();

It is now in the [queue](https://bors-test.com/queue/borstest) for this repository.
");
ctx.pr_workflow_success(workflow).await?;
ctx.pr(())
.await
.expect_approved_by(&User::default_pr_author().name);
Expand All @@ -240,11 +245,9 @@ mod tests {
let workflow = ctx.pr_ci_workflow(());
ctx.approve(()).await?;

ctx.workflow_event(WorkflowEvent::failure(workflow)).await?;
ctx.pr_workflow_failure(workflow).await?;

insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @"
:x: Tentatively approved commit pr-1-sha has been unapproved due to PR CI failure.
");
insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Commit pr-1-sha has been unapproved due to PR CI failure. Reapprove it with `@bors r+ force` if you want to ignore the failure.");
ctx.pr(()).await.expect_unapproved();
Ok(())
})
Expand All @@ -261,11 +264,6 @@ mod tests {

ctx.refresh_tentative_approvals().await;

insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @"
:pushpin: Commit pr-1-sha has been approved by `default-user`

It is now in the [queue](https://bors-test.com/queue/borstest) for this repository.
");
ctx.pr(())
.await
.expect_approved_by(&User::default_pr_author().name);
Expand All @@ -284,9 +282,7 @@ mod tests {

ctx.refresh_tentative_approvals().await;

insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @"
:x: Tentatively approved commit pr-1-sha has been unapproved due to PR CI failure.
");
insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Commit pr-1-sha has been unapproved due to PR CI failure. Reapprove it with `@bors r+ force` if you want to ignore the failure.");
ctx.pr(()).await.expect_unapproved();
Ok(())
})
Expand All @@ -303,7 +299,8 @@ mod tests {

ctx.pr(())
.await
.expect_approver(&User::default_pr_author().name);
.expect_approver(&User::default_pr_author().name)
.expect_tentative_approval();
Ok(())
})
.await;
Expand All @@ -320,7 +317,7 @@ mod tests {
})
.await;

insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Tentatively approved commit pr-1-sha has been unapproved because PR CI timed out after `7200`s.");
insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Commit pr-1-sha has been unapproved because PR CI timed out after `7200s`.");
ctx.pr(()).await.expect_unapproved();
Ok(())
})
Expand All @@ -338,7 +335,7 @@ mod tests {
})
.await;

insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Tentatively approved commit pr-1-sha has been unapproved because PR CI timed out after `7200`s.");
insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Commit pr-1-sha has been unapproved because PR CI timed out after `7200s`.");
ctx.pr(()).await.expect_unapproved();
Ok(())
})
Expand All @@ -358,11 +355,6 @@ mod tests {
})
.await;

insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @"
:pushpin: Commit pr-1-sha has been approved by `default-user`

It is now in the [queue](https://bors-test.com/queue/borstest) for this repository.
");
ctx.pr(())
.await
.expect_approved_by(&User::default_pr_author().name);
Expand Down Expand Up @@ -402,4 +394,22 @@ mod tests {
})
.await;
}

#[sqlx::test(migrator = "crate::MIGRATOR")]
async fn approval_confirmation_adds_labels(pool: sqlx::PgPool) {
let gh = GitHub::default().append_to_default_config(
r#"
[labels]
approved = ["+approved"]
"#,
);
run_test((pool, gh), async |ctx: &mut BorsTester| {
let workflow = ctx.pr_ci_workflow(());
ctx.approve(()).await?;
ctx.pr_workflow_success(workflow).await?;
ctx.pr(()).await.expect_added_labels(&["approved"]);
Ok(())
})
.await;
}
}
Loading
Loading