Apply approval labels eagerly when a PR is tentatively approved

src/bors/approval.rs

@@ -1,9 +1,7 @@
 use crate::bors::RepositoryState;
-use crate::bors::labels::handle_label_trigger;
-use crate::bors::merge_queue::MergeQueueSender;
 use crate::database::WorkflowStatus;
+use crate::github::PullRequest;
 use crate::github::api::client::WorkflowSource;
-use crate::github::{LabelTrigger, PullRequest};
 
 /// Note that can be attached to an approval comment.
 pub enum ApprovalNote {
@@ -13,19 +11,6 @@ pub enum ApprovalNote {
     TentativeApproval,
 }
 
-/// Perform post-approve actions.
-/// Should only be called if the PR is fully approved!
-///
-/// Notifies the merge queue and applies approval labels.
-pub(super) async fn finalize_approval(
-    repo: &RepositoryState,
-    pr: &PullRequest,
-    merge_queue_tx: &MergeQueueSender,
-) -> anyhow::Result<()> {
-    merge_queue_tx.notify().await?;
-    handle_label_trigger(repo, &pr.clone().into(), LabelTrigger::Approved).await
-}
-
 #[derive(Copy, Clone)]
 pub enum PrCiStatus {
     /// Waiting for PR CI to finish.

src/bors/approval_queue.rs

@@ -1,13 +1,14 @@
 use crate::BorsContext;
-use crate::bors::approval::{PrCiStatus, finalize_approval, get_pr_ci_status};
+use crate::bors::approval::{PrCiStatus, 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::unapprove_pr;
 use crate::bors::merge_queue::MergeQueueSender;
 use crate::bors::{PullRequestStatus, RepositoryState, elapsed_time_since};
 use crate::database::{ApprovalInfo, PullRequestModel};
-use crate::github::{GithubRepoName, PullRequest};
+use crate::github::{GithubRepoName, PullRequest, PullRequestInfo};
 use std::sync::Arc;
 use tokio::sync::mpsc;
 
@@ -181,7 +182,7 @@ async fn process_tentative_approval(
 
             // CI has timed out
             if elapsed_time_since(head_update_time) >= timeout {
-                ctx.db.unapprove(pr).await?;
+                unapprove_pr(repo, &ctx.db, pr, &PullRequestInfo::from(gh_pr.clone())).await?;
                 repo.client
                     .post_comment(
                         pr.number,
@@ -194,11 +195,18 @@ async fn process_tentative_approval(
         PrCiStatus::Success => {
             // CI is green! Confirm the approval
             ctx.db.confirm_tentative_approval(pr).await?;
-            finalize_approval(repo, &gh_pr, merge_queue_tx).await?;
+            // Let the merge queue know
+            merge_queue_tx.notify().await?;
+
+            // Labels were already applied when tentatively approving the PR, so no need to modify
+            // them further
         }
         PrCiStatus::Failed => {
-            // CI has failed
-            ctx.db.unapprove(pr).await?;
+            // CI has failed, unapprove the PR
+            // Note: if we allow tentatively PRs to become parts of a rollup, this should ideally
+            // call invalidate_pr instead.
+            unapprove_pr(repo, &ctx.db, pr, &PullRequestInfo::from(gh_pr.clone())).await?;
+
             repo.client
                 .post_comment(
                     pr.number,
@@ -396,18 +404,21 @@ mod tests {
     }
 
     #[sqlx::test(migrator = "crate::MIGRATOR")]
-    async fn approval_confirmation_adds_labels(pool: sqlx::PgPool) {
+    async fn pr_ci_failure_applies_unapprove_labels(pool: sqlx::PgPool) {
         let gh = GitHub::default().append_to_default_config(
             r#"
 [labels]
 approved = ["+approved"]
+unapproved = ["-approved", "+unapproved"]
 "#,
         );
         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"]);
+            ctx.pr_workflow_failure(workflow).await?;
+            ctx.expect_comments((), 1).await;
+
+            ctx.pr(()).await.expect_labels(&["unapproved"]);
             Ok(())
         })
         .await;

src/bors/handlers/review.rs

@@ -1,5 +1,5 @@
 use crate::bors::RepositoryState;
-use crate::bors::approval::{ApprovalNote, PrCiStatus, finalize_approval, get_pr_ci_status};
+use crate::bors::approval::{ApprovalNote, PrCiStatus, get_pr_ci_status};
 use crate::bors::command::{Approver, CommandPrefix, Delegatee};
 use crate::bors::command::{DelegateCommand, RollupMode};
 use crate::bors::comment::{
@@ -9,12 +9,13 @@ use crate::bors::comment::{
 };
 use crate::bors::handlers::{InvalidationInfo, InvalidationReason, PullRequestData, deny_request};
 use crate::bors::handlers::{has_permission, invalidate_pr};
+use crate::bors::labels::handle_label_trigger;
 use crate::bors::merge_queue::MergeQueueSender;
 use crate::bors::{Comment, PullRequestStatus};
 use crate::database::DelegatedPermission;
 use crate::database::{ApprovalInfo, ApprovalMode, PullRequestModel};
 use crate::database::{MergeableState, TreeState};
-use crate::github::{CommitSha, PullRequest};
+use crate::github::{CommitSha, LabelTrigger, PullRequest, PullRequestInfo};
 use crate::github::{GithubUser, PullRequestNumber};
 use crate::permissions::PermissionType;
 use crate::{BorsContext, PgDbClient, ZulipClient};
@@ -173,13 +174,16 @@ pub(super) async fn command_approve(
         )
         .await?;
 
-    match approval_mode {
-        ApprovalMode::Eager => {
-            finalize_approval(&repo, pr.github, merge_queue_tx).await?;
-        }
-        ApprovalMode::Tentative => {}
-    }
-    Ok(())
+    merge_queue_tx.notify().await?;
+
+    // Eagerly apply label changes, even if we are only in a tentative approval, so that the PR
+    // gets out of the reviewer's GitHub queue
+    handle_label_trigger(
+        &repo,
+        &PullRequestInfo::from(pr.github.clone()),
+        LabelTrigger::Approved,
+    )
+    .await
 }
 
 /// Check if the specified approvers exist as GitHub users or teams.
@@ -807,23 +811,6 @@ approved = ["+approved"]
         .await;
     }
 
-    #[sqlx::test(migrator = "crate::MIGRATOR")]
-    async fn tentative_approve_doesnt_add_labels(pool: sqlx::PgPool) {
-        let gh = GitHub::default().append_to_default_config(
-            r#"
-[labels]
-approved = ["+approved"]
-"#,
-        );
-        run_test((pool, gh), async |ctx: &mut BorsTester| {
-            ctx.pr_ci_workflow(());
-            ctx.approve(()).await?;
-            ctx.pr(()).await.expect_labels(&[]);
-            Ok(())
-        })
-        .await;
-    }
-
     #[sqlx::test(migrator = "crate::MIGRATOR")]
     async fn approve_on_behalf(pool: sqlx::PgPool) {
         let approve_user_id = 200;
@@ -2254,6 +2241,23 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri
             .await;
     }
 
+    #[sqlx::test(migrator = "crate::MIGRATOR")]
+    async fn tentative_approval_eagerly_applies_labels(pool: sqlx::PgPool) {
+        let gh = GitHub::default().append_to_default_config(
+            r#"
+[labels]
+approved = ["+approved"]
+"#,
+        );
+        run_test((pool, gh), async |ctx: &mut BorsTester| {
+            ctx.pr_ci_workflow(());
+            ctx.approve(()).await?;
+            ctx.pr(()).await.expect_added_labels(&["approved"]);
+            Ok(())
+        })
+        .await;
+    }
+
     #[sqlx::test(migrator = "crate::MIGRATOR")]
     async fn unapprove_running_auto_build_pr_comment(pool: sqlx::PgPool) {
         run_test(pool, async |ctx: &mut BorsTester| {