Merge pull request #858 from Kobzol/approval-upgrade

Upgrade tentative approvals if the PR was already fully approved

Author: Kobzol

src/bors/approval.rs

@@ -18,6 +18,15 @@ pub(super) fn check_unknown_reviewers(repo: &RepositoryState, approvers: &str) -
         .collect()
 }
 
+/// 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,
+}
+
 /// Finalize an approval and prepare the pull request for the merge queue.
 ///
 /// Clears any failed auto build so it can be retried, wakes the merge queue, posts the approval
@@ -30,17 +39,17 @@ pub(super) async fn finalize_approval(
     approver: &str,
     priority: Option<u32>,
     merge_queue_tx: &MergeQueueSender,
-    failed_pr_ci: bool,
+    note: Option<ApprovalNote>,
 ) -> anyhow::Result<()> {
     let unknown_reviewers = check_unknown_reviewers(repo, approver);
-    let was_failed = pr
+    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 was_failed {
+    if had_failed_auto_build {
         ctx.db.clear_auto_build(pr.db).await?;
     }
 
@@ -74,8 +83,8 @@ pub(super) async fn finalize_approval(
                 approver,
                 unknown_reviewers,
                 tree_state,
-                was_failed,
-                failed_pr_ci,
+                had_failed_auto_build,
+                note,
             ),
             &ctx.db,
         )
@@ -142,6 +151,6 @@ pub(super) async fn try_resolve_tentative_approval(
     }
 
     ctx.db.confirm_tentative_approval(pr.db).await?;
-    finalize_approval(ctx, repo, pr, approver, priority, merge_queue_tx, false).await?;
+    finalize_approval(ctx, repo, pr, approver, priority, merge_queue_tx, None).await?;
     Ok(TentativeApprovalOutcome::Resolved)
 }

src/bors/comment.rs

@@ -1,3 +1,4 @@
+use crate::bors::approval::ApprovalNote;
 use crate::bors::command::CommandPrefix;
 use crate::bors::{FailedWorkflowRun, WorkflowRun};
 use crate::database::PullRequestModel;
@@ -296,8 +297,8 @@ pub fn approved_comment(
     reviewer: &str,
     unknown_reviewers: Vec<String>,
     tree_state: TreeState,
-    was_failed: bool,
-    failed_pr_ci: bool,
+    had_failed_auto_build: bool,
+    note: Option<ApprovalNote>,
 ) -> Comment {
     let approve_emoji = if is_holiday_season() {
         "star2"
@@ -312,20 +313,38 @@ It is now in the [queue]({web_url}/queue/{}) for this repository.
         repo.name()
     );
 
-    if was_failed {
+    if had_failed_auto_build {
         writeln!(
             comment,
             "\nA failed build status on this PR was cleared due to the approval."
         )
         .unwrap();
     }
 
-    if failed_pr_ci {
-        writeln!(
-            comment,
-            "\n> [!WARNING]\n> This PR was force-approved despite failing PR CI."
-        )
-        .unwrap();
+    if let Some(note) = note {
+        match note {
+            ApprovalNote::PrCiIsFailing => {
+                writeln!(
+                    comment,
+                    "\n> [!WARNING]\n> This PR was force-approved despite failing PR CI."
+                )
+                .unwrap();
+            }
+            ApprovalNote::TentativeApprovalUpgraded => {
+                writeln!(
+                    comment,
+                    r#"
+> [!WARNING]
+> This PR was already fully approved previously, so this tentative approval was treated as a full approval. If you want to instead downgrade the PR to be only tentatively approved, unapprove it first and then re-approve it again:
+> ```
+> @bors r-
+> @bors r+
+> ```
+"#
+                )
+                .unwrap();
+            }
+        }
     }
 
     if !unknown_reviewers.is_empty() {

src/bors/handlers/review.rs

@@ -1,6 +1,6 @@
 use crate::bors::RepositoryState;
 use crate::bors::approval::{
-    TentativeApprovalOutcome, check_unknown_reviewers, finalize_approval,
+    ApprovalNote, TentativeApprovalOutcome, check_unknown_reviewers, finalize_approval,
     try_resolve_tentative_approval,
 };
 use crate::bors::command::{Approver, CommandPrefix, Delegatee};
@@ -84,6 +84,18 @@ pub(super) async fn command_approve(
         sha: pr.github.head.sha.to_string(),
     };
 
+    // It is possible that the PR was already (fully) approved before.
+    // If we are now doing a tentative approval, we will "upgrade" it to a full approval, which is
+    // usually what the user wants.
+    // This situation should be very rare anyway.
+    let already_approved = pr.db.is_approved();
+    let (approval_mode, approval_upgraded) =
+        if already_approved && matches!(approval_mode, ApprovalMode::Tentative) {
+            (ApprovalMode::Eager, true)
+        } else {
+            (approval_mode, false)
+        };
+
     db.approve(
         pr.db,
         approval_info,
@@ -130,18 +142,27 @@ pub(super) async fn command_approve(
             Ok(())
         }
         ApprovalMode::Eager => {
-            let failed_pr_ci = match repo_state
-                .client
-                .get_workflow_runs_for_commit_sha(WorkflowSource::PullRequest(pr.github))
-                .await
-            {
-                Ok(runs) => runs.iter().any(|run| run.status == WorkflowStatus::Failure),
-                Err(error) => {
-                    tracing::warn!(
-                        "Failed to get pull request CI status for commit {}: {error:?}",
-                        pr.github.head.sha
-                    );
-                    false
+            let note = if approval_upgraded {
+                Some(ApprovalNote::TentativeApprovalUpgraded)
+            } else {
+                let pr_ci_fails = match repo_state
+                    .client
+                    .get_workflow_runs_for_commit_sha(WorkflowSource::PullRequest(pr.github))
+                    .await
+                {
+                    Ok(runs) => runs.iter().any(|run| run.status == WorkflowStatus::Failure),
+                    Err(error) => {
+                        tracing::warn!(
+                            "Failed to get pull request CI status for commit {}: {error:?}",
+                            pr.github.head.sha
+                        );
+                        false
+                    }
+                };
+                if pr_ci_fails {
+                    Some(ApprovalNote::PrCiIsFailing)
+                } else {
+                    None
                 }
             };
             finalize_approval(
@@ -151,7 +172,7 @@ pub(super) async fn command_approve(
                 &approver,
                 priority,
                 merge_queue_tx,
-                failed_pr_ci,
+                note,
             )
             .await
         }
@@ -2302,7 +2323,7 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri
             ctx.start_auto_build(()).await?;
             ctx.workflow_full_failure(ctx.auto_workflow()).await?;
             ctx.expect_comments((), 1).await; // build failed
-            ctx.post_comment("@bors r+").await?;
+            ctx.post_comment("@bors r+ force").await?;
             insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @"
             :pushpin: Commit pr-1-sha has been approved by `default-user`
 
@@ -2316,4 +2337,37 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri
         })
         .await;
     }
+
+    #[sqlx::test(migrator = "crate::MIGRATOR")]
+    async fn tentative_approval_upgrade(pool: sqlx::PgPool) {
+        run_test(pool, async |ctx: &mut BorsTester| {
+            // Full approval
+            ctx.post_comment("@bors r+ force").await?;
+            ctx.expect_comments((), 1).await;
+
+            // Cause PR CI to fail
+            ctx.pr_workflow_failure(ctx.pr_ci_workflow(())).await?;
+
+            // Tentative approval
+            ctx.post_comment("@bors r+").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.
+
+            > [!WARNING]
+            > This PR was already fully approved previously, so this tentative approval was treated as a full approval. If you want to instead downgrade the PR to be only tentatively approved, unapprove it first and then re-approve it again:
+            > ```
+            > @bors r-
+            > @bors r+
+            > ```
+            ");
+
+            // The tentative approval should not unapprove the PR
+            ctx.pr(()).await.expect_approved_by(&User::default_pr_author().name);
+
+            Ok(())
+        })
+        .await;
+    }
 }