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

PR assignment based on work queue availability (take #2) #1793

Conversation

apiraino
Copy link
Contributor

@apiraino apiraino commented Apr 16, 2024

This is a version of #1786 that fixes the two following bugs:

Fixes 2 bugs:

  1. the initial migration wasn't formatted correctly and the new schema changes weren't applied

  2. The SELECT to find a reviewer would return wrong results when a team member had NULL in the table review_prefs.max_assigned_prs

r? @jackh726

This is a version of rust-lang#1786 that fixes the two following bugs:

There were 2 bugs:

1. the initial migration wasn't formatted correctly and the new schema changes weren't applied

2. The SELECT to find a reviewer would return wrong results when a team member had NULL in the table `review_prefs.max_assigned_prs`
Comment on lines +335 to +341
CREATE EXTENSION IF NOT EXISTS intarray;",
"
CREATE UNIQUE INDEX IF NOT EXISTS review_prefs_user_id ON review_prefs(user_id);
",
"
ALTER TABLE review_prefs ADD COLUMN IF NOT EXISTS max_assigned_prs INTEGER DEFAULT NULL;
",
Copy link
Contributor Author

@apiraino apiraino Apr 16, 2024

Choose a reason for hiding this comment

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

The migration should now be correctly formatted.

Adding the clause IF NOT EXISTS should address the concern addressed in this comment about re-running them.

cc: @Mark-Simulacrum

FROM review_prefs r
JOIN users ON users.user_id = r.user_id
WHERE username = $1
AND ( CARDINALITY(r.assigned_prs) < max_assigned_prs OR max_assigned_prs IS NULL );";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old version of this SQL query (here) would fail to find a reviewer if the candidate had max_assignment_prs was NULL

Copy link
Contributor Author

@apiraino apiraino left a comment

Choose a reason for hiding this comment

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

I'm going to add a bit of documentation about the migrations.

Apart from that, hopefully this should be fine.

@jackh726
Copy link
Member

Okay, let's try this again.

@jackh726 jackh726 merged commit 05e40cb into rust-lang:master Apr 16, 2024
2 checks passed
@jackh726
Copy link
Member

For sure if this is still problematic, we need to consider splitting this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants