Merge pull request #367 from rust-lang/pr-approval-labels

Forbid approving PRs with labels blocking approval

Author: Kobzol

rust-bors.example.toml

@@ -19,3 +19,7 @@ approve = ["+approved"]
 try = ["+foo", "-bar"]
 try_succeed = ["+foobar", "+foo", "+baz"]
 try_failed = []
+
+# Labels that will block approval when present on a PR
+# (Optional)
+labels_blocking_approval = ["final-comment-period", "proposed-final-comment-period"]

src/bors/comment.rs

@@ -215,7 +215,7 @@ handled during merge and rebase. This is normal, and you should still perform st
 }
 
 pub fn approve_non_open_pr_comment() -> Comment {
-    Comment::new("Only open, non-draft PRs can be approved".to_string())
+    Comment::new(":clipboard: Only open, non-draft PRs can be approved.".to_string())
 }
 
 pub fn approve_wip_title(keyword: &str) -> Comment {
@@ -227,6 +227,17 @@ Hint: Remove **{keyword}** from this PR's title when it is ready for review.
     ))
 }
 
+pub fn approve_blocking_labels_present(blocking_labels: &[&str]) -> Comment {
+    let labels = blocking_labels
+        .iter()
+        .map(|label| format!("`{label}`"))
+        .join(", ");
+    Comment::new(format!(
+        ":clipboard: This PR cannot be approved because it currently has the following {}: {labels}.",
+        pluralize("label", blocking_labels.len())
+    ))
+}
+
 pub fn delegate_try_builds_comment(delegatee: &str, bot_prefix: &CommandPrefix) -> Comment {
     Comment::new(format!(
         r":v: @{delegatee}, you can now perform try builds on this pull request!

src/bors/handlers/review.rs

@@ -5,7 +5,8 @@ use crate::bors::RepositoryState;
 use crate::bors::command::RollupMode;
 use crate::bors::command::{Approver, CommandPrefix};
 use crate::bors::comment::{
-    approve_non_open_pr_comment, approve_wip_title, delegate_comment, delegate_try_builds_comment,
+    approve_blocking_labels_present, approve_non_open_pr_comment, approve_wip_title,
+    delegate_comment, delegate_try_builds_comment,
 };
 use crate::bors::handlers::has_permission;
 use crate::bors::handlers::labels::handle_label_trigger;
@@ -38,7 +39,7 @@ pub(super) async fn command_approve(
         return Ok(());
     };
 
-    if let Some(error_comment) = check_pr_approval_validity(pr).await? {
+    if let Some(error_comment) = check_pr_approval_validity(pr, &repo_state).await? {
         repo_state
             .client
             .post_comment(pr.number(), error_comment)
@@ -69,7 +70,10 @@ const WIP_KEYWORDS: &[&str] = &["wip", "[do not merge]"];
 /// Check that the given PR can be approved in its current state.
 /// Returns `Ok(Some(comment))` if it **cannot** be approved; the comment should be sent to the
 /// pull request.
-async fn check_pr_approval_validity(pr: &PullRequestData) -> anyhow::Result<Option<Comment>> {
+async fn check_pr_approval_validity(
+    pr: &PullRequestData,
+    repo: &RepositoryState,
+) -> anyhow::Result<Option<Comment>> {
     // Check PR status
     if !matches!(pr.github.status, PullRequestStatus::Open) {
         return Ok(Some(approve_non_open_pr_comment()));
@@ -81,6 +85,24 @@ async fn check_pr_approval_validity(pr: &PullRequestData) -> anyhow::Result<Opti
         return Ok(Some(approve_wip_title(wip_kw)));
     }
 
+    // Check blocking labels
+    let config = repo.config.load();
+    let blocking_labels: Vec<&str> = pr
+        .github
+        .labels
+        .iter()
+        .map(|label| label.as_str())
+        .filter(|label| {
+            config
+                .labels_blocking_approval
+                .iter()
+                .any(|blocking_label| blocking_label == label)
+        })
+        .collect();
+    if !blocking_labels.is_empty() {
+        return Ok(Some(approve_blocking_labels_present(&blocking_labels)));
+    }
+
     Ok(None)
 }
 
@@ -1102,7 +1124,7 @@ mod tests {
                 .set_pr_status_draft(default_repo_name(), default_pr_number())
                 .await?;
             tester.post_comment("@bors r+").await?;
-            insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
+            insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: Only open, non-draft PRs can be approved.");
             tester.default_pr().await.expect_unapproved();
             Ok(())
         })
@@ -1116,7 +1138,7 @@ mod tests {
                 .set_pr_status_closed(default_repo_name(), default_pr_number())
                 .await?;
             tester.post_comment("@bors r+").await?;
-            insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
+            insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: Only open, non-draft PRs can be approved.");
             tester.default_pr().await.expect_unapproved();
             Ok(())
         })
@@ -1130,7 +1152,7 @@ mod tests {
                 .set_pr_status_merged(default_repo_name(), default_pr_number())
                 .await?;
             tester.post_comment("@bors r+").await?;
-            insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
+            insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: Only open, non-draft PRs can be approved.");
             tester.default_pr().await.expect_unapproved();
             Ok(())
         })
@@ -1156,4 +1178,56 @@ mod tests {
         })
         .await;
     }
+
+    #[sqlx::test]
+    async fn approve_pr_with_blocked_label(pool: sqlx::PgPool) {
+        BorsBuilder::new(pool)
+            .github(GitHubState::default().with_default_config(
+                r#"
+labels_blocking_approval = ["proposed-final-comment-period"]
+"#,
+            ))
+            .run_test(async |tester: &mut BorsTester| {
+                tester
+                    .edit_pr(default_repo_name(), default_pr_number(), |pr| {
+                        pr.labels = vec![
+                            "S-waiting-on-review".to_string(),
+                            "proposed-final-comment-period".to_string(),
+                        ];
+                    })
+                    .await?;
+                tester.post_comment("@bors r+").await?;
+                insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: This PR cannot be approved because it currently has the following label: `proposed-final-comment-period`.");
+                tester.default_pr().await.expect_unapproved();
+                Ok(())
+            })
+            .await;
+    }
+
+    #[sqlx::test]
+    async fn approve_pr_with_blocked_labels(pool: sqlx::PgPool) {
+        BorsBuilder::new(pool)
+            .github(GitHubState::default().with_default_config(
+                r#"
+labels_blocking_approval = ["proposed-final-comment-period", "final-comment-period"]
+"#,
+            ))
+            .run_test(async |tester: &mut BorsTester| {
+                tester
+                    .edit_pr(default_repo_name(), default_pr_number(), |pr| {
+                        pr.labels = vec![
+                            "S-waiting-on-review".to_string(),
+                            "proposed-final-comment-period".to_string(),
+                            "final-comment-period".to_string(),
+                            "S-blocked".to_string(),
+                        ];
+                    })
+                    .await?;
+                tester.post_comment("@bors r+").await?;
+                insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: This PR cannot be approved because it currently has the following labels: `proposed-final-comment-period`, `final-comment-period`.");
+                tester.default_pr().await.expect_unapproved();
+                Ok(())
+            })
+            .await;
+    }
 }

src/config.rs

@@ -11,6 +11,7 @@ pub const CONFIG_FILE_PATH: &str = "rust-bors.toml";
 /// Configuration of a repository loaded from a `rust-bors.toml`
 /// file located in the root of the repository file tree.
 #[derive(serde::Deserialize, Debug)]
+#[serde(deny_unknown_fields)]
 pub struct RepositoryConfig {
     /// Maximum duration (in seconds) to wait for CI checks to complete before timing out.
     /// Defaults to 3600 seconds (1 hour).
@@ -24,6 +25,9 @@ pub struct RepositoryConfig {
     /// Format: `trigger = ["+label_to_add", "-label_to_remove"]`
     #[serde(default, deserialize_with = "deserialize_labels")]
     pub labels: HashMap<LabelTrigger, Vec<LabelModification>>,
+    /// Labels that will block a PR from being approved when present on the PR.
+    #[serde(default)]
+    pub labels_blocking_approval: Vec<String>,
     /// Minimum time (in seconds) to wait for CI checks to complete before proceeding.
     /// Defaults to `None` (no minimum wait time).
     #[serde(default, deserialize_with = "deserialize_duration_from_secs_opt")]
@@ -253,6 +257,25 @@ try = ["foo"]
         load_config(content);
     }
 
+    #[test]
+    fn deserialize_labels_blocking_approval() {
+        let content = r#"labels_blocking_approval = ["foo", "bar"]"#;
+        let config = load_config(content);
+        insta::assert_debug_snapshot!(config.labels_blocking_approval, @r#"
+        [
+            "foo",
+            "bar",
+        ]
+        "#);
+    }
+
+    #[test]
+    #[should_panic(expected = "unknown field `labels-blocking-approval`")]
+    fn deserialize_unknown_key_fail() {
+        let content = r#"labels-blocking-approval = ["foo", "bar"]"#;
+        load_config(content);
+    }
+
     fn load_config(config: &str) -> RepositoryConfig {
         toml::from_str(config).unwrap()
     }

src/github/mod.rs

@@ -121,6 +121,7 @@ pub struct PullRequest {
     pub author: GithubUser,
     pub assignees: Vec<GithubUser>,
     pub status: PullRequestStatus,
+    pub labels: Vec<String>,
 }
 
 impl From<octocrab::models::pulls::PullRequest> for PullRequest {
@@ -157,6 +158,12 @@ impl From<octocrab::models::pulls::PullRequest> for PullRequest {
             } else {
                 PullRequestStatus::Open
             },
+            labels: pr
+                .labels
+                .unwrap_or_default()
+                .into_iter()
+                .map(|l| l.name)
+                .collect(),
         }
     }
 }

src/github/webhook.rs

@@ -607,6 +607,11 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Open,
+                                labels: [
+                                    "foo",
+                                    "foobar",
+                                    "baz",
+                                ],
                             },
                             from_base_sha: Some(
                                 CommitSha(
@@ -679,6 +684,11 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Open,
+                                labels: [
+                                    "foo",
+                                    "foobar",
+                                    "baz",
+                                ],
                             },
                         },
                     ),
@@ -795,6 +805,7 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Open,
+                                labels: [],
                             },
                             draft: false,
                         },
@@ -863,6 +874,9 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Closed,
+                                labels: [
+                                    "foo",
+                                ],
                             },
                         },
                     ),
@@ -930,6 +944,7 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Merged,
+                                labels: [],
                             },
                         },
                     ),
@@ -997,6 +1012,9 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Open,
+                                labels: [
+                                    "foo",
+                                ],
                             },
                         },
                     ),
@@ -1064,6 +1082,7 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Draft,
+                                labels: [],
                             },
                             draft: true,
                         },
@@ -1132,6 +1151,7 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Draft,
+                                labels: [],
                             },
                         },
                     ),
@@ -1199,6 +1219,7 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Open,
+                                labels: [],
                             },
                         },
                     ),
@@ -1337,6 +1358,7 @@ mod tests {
                                     },
                                 ],
                                 status: Open,
+                                labels: [],
                             },
                         },
                     ),
@@ -1404,6 +1426,7 @@ mod tests {
                                 },
                                 assignees: [],
                                 status: Open,
+                                labels: [],
                             },
                         },
                     ),