Skip to content

Forbid approving PRs with labels blocking approval #367

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
merged 4 commits into from
Jul 19, 2025
Merged

Forbid approving PRs with labels blocking approval #367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41f23c0
Deserialize labels blocking approval
Kobzol Jul 19, 2025
94b1b91
Deny unknown fields in repository config
Kobzol Jul 19, 2025
d53981a
Forbid approving PRs with approval blocking labels
Kobzol Jul 19, 2025
4861da4
Add example to config
Kobzol Jul 19, 2025
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
4 changes: 4 additions & 0 deletions rust-bors.example.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add "needs-fcp" here, or is that a label we expect to keep even after FCP passed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is just an example config, this hasn't been enabled on rust-lang/rust yet :) I'll send a PR shortly, we can discuss it there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw seems like needs-fcp indeed isn't removed after the FCP finishes (rust-lang/rust#79278), so maybe we shouldn't mark it as blocking.

13 changes: 12 additions & 1 deletion src/bors/comment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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!
Expand Down
86 changes: 80 additions & 6 deletions src/bors/handlers/review.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()));
Expand All @@ -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)
}

Expand Down Expand Up @@ -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(())
})
Expand All @@ -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(())
})
Expand All @@ -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(())
})
Expand All @@ -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;
}
}
23 changes: 23 additions & 0 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand All @@ -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")]
Expand Down Expand Up @@ -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()
}
Expand Down
7 changes: 7 additions & 0 deletions src/github/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(),
}
}
}
Expand Down
23 changes: 23 additions & 0 deletions src/github/webhook.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,11 @@ mod tests {
},
assignees: [],
status: Open,
labels: [
"foo",
"foobar",
"baz",
],
},
from_base_sha: Some(
CommitSha(
Expand Down Expand Up @@ -679,6 +684,11 @@ mod tests {
},
assignees: [],
status: Open,
labels: [
"foo",
"foobar",
"baz",
],
},
},
),
Expand Down Expand Up @@ -795,6 +805,7 @@ mod tests {
},
assignees: [],
status: Open,
labels: [],
},
draft: false,
},
Expand Down Expand Up @@ -863,6 +874,9 @@ mod tests {
},
assignees: [],
status: Closed,
labels: [
"foo",
],
},
},
),
Expand Down Expand Up @@ -930,6 +944,7 @@ mod tests {
},
assignees: [],
status: Merged,
labels: [],
},
},
),
Expand Down Expand Up @@ -997,6 +1012,9 @@ mod tests {
},
assignees: [],
status: Open,
labels: [
"foo",
],
},
},
),
Expand Down Expand Up @@ -1064,6 +1082,7 @@ mod tests {
},
assignees: [],
status: Draft,
labels: [],
},
draft: true,
},
Expand Down Expand Up @@ -1132,6 +1151,7 @@ mod tests {
},
assignees: [],
status: Draft,
labels: [],
},
},
),
Expand Down Expand Up @@ -1199,6 +1219,7 @@ mod tests {
},
assignees: [],
status: Open,
labels: [],
},
},
),
Expand Down Expand Up @@ -1337,6 +1358,7 @@ mod tests {
},
],
status: Open,
labels: [],
},
},
),
Expand Down Expand Up @@ -1404,6 +1426,7 @@ mod tests {
},
assignees: [],
status: Open,
labels: [],
},
},
),
Expand Down
Loading
Loading