diff --git a/rust-bors.example.toml b/rust-bors.example.toml index f739e63e..dc3247f4 100644 --- a/rust-bors.example.toml +++ b/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"] diff --git a/src/bors/comment.rs b/src/bors/comment.rs index af7643e7..546c58a6 100644 --- a/src/bors/comment.rs +++ b/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! diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 83870206..01ad0b01 100644 --- a/src/bors/handlers/review.rs +++ b/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> { +async fn check_pr_approval_validity( + pr: &PullRequestData, + repo: &RepositoryState, +) -> anyhow::Result> { // 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 = 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; + } } diff --git a/src/config.rs b/src/config.rs index 28e8634f..1e584627 100644 --- a/src/config.rs +++ b/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>, + /// Labels that will block a PR from being approved when present on the PR. + #[serde(default)] + pub labels_blocking_approval: Vec, /// 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() } diff --git a/src/github/mod.rs b/src/github/mod.rs index 89e6bb26..f8082f1b 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -121,6 +121,7 @@ pub struct PullRequest { pub author: GithubUser, pub assignees: Vec, pub status: PullRequestStatus, + pub labels: Vec, } impl From for PullRequest { @@ -157,6 +158,12 @@ impl From for PullRequest { } else { PullRequestStatus::Open }, + labels: pr + .labels + .unwrap_or_default() + .into_iter() + .map(|l| l.name) + .collect(), } } } diff --git a/src/github/webhook.rs b/src/github/webhook.rs index 318ce092..1218e1ba 100644 --- a/src/github/webhook.rs +++ b/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: [], }, }, ), diff --git a/src/tests/mocks/pull_request.rs b/src/tests/mocks/pull_request.rs index 698b8d3f..a689da3f 100644 --- a/src/tests/mocks/pull_request.rs +++ b/src/tests/mocks/pull_request.rs @@ -134,7 +134,7 @@ async fn mock_pr_labels( let Some(pr) = repo.pull_requests.get_mut(&pr_number) else { return ResponseTemplate::new(404); }; - pr.added_labels.extend(data.labels.clone()); + pr.labels_added_by_bors.extend(data.labels.clone()); let labels: Vec = data .labels @@ -162,7 +162,7 @@ async fn mock_pr_labels( let Some(pr) = repo.pull_requests.get_mut(&pr_number) else { return ResponseTemplate::new(404); }; - pr.removed_labels.push(label_name.to_string()); + pr.labels_removed_by_bors.push(label_name.to_string()); ResponseTemplate::new(200).set_body_json::<&[GitHubLabel]>(&[]) }, @@ -195,6 +195,7 @@ pub struct GitHubPullRequest { user: GitHubUser, assignees: Vec, + labels: Vec, } impl From for GitHubPullRequest { @@ -202,8 +203,8 @@ impl From for GitHubPullRequest { let PullRequest { number, repo: _, - added_labels: _, - removed_labels: _, + labels_added_by_bors: _, + labels_removed_by_bors: _, comment_counter: _, head_sha, author, @@ -215,6 +216,7 @@ impl From for GitHubPullRequest { assignees, description, title, + labels, } = pr; GitHubPullRequest { user: author.clone().into(), @@ -237,10 +239,31 @@ impl From for GitHubPullRequest { merged_at, closed_at, assignees: assignees.into_iter().map(Into::into).collect(), + labels: labels + .into_iter() + .map(|label| GitHubLabel { + id: LabelId(1), + node_id: "".to_string(), + url: "https://test.com".to_string().parse().unwrap(), + name: label, + color: "".to_string(), + default: false, + }) + .collect(), } } } +#[derive(Serialize)] +struct GitHubLabel { + id: LabelId, + node_id: String, + url: Url, + name: String, + color: String, + default: bool, +} + #[derive(Serialize)] struct GitHubHead { label: String, @@ -255,17 +278,6 @@ struct GitHubBase { ref_field: String, sha: String, } - -#[derive(Serialize)] -struct GitHubLabel { - id: LabelId, - node_id: String, - url: Url, - name: String, - color: String, - default: bool, -} - #[derive(Serialize)] pub(super) struct GitHubPullRequestEventPayload { action: String, diff --git a/src/tests/mocks/repository.rs b/src/tests/mocks/repository.rs index d8e62508..0e24bdcc 100644 --- a/src/tests/mocks/repository.rs +++ b/src/tests/mocks/repository.rs @@ -43,8 +43,8 @@ pub struct CheckRunData { pub struct PullRequest { pub number: PullRequestNumber, pub repo: GithubRepoName, - pub added_labels: Vec, - pub removed_labels: Vec, + pub labels_added_by_bors: Vec, + pub labels_removed_by_bors: Vec, pub comment_counter: u64, pub head_sha: String, pub author: User, @@ -56,6 +56,7 @@ pub struct PullRequest { pub assignees: Vec, pub description: String, pub title: String, + pub labels: Vec, } impl PullRequest { @@ -63,8 +64,8 @@ impl PullRequest { Self { number: PullRequestNumber(number), repo, - added_labels: Vec::new(), - removed_labels: Vec::new(), + labels_added_by_bors: Vec::new(), + labels_removed_by_bors: Vec::new(), comment_counter: 0, head_sha: format!("pr-{number}-sha"), author, @@ -80,6 +81,7 @@ impl PullRequest { assignees: Vec::new(), description: format!("Description of PR {number}"), title: format!("Title of PR {number}"), + labels: Vec::new(), } } } @@ -100,7 +102,7 @@ impl Default for PullRequest { impl PullRequest { pub fn check_added_labels(&self, labels: &[&str]) { let added_labels = self - .added_labels + .labels_added_by_bors .iter() .map(|s| s.as_str()) .collect::>(); @@ -109,7 +111,7 @@ impl PullRequest { pub fn check_removed_labels(&self, labels: &[&str]) { let removed_labels = self - .removed_labels + .labels_removed_by_bors .iter() .map(|s| s.as_str()) .collect::>();