New pull request assignment proposal #1719

apiraino · 2023-09-12T14:35:49Z

This patchset is the implementation of a new workflow for assigning pull requests to the Rust project contributors.

2023-11-08 UPDATE: After a meeting with T-infra, it was remarked that this block of changes are hard to review, so we agreed a plan to split into smaller bites. This PR will stay as tracking issues for the work being split into smaller pull requests:

Step 1: Change pull request assignment algo (Add new PR review assignment #1745 ):
- create new DB table, just store there "how busy ppl are"
- add new mechanism to assign pull requests
- add CLI tool to populate the new DB table and synchronize it. Make it so that it will speak HTTP instead of using public triagebot APIs.
Step 2: Implement the backoffice where people can set themselves on time-off.
- TBD: standalone app or inside the Triagebot codebase?

A slightly old design document is at this HackMD link.

This proposal was discussed separately in a number of meetings with the compiler team (also mentioned to @Mark-Simulacrum).

Summary

The current pull request assignment is basically randomly assigned among team members. The new proposed workflow has the following features:

each Rust project contributor belonging to a team (I'd start will a small cohort of testers) can set their own review capacity and time off using a web backoffice
the web backoffice will persiste these data in a DB
everytime a pull request assignment is invoked with r? <team> or <team_member> the triagebot will check the current availability and workload of the candidates and assign the pull request to the team member less busy, excluding automatically those that are on time off in that moment.

Permissions and Authorization

This backoffice is a simple form listing all team members. It is served by the triagebot (from triagebot.infra.rust-lang.org/). Access to this backoffice is gated by a GitHub App. Users allowed to this backoffice:

must be logged into GitHub
must agree to the share some details with a GitHub App (only to read their profile)
must be a Rust project team member
the team they belong must be whitelisted in the env var NEW_PR_ASSIGNMENT_TEAMS (see documentation)

Team leaders are considered administrators and can see the preferences of every team member. Normal team members can only see their own preferences and those of team members that agree to share theirs within the team.

DEPLOYMENT

There are a few env vars to enable the new PR assignment and retrict access to this backoffice (see: .env.sample).

My idea would be to iterate on this pull request and iron out all the wrinkles visible without deployment:

Deploy this changes but keep the new PR assignment DISABLED. The web backoffice will still be accessible.
Team members can access that, start playing with with it fill their preferences
When we are all confident about these changes, we can flip the env variable USE_NEW_PR_ASSIGNMENT and have the team listed in NEW_PR_ASSIGNMENT_TEAMS use it for real.
After a first period of tests, add more teams

Of course, happy to receive inputs!

TODO

I also left a few notes about a few items to be implemented, in my opinion also in a second interation; see the documentation.

(I will iterate on the pull request description to add context)

apiraino

Left a few comments to provide more context.

apiraino · 2023-09-12T15:06:39Z

Cargo.toml

+crypto-hash = { version = "0.3.4", default-features = false }
+flate2 = { version = "1.0.27", default-features = false, features = ["zlib-ng"] }


These new crates are used to decrypt and encrypt a digest set in the cookie for users returning to the backoffice

apiraino · 2023-09-12T15:09:43Z

src/handlers/assign.rs

+    if use_new_pr_assignment(teams, &candidates) {
+        log::debug!("Using NEW pull request assignment workflow");
+        assignee = new_find_reviewer_from_names(db_client, &candidates).await;
+    } else {
+        log::debug!("Using OLD pull request assignment workflow");
+        let username = old_find_reviewer_from_names(candidates);
+        assignee = Ok(ReviewCapacityUser::phony(username.unwrap()));
+    }


The new pull request assignment can be enabled or disengaged using an env var (USE_NEW_PR_ASSIGNMENT) when deploying the Triagebot. This conditional allows using the current workflow or the new one.

apiraino · 2023-09-12T15:11:00Z

src/lib.rs

+    }
+}
+
+impl From<HashMap<String, String>> for ReviewCapacityUser {


this From parses the form from the backoffice. I'm not too happy about this code though ...

apiraino · 2023-09-12T15:13:05Z

src/main.rs

@@ -145,6 +182,230 @@ async fn serve_req(
            .body(Body::from(triagebot::zulip::respond(&ctx, req).await))
            .unwrap());
    }
+
+    if req.uri.path() == "/review-settings" {


/review-settings is a new endpoint of the Triagebot that will allow access to the backoffice to set the pull request assignment preferences.

Here there is quite some code to authenticate the user. could probably be split and refactored a bit for better clarity

apiraino · 2023-09-12T15:21:41Z

src/main.rs

+fn create_cookie_content(user_login: &str, user_id: i64) -> serde_json::Value {
+    let auth_secret = std::env::var("BACKOFFICE_SECRET").expect("BACKOFFICE_SECRET is not set");
+    let exp = Utc::now() + Duration::minutes(30);
+    let digest = format!("{};{};{}", user_id, user_login, auth_secret);
+    let digest = hex_digest(Algorithm::SHA256, &digest.into_bytes());
+    serde_json::json!({"iss":"triagebot", "sub":user_login, "uid": user_id, "exp":exp, "checksum":digest})
+}


Everytime users log in, a cookie with this content will be saved on their browser. If the cookie is not found, the user will be redirected to the GitHub auth login workflow again

apiraino · 2023-09-12T15:25:17Z

src/triage.rs

@@ -17,6 +18,34 @@ pub fn index() -> Result<Response<Body>, hyper::Error> {
        .unwrap())
 }

+pub async fn asset(file: &str) -> Result<Response<Body>, hyper::Error> {


The backoffice started as an HTML page with a separate JavaScript and CSS file, so I thought to add a static asset endpoint. Then I moved everything to the server, the JavaScript was basically removed. scripts and stylesheets are now embedded in the HTML file (~4kb with gzip compression).

This asset path is now only used to serve 2 SVG. Unsure if we want to keep this new endpoint after all :)

apiraino · 2023-09-14T08:40:31Z

src/handlers/review_prefs.rs

+SELECT username, r.*, sum(r.max_assigned_prs - r.num_assigned_prs) as avail_slots
+FROM review_capacity r
+JOIN users on users.user_id=r.user_id
+WHERE active = true
+AND current_date NOT BETWEEN pto_date_start AND pto_date_end
+AND username = ANY('{{ {} }}')
+AND num_assigned_prs < max_assigned_prs
+GROUP BY username, r.id
+ORDER BY avail_slots DESC
+LIMIT 1",


The core of the new PR assignment is here: This SQL query will retrieve all active team members and sort them by "avail slots" (max # of PRs - # of PRs assigned)

This CLI tool insert or sync all members defined in $NEW_PR_ASSIGNMENT_TEAMS into the review preferences DB table.

apiraino · 2023-12-12T15:11:54Z

Closing and restarting from scratch in #1753

apiraino added 5 commits September 12, 2023 15:35

Add pull request preferences backoffice

050e488

PR preferences backoffice impl

f7e3e97

Avoid triggering spurious triagebot command

4059c97

Update dependencies and documentation

30dceea

Add PR backoffice logic

8087779

apiraino commented Sep 12, 2023

View reviewed changes

apiraino commented Sep 14, 2023

View reviewed changes

apiraino added 3 commits September 14, 2023 10:41

Add more TODO items

6a13ed6

Remove redundant teams members download

48c4f26

Initial setup of review preferences

ce8b243

This CLI tool insert or sync all members defined in $NEW_PR_ASSIGNMENT_TEAMS into the review preferences DB table.

apiraino force-pushed the pr-review-prefs-3 branch from 35a93f8 to 5d9e2d5 Compare September 30, 2023 09:46

Update documentation

e8cc894

apiraino force-pushed the pr-review-prefs-3 branch from 5d9e2d5 to e8cc894 Compare October 31, 2023 14:28

apiraino marked this pull request as draft November 8, 2023 13:37

Merge branch 'master' into pr-review-prefs-3

2fb864e

apiraino mentioned this pull request Nov 15, 2023

Add new PR review assignment #1745

Closed

apiraino mentioned this pull request Dec 12, 2023

[Tracking] New pull request assignment proposal #1753

Open

8 tasks

apiraino closed this Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New pull request assignment proposal #1719

New pull request assignment proposal #1719

apiraino commented Sep 12, 2023 •

edited

Loading

apiraino left a comment

apiraino Sep 12, 2023

apiraino Sep 12, 2023

apiraino Sep 12, 2023

apiraino Sep 12, 2023 •

edited

Loading

apiraino Sep 12, 2023

apiraino Sep 12, 2023 •

edited

Loading

apiraino Sep 14, 2023

apiraino commented Dec 12, 2023

		crypto-hash = { version = "0.3.4", default-features = false }
		flate2 = { version = "1.0.27", default-features = false, features = ["zlib-ng"] }

New pull request assignment proposal #1719

New pull request assignment proposal #1719

Conversation

apiraino commented Sep 12, 2023 • edited Loading

Summary

Permissions and Authorization

DEPLOYMENT

TODO

apiraino left a comment

Choose a reason for hiding this comment

apiraino Sep 12, 2023

Choose a reason for hiding this comment

apiraino Sep 12, 2023

Choose a reason for hiding this comment

apiraino Sep 12, 2023

Choose a reason for hiding this comment

apiraino Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

apiraino Sep 12, 2023

Choose a reason for hiding this comment

apiraino Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

apiraino Sep 14, 2023

Choose a reason for hiding this comment

apiraino commented Dec 12, 2023

apiraino commented Sep 12, 2023 •

edited

Loading

apiraino Sep 12, 2023 •

edited

Loading

apiraino Sep 12, 2023 •

edited

Loading