Skip to content
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

Jump to bottom

Revert "Assign PR assignment based on work queue" #1792

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Revert "Assign PR assignment based on work queue" #1792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion src/db.rs
View file
Open in desktop
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;",
Copy link
Member

Choose a reason for hiding this comment

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

This looks iffy, but I think should be ok to revert. We'll need to be careful when reapplying this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I'll keep it in mind, thanks

];
4 changes: 2 additions & 2 deletions src/handlers.rs
View file
Open in desktop
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
Open in desktop
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
Loading