Skip to content

Commit

Permalink
Merge pull request #1792 from apiraino/revert-1786-assign-prs-based-o…
Browse files Browse the repository at this point in the history 
…n-work-queue

Revert "Assign PR assignment based on work queue"
  • Loading branch information
@Mark-Simulacrum
Mark-Simulacrum authored Apr 16, 2024
2 parents 19e3c28 + 70dd15e commit a0ad972
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 194 deletions.
1 change: 0 additions & 1 deletion src/db.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,5 +335,4 @@ CREATE table review_prefs (
CREATE EXTENSION intarray;
CREATE UNIQUE INDEX review_prefs_user_id ON review_prefs(user_id);
",
"ALTER TABLE review_prefs ADD COLUMN max_assigned_prs INTEGER DEFAULT NULL;",
];
4 changes: 2 additions & 2 deletions src/handlers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ macro_rules! issue_handlers {

// Handle events that happened on issues
//
// This is for events that happen only on issues or pull requests (e.g. label changes or assignments).
// This is for events that happen only on issues (e.g. label changes).
// Each module in the list must contain the functions `parse_input` and `handle_input`.
issue_handlers! {
assign,
Expand Down Expand Up @@ -280,7 +280,7 @@ macro_rules! command_handlers {
//
// This is for handlers for commands parsed by the `parser` crate.
// Each variant of `parser::command::Command` must be in this list,
// preceded by the module containing the corresponding `handle_command` function
// preceded by the module containing the coresponding `handle_command` function
command_handlers! {
assign: Assign,
glacier: Glacier,
Expand Down
145 changes: 15 additions & 130 deletions src/handlers/assign.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@
//! * `@rustbot release-assignment`: Removes the commenter's assignment.
//! * `r? @user`: Assigns to the given user (PRs only).
//!
//! Note: this module does not handle review assignments issued from the
//! GitHub "Assignees" dropdown menu
//!
//! This is capable of assigning to any user, even if they do not have write
//! access to the repo. It does this by fake-assigning the bot and adding a
//! "claimed by" section to the top-level comment.
Expand All @@ -23,7 +20,7 @@
use crate::{
config::AssignConfig,
github::{self, Event, FileDiff, Issue, IssuesAction, Selection},
handlers::{pr_tracking::has_user_capacity, Context, GithubClient, IssuesEvent},
handlers::{Context, GithubClient, IssuesEvent},
interactions::EditIssueBody,
};
use anyhow::{bail, Context as _};
Expand All @@ -33,7 +30,6 @@ use rand::seq::IteratorRandom;
use rust_team_data::v1::Teams;
use std::collections::{HashMap, HashSet};
use std::fmt;
use tokio_postgres::Client as DbClient;
use tracing as log;

#[cfg(test)]
Expand Down Expand Up @@ -79,27 +75,6 @@ const NON_DEFAULT_BRANCH: &str =

const SUBMODULE_WARNING_MSG: &str = "These commits modify **submodules**.";

pub const SELF_ASSIGN_HAS_NO_CAPACITY: &str = "
You have insufficient capacity to be assigned the pull request at this time. PR assignment has been reverted.
Please choose another assignee or increase your assignment limit.
(see [documentation](https://forge.rust-lang.org/triagebot/pr-assignment-tracking.html))";

pub const REVIEWER_HAS_NO_CAPACITY: &str = "
`{username}` has insufficient capacity to be assigned the pull request at this time. PR assignment has been reverted.
Please choose another assignee.
(see [documentation](https://forge.rust-lang.org/triagebot/pr-assignment-tracking.html))";

const NO_REVIEWER_HAS_CAPACITY: &str = "
Could not find a reviewer with enough capacity to be assigned at this time. This is a problem.
Please contact us on [#t-infra](https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra) on Zulip.
cc: @jackh726 @apiraino";

fn on_vacation_msg(user: &str) -> String {
ON_VACATION_WARNING.replace("{username}", user)
}
Expand Down Expand Up @@ -297,8 +272,6 @@ async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) {
/// Determines who to assign the PR to based on either an `r?` command, or
/// based on which files were modified.
///
/// Will also check if candidates have capacity in their work queue.
///
/// Returns `(assignee, from_comment)` where `assignee` is who to assign to
/// (or None if no assignee could be found). `from_comment` is a boolean
/// indicating if the assignee came from an `r?` command (it is false if
Expand All @@ -309,14 +282,13 @@ async fn determine_assignee(
config: &AssignConfig,
diff: &[FileDiff],
) -> anyhow::Result<(Option<String>, bool)> {
let db_client = ctx.db.get().await;
let teams = crate::team_data::teams(&ctx.github).await?;
if let Some(name) = find_assign_command(ctx, event) {
if is_self_assign(&name, &event.issue.user.login) {
return Ok((Some(name.to_string()), true));
}
// User included `r?` in the opening PR body.
match find_reviewer_from_names(&db_client, &teams, config, &event.issue, &[name]).await {
match find_reviewer_from_names(&teams, config, &event.issue, &[name]) {
Ok(assignee) => return Ok((Some(assignee), true)),
Err(e) => {
event
Expand All @@ -330,9 +302,7 @@ async fn determine_assignee(
// Errors fall-through to try fallback group.
match find_reviewers_from_diff(config, diff) {
Ok(candidates) if !candidates.is_empty() => {
match find_reviewer_from_names(&db_client, &teams, config, &event.issue, &candidates)
.await
{
match find_reviewer_from_names(&teams, config, &event.issue, &candidates) {
Ok(assignee) => return Ok((Some(assignee), false)),
Err(FindReviewerError::TeamNotFound(team)) => log::warn!(
"team {team} not found via diff from PR {}, \
Expand All @@ -342,9 +312,7 @@ async fn determine_assignee(
// TODO: post a comment on the PR if the reviewers were filtered due to being on vacation
Err(
e @ FindReviewerError::NoReviewer { .. }
| e @ FindReviewerError::AllReviewersFiltered { .. }
| e @ FindReviewerError::NoReviewerHasCapacity
| e @ FindReviewerError::ReviewerHasNoCapacity { .. },
| e @ FindReviewerError::AllReviewersFiltered { .. },
) => log::trace!(
"no reviewer could be determined for PR {}: {e}",
event.issue.global_id()
Expand All @@ -362,7 +330,7 @@ async fn determine_assignee(
}

if let Some(fallback) = config.adhoc_groups.get("fallback") {
match find_reviewer_from_names(&db_client, &teams, config, &event.issue, fallback).await {
match find_reviewer_from_names(&teams, config, &event.issue, fallback) {
Ok(assignee) => return Ok((Some(assignee), false)),
Err(e) => {
log::trace!(
Expand Down Expand Up @@ -524,20 +492,7 @@ pub(super) async fn handle_command(
// welcome message).
return Ok(());
}
let db_client = ctx.db.get().await;
if is_self_assign(&name, &event.user().login) {
match has_user_capacity(&db_client, &name).await {
Ok(work_queue) => work_queue.username,
Err(_) => {
issue
.post_comment(
&ctx.github,
&REVIEWER_HAS_NO_CAPACITY.replace("{username}", &name),
)
.await?;
return Ok(());
}
};
name.to_string()
} else {
let teams = crate::team_data::teams(&ctx.github).await?;
Expand All @@ -557,14 +512,8 @@ pub(super) async fn handle_command(
}
}
}
match find_reviewer_from_names(
&db_client,
&teams,
config,
issue,
&[team_name.to_string()],
)
.await

match find_reviewer_from_names(&teams, config, issue, &[team_name.to_string()])
{
Ok(assignee) => assignee,
Err(e) => {
Expand All @@ -575,11 +524,7 @@ pub(super) async fn handle_command(
}
}
};

// This user is validated and can accept the PR
set_assignee(issue, &ctx.github, &username).await;
// This PR will now be registered in the reviewer's work queue
// by the `pr_tracking` handler
return Ok(());
}

Expand Down Expand Up @@ -637,7 +582,6 @@ pub(super) async fn handle_command(

e.apply(&ctx.github, String::new(), &data).await?;

// Assign the PR: user's work queue has been checked and can accept this PR
match issue.set_assignee(&ctx.github, &to_assign).await {
Ok(()) => return Ok(()), // we are done
Err(github::AssignmentError::InvalidAssignee) => {
Expand All @@ -659,7 +603,7 @@ pub(super) async fn handle_command(
}

#[derive(PartialEq, Debug)]
pub enum FindReviewerError {
enum FindReviewerError {
/// User specified something like `r? foo/bar` where that team name could
/// not be found.
TeamNotFound(String),
Expand All @@ -677,11 +621,6 @@ pub enum FindReviewerError {
initial: Vec<String>,
filtered: Vec<String>,
},
/// No reviewer has capacity to accept a pull request assignment at this time
NoReviewerHasCapacity,
/// The requested reviewer has no capacity to accept a pull request
/// assignment at this time
ReviewerHasNoCapacity { username: String },
}

impl std::error::Error for FindReviewerError {}
Expand Down Expand Up @@ -711,23 +650,13 @@ impl fmt::Display for FindReviewerError {
write!(
f,
"Could not assign reviewer from: `{}`.\n\
User(s) `{}` are either the PR author, already assigned, or on vacation. \
If it's a team, we could not find any candidates.\n\
Please use `r?` to specify someone else to assign.",
User(s) `{}` are either the PR author, already assigned, or on vacation, \
and there are no other candidates.\n\
Use `r?` to specify someone else to assign.",
initial.join(","),
filtered.join(","),
)
}
FindReviewerError::ReviewerHasNoCapacity { username } => {
write!(
f,
"{}",
REVIEWER_HAS_NO_CAPACITY.replace("{username}", username)
)
}
FindReviewerError::NoReviewerHasCapacity => {
write!(f, "{}", NO_REVIEWER_HAS_CAPACITY)
}
}
}
}
Expand All @@ -738,8 +667,7 @@ impl fmt::Display for FindReviewerError {
/// `@octocat`, or names from the owners map. It can contain GitHub usernames,
/// auto-assign groups, or rust-lang team names. It must have at least one
/// entry.
async fn find_reviewer_from_names(
db: &DbClient,
fn find_reviewer_from_names(
teams: &Teams,
config: &AssignConfig,
issue: &Issue,
Expand All @@ -765,57 +693,14 @@ async fn find_reviewer_from_names(
//
// These are all ideas for improving the selection here. However, I'm not
// sure they are really worth the effort.

// filter out team members without capacity
let filtered_candidates = filter_by_capacity(db, &candidates)
.await
.expect("Error while filtering out team members");

if filtered_candidates.is_empty() {
return Err(FindReviewerError::AllReviewersFiltered {
initial: names.to_vec(),
filtered: names.to_vec(),
});
}

log::debug!("Filtered list of candidates: {:?}", filtered_candidates);

Ok(filtered_candidates
Ok(candidates
.into_iter()
.choose(&mut rand::thread_rng())
.expect("candidate_reviewers_from_names should return at least one entry")
.expect("candidate_reviewers_from_names always returns at least one entry")
.to_string())
}

/// Filter out candidates not having review capacity
async fn filter_by_capacity(
db: &DbClient,
candidates: &HashSet<&str>,
) -> anyhow::Result<HashSet<String>> {
let usernames = candidates
.iter()
.map(|c| *c)
.collect::<Vec<&str>>()
.join(",");

let q = format!(
"
SELECT username
FROM review_prefs r
JOIN users on users.user_id=r.user_id
AND username = ANY('{{ {} }}')
AND ARRAY_LENGTH(r.assigned_prs, 1) < max_assigned_prs",
usernames
);
let result = db.query(&q, &[]).await.context("Select DB error")?;
let mut candidates: HashSet<String> = HashSet::new();
for row in result {
candidates.insert(row.get("username"));
}
Ok(candidates)
}

/// returns a list of candidate usernames (from relevant teams) to choose as a reviewer.
/// Returns a list of candidate usernames to choose as a reviewer.
fn candidate_reviewers_from_names<'a>(
teams: &'a Teams,
config: &'a AssignConfig,
Expand Down
Loading

0 comments on commit a0ad972

Please sign in to comment.