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.

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

Workflow for tracking PRs assignment #1754

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ pub(crate) struct Config {
pub(crate) note: Option<NoteConfig>,
pub(crate) mentions: Option<MentionsConfig>,
pub(crate) no_merges: Option<NoMergesConfig>,
pub(crate) review_work_queue: Option<TeamMemberWorkQueueConfig>,
// We want this validation to run even without the entry in the config file
#[serde(default = "ValidateConfig::default")]
pub(crate) validate_config: Option<ValidateConfig>,
Expand Down Expand Up @@ -164,6 +165,13 @@ pub(crate) struct ShortcutConfig {
_empty: (),
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
#[serde(rename_all = "kebab-case")]
pub(crate) struct TeamMemberWorkQueueConfig {
#[serde(default)]
pub(crate) enabled_for_teams: Vec<String>,
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
#[serde(deny_unknown_fields)]
pub(crate) struct PrioritizeConfig {
Expand Down Expand Up @@ -388,6 +396,9 @@ mod tests {
[assign]
users_on_vacation = ["jyn514"]

[review-work-queue]
enabled-for-teams = ["aaa", "bbb"]
Comment on lines +399 to +400
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to enable this feature only for a subset of teams, rather than tracking all PRs of every contributor from the start.

But that would add a little of complexity to this PR. Maybe worth adding in a subsequent patch, when/if this one is merged?


[note]

[ping.compiler]
Expand Down Expand Up @@ -460,6 +471,9 @@ mod tests {
github_releases: None,
review_submitted: None,
review_requested: None,
review_work_queue: Some(TeamMemberWorkQueueConfig {
enabled_for_teams: vec!["aaa".into(), "bbb".into()]
}),
mentions: None,
no_merges: None,
validate_config: Some(ValidateConfig {}),
Expand Down
8 changes: 8 additions & 0 deletions src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,5 +307,13 @@ CREATE UNIQUE INDEX jobs_name_scheduled_at_unique_index
ON jobs (
name, scheduled_at
);
",
"
CREATE table review_prefs (
Copy link
Member

Choose a reason for hiding this comment

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

Is the intention that this will eventually contain review preferences too? (That's what the name suggests)

My first thought was that there should instead be a table of PRs, with assigned reviewers in that list. But, maybe that doesn't make a bunch of sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. The new DB table at the end of the story will also persist the review preferences for each reviewer (thus the name). I'm trying to slip in changes one by one and as slowly as possible, as per the plan detailed in #1753

PRs in a separate joined table def. make sense in general. But in this case I think the use case is so minimal to not warrant that. If in the far future the tool develops such as to make it a compelling choice, we can always migrate.

id UUID DEFAULT gen_random_uuid() PRIMARY KEY,
user_id BIGINT REFERENCES users(user_id),
assigned_prs INT[] NOT NULL DEFAULT array[]::INT[]
);
CREATE UNIQUE INDEX review_prefs_user_id ON review_prefs(user_id);
",
];
2 changes: 2 additions & 0 deletions src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ mod prioritize;
mod relabel;
mod review_requested;
mod review_submitted;
mod review_work_queue;
mod rfc_helper;
pub mod rustc_commits;
mod shortcut;
Expand Down Expand Up @@ -167,6 +168,7 @@ issue_handlers! {
no_merges,
notify_zulip,
review_requested,
review_work_queue,
validate_config,
}

Expand Down
148 changes: 148 additions & 0 deletions src/handlers/review_work_queue.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
use crate::{
config::TeamMemberWorkQueueConfig,
github::{IssuesAction, IssuesEvent},
handlers::Context,
TeamMemberWorkQueue,
};
use anyhow::Context as _;
use tokio_postgres::Client as DbClient;
use tracing as log;

// This module updates the PR work queue of team members
// - When a PR has been assigned, adds the PR to the work queue of team members
// - When a PR is unassigned, removes the PR from the work queue of all team members

/// Get all assignees for a pull request
async fn get_pr_assignees(
db: &DbClient,
issue_num: i32,
) -> anyhow::Result<Vec<TeamMemberWorkQueue>> {
let q = "
SELECT u.username, r.*, array_length(assigned_prs, 1) as num_assigned_prs
FROM review_prefs r
JOIN users u on u.user_id=r.user_id
WHERE $1 = ANY (assigned_prs)";
let rows = db.query(q, &[&issue_num]).await?;
Ok(rows
.into_iter()
.filter_map(|row| Some(TeamMemberWorkQueue::from(row)))
.collect())
}

/// Update a team member work queue
async fn update_team_member_workqueue(
db: &DbClient,
assignee: &TeamMemberWorkQueue,
) -> anyhow::Result<TeamMemberWorkQueue> {
let q = "
UPDATE review_prefs r
SET assigned_prs = $2
FROM users u
WHERE r.user_id=$1 AND u.user_id=r.user_id
RETURNING u.username, r.*, array_length(assigned_prs, 1) as num_assigned_prs";
let rec = db
.query_one(q, &[&assignee.user_id, &assignee.assigned_prs])
.await
.context("Update DB error")?;
Ok(rec.into())
}

/// Add a new user (if not existing)
async fn ensure_team_member(db: &DbClient, user_id: i64, username: &str) -> anyhow::Result<u64> {
let q = "
INSERT INTO users (user_id, username) VALUES ($1, $2)
ON CONFLICT DO NOTHING";
let rec = db
.execute(q, &[&user_id, &username])
.await
.context("Insert user DB error")?;
Ok(rec)
}

/// Create or increase by one a team member work queue
async fn upsert_team_member_workqueue(
db: &DbClient,
user_id: i64,
pr: i32,
) -> anyhow::Result<u64, anyhow::Error> {
let q = "
INSERT INTO review_prefs
(user_id, assigned_prs) VALUES ($1, $2)
ON CONFLICT (user_id)
DO UPDATE SET assigned_prs = array_append(review_prefs.assigned_prs, $3)
WHERE review_prefs.user_id=$1";
let pr_v = vec![pr];
db.execute(q, &[&user_id, &pr_v, &pr])
.await
.context("Upsert DB error")
}

pub(super) struct ReviewPrefsInput {}

pub(super) async fn parse_input(
_ctx: &Context,
event: &IssuesEvent,
config: Option<&TeamMemberWorkQueueConfig>,
) -> Result<Option<ReviewPrefsInput>, String> {
// IMPORTANT: this config check MUST exist. Else, the triagebot will emit an error that this
// feature is not enabled
if config.is_none() {
return Ok(None);
}

// Act only if this is a PR or an assignment / unassignment
if !event.issue.is_pr()
|| !matches!(
event.action,
IssuesAction::Assigned | IssuesAction::Unassigned
)
{
return Ok(None);
}
Ok(Some(ReviewPrefsInput {}))
}

pub(super) async fn handle_input<'a>(
ctx: &Context,
_config: &TeamMemberWorkQueueConfig,
event: &IssuesEvent,
_inputs: ReviewPrefsInput,
) -> anyhow::Result<()> {
let db_client = ctx.db.get().await;
let iss_num = event.issue.number as i32;

// Note: When changing assignees for a PR, we don't receive the assignee(s) removed, we receive
// an event `Unassigned` and the remaining assignees

Copy link
Member

Choose a reason for hiding this comment

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

I would expect this function to "diff" the current assignees with the new assigness, and only update based on that diff...

Copy link
Member

Choose a reason for hiding this comment

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

This is also why it makes sense to me that the table be of PRs with a list of assignees.

But, that complicates the querying. Could think about a table view that aggregates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect this function to "diff" the current assignees with the new assigness, and only update based on that diff

Hm, I'm a bit unclear on this comment 🤔 do you suggest a "3-way" comparison between two arrays (list of current assignees from the DB vs. new list of assignees from GH)? Example:

  • assign to new assignees
  • check if current assignees are still such
  • unassign from removed assignees

(or maybe something else?)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mean, just compare the lists and find what needs to be removed and what needs to be added.

// 1) Remove the PR from everyones' work queue
let mut current_assignees = get_pr_assignees(&db_client, iss_num).await?;
log::debug!("Removing assignment from user(s): {:?}", current_assignees);
for assignee in &mut current_assignees {
if let Some(index) = assignee
.assigned_prs
.iter()
.position(|value| *value == iss_num)
{
assignee.assigned_prs.swap_remove(index);
}
update_team_member_workqueue(&db_client, &assignee).await?;
}

// 2) create or increase by one the team members work queue
// create team members if they don't exist
for u in event.issue.assignees.iter() {
let user_id = u.id.expect("Github user was expected! Please investigate.");
Copy link
Contributor Author

@apiraino apiraino Jan 5, 2024

Choose a reason for hiding this comment

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

This check is just to cover the unlikely case where we receive a User with an id=None

Unsure why the User struct is like that but better safe than sorry.


if let Err(err) = ensure_team_member(&db_client, user_id, &u.login).await {
log::error!("Failed to create user in DB: this PR assignment won't be tracked.");
return Err(err);
}

if let Err(err) = upsert_team_member_workqueue(&db_client, user_id, iss_num).await {
log::error!("Failed to track PR for user: this PR assignment won't be tracked.");
return Err(err);
}
}

Ok(())
}
22 changes: 22 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::github::PullRequestDetails;
use anyhow::Context;
use handlers::HandlerError;
use interactions::ErrorComment;
use serde::Serialize;
use std::fmt;
use tracing as log;

Expand Down Expand Up @@ -122,6 +123,27 @@ impl From<anyhow::Error> for WebhookError {
}
}

#[derive(Debug, Serialize, Clone)]
pub struct TeamMemberWorkQueue {
pub username: String,
pub id: uuid::Uuid,
pub user_id: i64,
pub assigned_prs: Vec<i32>,
pub num_assigned_prs: Option<i32>,
}

impl From<tokio_postgres::row::Row> for TeamMemberWorkQueue {
fn from(row: tokio_postgres::row::Row) -> Self {
Self {
username: row.get("username"),
id: row.get("id"),
user_id: row.get("user_id"),
assigned_prs: row.get("assigned_prs"),
num_assigned_prs: row.get("num_assigned_prs"),
}
}
}

pub fn deserialize_payload<T: serde::de::DeserializeOwned>(v: &str) -> anyhow::Result<T> {
let mut deserializer = serde_json::Deserializer::from_str(&v);
let res: Result<T, _> = serde_path_to_error::deserialize(&mut deserializer);
Expand Down
6 changes: 6 additions & 0 deletions triagebot.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ allow-unauthenticated = ["bug", "invalid", "question", "enhancement"]

[assign]

# Enable this feature to keep track of the PR review assignment
[review-work-queue]
# A list of teams that will use the new PR review assignment workflow.
# Team names match https://github.com/rust-lang/team/tree/master/teams
enabled-for-teams = ["compiler", "compiler-contributors"]

[note]

[ping.wg-meta]
Expand Down
Loading