From 9ed3a081172b36c6e88d545d710d51d38dd1bd57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 21 Dec 2025 23:13:58 +0100 Subject: [PATCH] Do not add or remove labels on PRs unnecessarily --- src/bors/build_queue.rs | 2 +- src/bors/handlers/mod.rs | 13 ++++++++++--- src/bors/handlers/pr_events.rs | 4 ++-- src/bors/handlers/review.rs | 10 ++++++++-- src/bors/labels.rs | 22 ++++++++++++++++++---- src/bors/mergeability_queue.rs | 21 ++++++++++++--------- 6 files changed, 51 insertions(+), 21 deletions(-) diff --git a/src/bors/build_queue.rs b/src/bors/build_queue.rs index 6e255325..bafc45b4 100644 --- a/src/bors/build_queue.rs +++ b/src/bors/build_queue.rs @@ -279,7 +279,7 @@ async fn maybe_complete_build( db.update_build_status(build, status).await?; if let Some(trigger) = trigger { - handle_label_trigger(repo, pr_num, trigger).await?; + handle_label_trigger(repo, pr_num, None, trigger).await?; } if let Some(check_run_id) = build.check_run_id { diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 2ddd85f3..90af7f31 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -643,10 +643,17 @@ async fn has_permission( pub async fn unapprove_pr( repo_state: &RepositoryState, db: &PgDbClient, - pr: &PullRequestModel, + pr_db: &PullRequestModel, + pr_gh: &PullRequest, ) -> anyhow::Result<()> { - db.unapprove(pr).await?; - handle_label_trigger(repo_state, pr.number, LabelTrigger::Unapproved).await + db.unapprove(pr_db).await?; + handle_label_trigger( + repo_state, + pr_db.number, + Some(pr_gh), + LabelTrigger::Unapproved, + ) + .await } /// Is this branch interesting for the bot? diff --git a/src/bors/handlers/pr_events.rs b/src/bors/handlers/pr_events.rs index 00d777e5..13c1484f 100644 --- a/src/bors/handlers/pr_events.rs +++ b/src/bors/handlers/pr_events.rs @@ -41,7 +41,7 @@ pub(super) async fn handle_pull_request_edited( return Ok(()); } - unapprove_pr(&repo_state, &db, &pr_model).await?; + unapprove_pr(&repo_state, &db, &pr_model, pr).await?; notify_of_edited_pr(&repo_state, pr_number, &payload.pull_request.base.name).await } @@ -76,7 +76,7 @@ pub(super) async fn handle_push_to_pull_request( .as_ref() .map(|b| b.status.is_failure()) .unwrap_or(false); - unapprove_pr(&repo_state, &db, &pr_model).await?; + unapprove_pr(&repo_state, &db, &pr_model, pr).await?; // If we had an approved PR with a failed build, there's not much point in sending this warning if !had_failed_build { diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index f14137e8..e85f0598 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -63,7 +63,13 @@ pub(super) async fn command_approve( let priority = priority.or(pr.db.priority.map(|p| p as u32)); merge_queue_tx.notify().await?; - handle_label_trigger(&repo_state, pr.number(), LabelTrigger::Approved).await?; + handle_label_trigger( + &repo_state, + pr.number(), + Some(pr.github), + LabelTrigger::Approved, + ) + .await?; notify_of_approval(ctx, &repo_state, pr, priority, approver.as_str()).await } @@ -145,7 +151,7 @@ pub(super) async fn command_unapprove( AutoBuildCancelReason::Unapproval, ) .await?; - unapprove_pr(&repo_state, &db, pr.db).await?; + unapprove_pr(&repo_state, &db, pr.db, pr.github).await?; notify_of_unapproval(&repo_state, pr, auto_build_cancel_message).await?; Ok(()) diff --git a/src/bors/labels.rs b/src/bors/labels.rs index 1c4ecd0a..8358bcf3 100644 --- a/src/bors/labels.rs +++ b/src/bors/labels.rs @@ -1,14 +1,16 @@ use itertools::Itertools; +use std::collections::HashSet; use tracing::log; use crate::bors::RepositoryState; -use crate::github::{LabelModification, LabelTrigger, PullRequestNumber}; +use crate::github::{LabelModification, LabelTrigger, PullRequest, PullRequestNumber}; /// If there are any label modifications that should be performed on the given PR when `trigger` /// happens, this function will perform them. pub async fn handle_label_trigger( repo: &RepositoryState, - pr: PullRequestNumber, + pr_number: PullRequestNumber, + pr: Option<&PullRequest>, trigger: LabelTrigger, ) -> anyhow::Result<()> { let mut add: Vec = Vec::new(); @@ -21,14 +23,26 @@ pub async fn handle_label_trigger( LabelModification::Add(label) => itertools::Either::Left(label.clone()), LabelModification::Remove(label) => itertools::Either::Right(label.clone()), }); + + // If we know the GitHub state, only remove/add labels that will actually have any effect on the + // PR. + if let Some(pr) = pr { + let existing_labels: HashSet<&str> = pr.labels.iter().map(|s| s.as_str()).collect(); + add.retain(|l| !existing_labels.contains(l.as_str())); + remove.retain(|l| existing_labels.contains(l.as_str())); + log::info!( + "Filtered labels: requested = {modifications:?}, pr = {existing_labels:?}, add = {add:?}, remove = {remove:?}" + ); + } } + if !add.is_empty() { log::info!("Adding label(s) {add:?}"); - repo.client.add_labels(pr, &add).await?; + repo.client.add_labels(pr_number, &add).await?; } if !remove.is_empty() { log::info!("Removing label(s) {remove:?}"); - repo.client.remove_labels(pr, &remove).await?; + repo.client.remove_labels(pr_number, &remove).await?; } Ok(()) } diff --git a/src/bors/mergeability_queue.rs b/src/bors/mergeability_queue.rs index 420b7543..20296803 100644 --- a/src/bors/mergeability_queue.rs +++ b/src/bors/mergeability_queue.rs @@ -14,7 +14,7 @@ use crate::PgDbClient; use crate::bors::comment::conflict_comment; use crate::bors::handlers::unapprove_pr; use crate::database::{MergeableState, OctocrabMergeableState, PullRequestModel}; -use crate::github::{GithubRepoName, PullRequestNumber}; +use crate::github::{GithubRepoName, PullRequest, PullRequestNumber}; use std::cmp::Reverse; use std::collections::{BTreeMap, BinaryHeap}; use std::sync::atomic::AtomicBool; @@ -390,11 +390,11 @@ pub async fn check_mergeability( .client .get_pull_request(pull_request.pr_number) .await?; - let new_mergeable_state = fetched_pr.mergeable_state; + let new_mergeable_state = fetched_pr.mergeable_state.clone(); // We don't know the mergeability state yet. Retry the PR after some delay if new_mergeable_state == OctocrabMergeableState::Unknown { - match fetched_pr.status { + match &fetched_pr.status { PullRequestStatus::Open | PullRequestStatus::Draft => { tracing::info!("Mergeability status unknown, scheduling retry."); mq_tx.enqueue_retry(mq_item); @@ -420,7 +420,7 @@ pub async fn check_mergeability( .await?; if new_mergeable_state == MergeableState::HasConflicts && was_previously_mergeable { - handle_pr_conflict(&repo_state, &ctx.db, pull_request, conflict_source).await?; + handle_pr_conflict(&repo_state, &ctx.db, &fetched_pr, conflict_source).await?; } Ok(()) @@ -429,7 +429,7 @@ pub async fn check_mergeability( async fn handle_pr_conflict( repo_state: &RepositoryState, db: &PgDbClient, - pr: &PullRequestData, + pr: &PullRequest, conflict_source: Option, ) -> anyhow::Result<()> { tracing::info!("Pull request {pr:?} was likely unmergeable (source: {conflict_source:?})"); @@ -439,20 +439,23 @@ async fn handle_pr_conflict( return Ok(()); } - let Some(pr) = db.get_pull_request(&pr.repo, pr.pr_number).await? else { + let Some(pr_db) = db + .get_pull_request(repo_state.repository(), pr.number) + .await? + else { return Err(anyhow::anyhow!("Pull request {pr:?} was not found")); }; // Unapprove PR - let unapproved = if pr.is_approved() { - unapprove_pr(repo_state, db, &pr).await?; + let unapproved = if pr_db.is_approved() { + unapprove_pr(repo_state, db, &pr_db, pr).await?; true } else { false }; repo_state .client - .post_comment(pr.number, conflict_comment(conflict_source, unapproved)) + .post_comment(pr_db.number, conflict_comment(conflict_source, unapproved)) .await?; Ok(()) }