Skip to content

Commit d53981a

Browse files
KobzolKobzol
committed
Forbid approving PRs with approval blocking labels
1 parent 94b1b91 commit d53981a

File tree

6 files changed

+157
-28
lines changed

6 files changed

+157
-28
lines changed

‎src/bors/comment.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ handled during merge and rebase. This is normal, and you should still perform st
215215
}
216216

217217
pub fn approve_non_open_pr_comment() -> Comment {
218-
Comment::new("Only open, non-draft PRs can be approved".to_string())
218+
Comment::new(":clipboard: Only open, non-draft PRs can be approved.".to_string())
219219
}
220220

221221
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.
227227
))
228228
}
229229

230+
pub fn approve_blocking_labels_present(blocking_labels: &[&str]) -> Comment {
231+
let labels = blocking_labels
232+
.iter()
233+
.map(|label| format!("`{label}`"))
234+
.join(", ");
235+
Comment::new(format!(
236+
":clipboard: This PR cannot be approved because it currently has the following {}: {labels}.",
237+
pluralize("label", blocking_labels.len())
238+
))
239+
}
240+
230241
pub fn delegate_try_builds_comment(delegatee: &str, bot_prefix: &CommandPrefix) -> Comment {
231242
Comment::new(format!(
232243
r":v: @{delegatee}, you can now perform try builds on this pull request!

‎src/bors/handlers/review.rs

Lines changed: 80 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ use crate::bors::RepositoryState;
55
use crate::bors::command::RollupMode;
66
use crate::bors::command::{Approver, CommandPrefix};
77
use crate::bors::comment::{
8-
approve_non_open_pr_comment, approve_wip_title, delegate_comment, delegate_try_builds_comment,
8+
approve_blocking_labels_present, approve_non_open_pr_comment, approve_wip_title,
9+
delegate_comment, delegate_try_builds_comment,
910
};
1011
use crate::bors::handlers::has_permission;
1112
use crate::bors::handlers::labels::handle_label_trigger;
@@ -38,7 +39,7 @@ pub(super) async fn command_approve(
3839
return Ok(());
3940
};
4041

41-
if let Some(error_comment) = check_pr_approval_validity(pr).await? {
42+
if let Some(error_comment) = check_pr_approval_validity(pr, &repo_state).await? {
4243
repo_state
4344
.client
4445
.post_comment(pr.number(), error_comment)
@@ -69,7 +70,10 @@ const WIP_KEYWORDS: &[&str] = &["wip", "[do not merge]"];
6970
/// Check that the given PR can be approved in its current state.
7071
/// Returns `Ok(Some(comment))` if it **cannot** be approved; the comment should be sent to the
7172
/// pull request.
72-
async fn check_pr_approval_validity(pr: &PullRequestData) -> anyhow::Result<Option<Comment>> {
73+
async fn check_pr_approval_validity(
74+
pr: &PullRequestData,
75+
repo: &RepositoryState,
76+
) -> anyhow::Result<Option<Comment>> {
7377
// Check PR status
7478
if !matches!(pr.github.status, PullRequestStatus::Open) {
7579
return Ok(Some(approve_non_open_pr_comment()));
@@ -81,6 +85,24 @@ async fn check_pr_approval_validity(pr: &PullRequestData) -> anyhow::Result<Opti
8185
return Ok(Some(approve_wip_title(wip_kw)));
8286
}
8387

88+
// Check blocking labels
89+
let config = repo.config.load();
90+
let blocking_labels: Vec<&str> = pr
91+
.github
92+
.labels
93+
.iter()
94+
.map(|label| label.as_str())
95+
.filter(|label| {
96+
config
97+
.labels_blocking_approval
98+
.iter()
99+
.any(|blocking_label| blocking_label == label)
100+
})
101+
.collect();
102+
if !blocking_labels.is_empty() {
103+
return Ok(Some(approve_blocking_labels_present(&blocking_labels)));
104+
}
105+
84106
Ok(None)
85107
}
86108

@@ -1102,7 +1124,7 @@ mod tests {
11021124
.set_pr_status_draft(default_repo_name(), default_pr_number())
11031125
.await?;
11041126
tester.post_comment("@bors r+").await?;
1105-
insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
1127+
insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: Only open, non-draft PRs can be approved.");
11061128
tester.default_pr().await.expect_unapproved();
11071129
Ok(())
11081130
})
@@ -1116,7 +1138,7 @@ mod tests {
11161138
.set_pr_status_closed(default_repo_name(), default_pr_number())
11171139
.await?;
11181140
tester.post_comment("@bors r+").await?;
1119-
insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
1141+
insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: Only open, non-draft PRs can be approved.");
11201142
tester.default_pr().await.expect_unapproved();
11211143
Ok(())
11221144
})
@@ -1130,7 +1152,7 @@ mod tests {
11301152
.set_pr_status_merged(default_repo_name(), default_pr_number())
11311153
.await?;
11321154
tester.post_comment("@bors r+").await?;
1133-
insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
1155+
insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: Only open, non-draft PRs can be approved.");
11341156
tester.default_pr().await.expect_unapproved();
11351157
Ok(())
11361158
})
@@ -1156,4 +1178,56 @@ mod tests {
11561178
})
11571179
.await;
11581180
}
1181+
1182+
#[sqlx::test]
1183+
async fn approve_pr_with_blocked_label(pool: sqlx::PgPool) {
1184+
BorsBuilder::new(pool)
1185+
.github(GitHubState::default().with_default_config(
1186+
r#"
1187+
labels_blocking_approval = ["proposed-final-comment-period"]
1188+
"#,
1189+
))
1190+
.run_test(async |tester: &mut BorsTester| {
1191+
tester
1192+
.edit_pr(default_repo_name(), default_pr_number(), |pr| {
1193+
pr.labels = vec![
1194+
"S-waiting-on-review".to_string(),
1195+
"proposed-final-comment-period".to_string(),
1196+
];
1197+
})
1198+
.await?;
1199+
tester.post_comment("@bors r+").await?;
1200+
insta::assert_snapshot!(tester.get_comment().await?, @":clipboard: This PR cannot be approved because it currently has the following label: `proposed-final-comment-period`.");
1201+
tester.default_pr().await.expect_unapproved();
1202+
Ok(())
1203+
})
1204+
.await;
1205+
}
1206+
1207+
#[sqlx::test]
1208+
async fn approve_pr_with_blocked_labels(pool: sqlx::PgPool) {
1209+
BorsBuilder::new(pool)
1210+
.github(GitHubState::default().with_default_config(
1211+
r#"
1212+
labels_blocking_approval = ["proposed-final-comment-period", "final-comment-period"]
1213+
"#,
1214+
))
1215+
.run_test(async |tester: &mut BorsTester| {
1216+
tester
1217+
.edit_pr(default_repo_name(), default_pr_number(), |pr| {
1218+
pr.labels = vec![
1219+
"S-waiting-on-review".to_string(),
1220+
"proposed-final-comment-period".to_string(),
1221+
"final-comment-period".to_string(),
1222+
"S-blocked".to_string(),
1223+
];
1224+
})
1225+
.await?;
1226+
tester.post_comment("@bors r+").await?;
1227+
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`.");
1228+
tester.default_pr().await.expect_unapproved();
1229+
Ok(())
1230+
})
1231+
.await;
1232+
}
11591233
}

‎src/github/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,7 @@ pub struct PullRequest {
121121
pub author: GithubUser,
122122
pub assignees: Vec<GithubUser>,
123123
pub status: PullRequestStatus,
124+
pub labels: Vec<String>,
124125
}
125126

126127
impl From<octocrab::models::pulls::PullRequest> for PullRequest {
@@ -157,6 +158,12 @@ impl From<octocrab::models::pulls::PullRequest> for PullRequest {
157158
} else {
158159
PullRequestStatus::Open
159160
},
161+
labels: pr
162+
.labels
163+
.unwrap_or_default()
164+
.into_iter()
165+
.map(|l| l.name)
166+
.collect(),
160167
}
161168
}
162169
}

‎src/github/webhook.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,11 @@ mod tests {
607607
},
608608
assignees: [],
609609
status: Open,
610+
labels: [
611+
"foo",
612+
"foobar",
613+
"baz",
614+
],
610615
},
611616
from_base_sha: Some(
612617
CommitSha(
@@ -679,6 +684,11 @@ mod tests {
679684
},
680685
assignees: [],
681686
status: Open,
687+
labels: [
688+
"foo",
689+
"foobar",
690+
"baz",
691+
],
682692
},
683693
},
684694
),
@@ -795,6 +805,7 @@ mod tests {
795805
},
796806
assignees: [],
797807
status: Open,
808+
labels: [],
798809
},
799810
draft: false,
800811
},
@@ -863,6 +874,9 @@ mod tests {
863874
},
864875
assignees: [],
865876
status: Closed,
877+
labels: [
878+
"foo",
879+
],
866880
},
867881
},
868882
),
@@ -930,6 +944,7 @@ mod tests {
930944
},
931945
assignees: [],
932946
status: Merged,
947+
labels: [],
933948
},
934949
},
935950
),
@@ -997,6 +1012,9 @@ mod tests {
9971012
},
9981013
assignees: [],
9991014
status: Open,
1015+
labels: [
1016+
"foo",
1017+
],
10001018
},
10011019
},
10021020
),
@@ -1064,6 +1082,7 @@ mod tests {
10641082
},
10651083
assignees: [],
10661084
status: Draft,
1085+
labels: [],
10671086
},
10681087
draft: true,
10691088
},
@@ -1132,6 +1151,7 @@ mod tests {
11321151
},
11331152
assignees: [],
11341153
status: Draft,
1154+
labels: [],
11351155
},
11361156
},
11371157
),
@@ -1199,6 +1219,7 @@ mod tests {
11991219
},
12001220
assignees: [],
12011221
status: Open,
1222+
labels: [],
12021223
},
12031224
},
12041225
),
@@ -1337,6 +1358,7 @@ mod tests {
13371358
},
13381359
],
13391360
status: Open,
1361+
labels: [],
13401362
},
13411363
},
13421364
),
@@ -1404,6 +1426,7 @@ mod tests {
14041426
},
14051427
assignees: [],
14061428
status: Open,
1429+
labels: [],
14071430
},
14081431
},
14091432
),

‎src/tests/mocks/pull_request.rs

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ async fn mock_pr_labels(
134134
let Some(pr) = repo.pull_requests.get_mut(&pr_number) else {
135135
return ResponseTemplate::new(404);
136136
};
137-
pr.added_labels.extend(data.labels.clone());
137+
pr.labels_added_by_bors.extend(data.labels.clone());
138138

139139
let labels: Vec<GitHubLabel> = data
140140
.labels
@@ -162,7 +162,7 @@ async fn mock_pr_labels(
162162
let Some(pr) = repo.pull_requests.get_mut(&pr_number) else {
163163
return ResponseTemplate::new(404);
164164
};
165-
pr.removed_labels.push(label_name.to_string());
165+
pr.labels_removed_by_bors.push(label_name.to_string());
166166

167167
ResponseTemplate::new(200).set_body_json::<&[GitHubLabel]>(&[])
168168
},
@@ -195,15 +195,16 @@ pub struct GitHubPullRequest {
195195

196196
user: GitHubUser,
197197
assignees: Vec<GitHubUser>,
198+
labels: Vec<GitHubLabel>,
198199
}
199200

200201
impl From<PullRequest> for GitHubPullRequest {
201202
fn from(pr: PullRequest) -> Self {
202203
let PullRequest {
203204
number,
204205
repo: _,
205-
added_labels: _,
206-
removed_labels: _,
206+
labels_added_by_bors: _,
207+
labels_removed_by_bors: _,
207208
comment_counter: _,
208209
head_sha,
209210
author,
@@ -215,6 +216,7 @@ impl From<PullRequest> for GitHubPullRequest {
215216
assignees,
216217
description,
217218
title,
219+
labels,
218220
} = pr;
219221
GitHubPullRequest {
220222
user: author.clone().into(),
@@ -237,10 +239,31 @@ impl From<PullRequest> for GitHubPullRequest {
237239
merged_at,
238240
closed_at,
239241
assignees: assignees.into_iter().map(Into::into).collect(),
242+
labels: labels
243+
.into_iter()
244+
.map(|label| GitHubLabel {
245+
id: LabelId(1),
246+
node_id: "".to_string(),
247+
url: "https://test.com".to_string().parse().unwrap(),
248+
name: label,
249+
color: "".to_string(),
250+
default: false,
251+
})
252+
.collect(),
240253
}
241254
}
242255
}
243256

257+
#[derive(Serialize)]
258+
struct GitHubLabel {
259+
id: LabelId,
260+
node_id: String,
261+
url: Url,
262+
name: String,
263+
color: String,
264+
default: bool,
265+
}
266+
244267
#[derive(Serialize)]
245268
struct GitHubHead {
246269
label: String,
@@ -255,17 +278,6 @@ struct GitHubBase {
255278
ref_field: String,
256279
sha: String,
257280
}
258-
259-
#[derive(Serialize)]
260-
struct GitHubLabel {
261-
id: LabelId,
262-
node_id: String,
263-
url: Url,
264-
name: String,
265-
color: String,
266-
default: bool,
267-
}
268-
269281
#[derive(Serialize)]
270282
pub(super) struct GitHubPullRequestEventPayload {
271283
action: String,

0 commit comments

Comments
 (0)