-
Notifications
You must be signed in to change notification settings - Fork 46
feat(rollups): add rollup_contents mapping #519
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
base: main
Are you sure you want to change the base?
Changes from all commits
41da37f
8dff580
d76bba4
0c5aeee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| /.sqlx/*.json binary linguist-generated=true | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| DROP TABLE IF EXISTS rollup_member; |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,11 @@ | ||||||||||||||
| -- Add up migration script here | ||||||||||||||
| CREATE TABLE IF NOT EXISTS rollup_member | ||||||||||||||
| ( | ||||||||||||||
| -- We have to store the PR through their numbers rather than a FK to the pull_request table. This is due to the rollup PR | ||||||||||||||
| -- being created at the time of record insertion in this table, so we won't have an entry in pull_request for it | ||||||||||||||
| rollup_pr_number BIGINT NOT NULL, | ||||||||||||||
| member_pr_number BIGINT NOT NULL, | ||||||||||||||
| repository TEXT NOT NULL, | ||||||||||||||
|
Comment on lines
+6
to
+8
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Just a nit, it's easier to read like this when inspecting the DB state manually :) The migration data SQL file will also need to be updated after this change. |
||||||||||||||
|
|
||||||||||||||
| PRIMARY KEY (repository, rollup_pr_number, member_pr_number) | ||||||||||||||
| ); | ||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,8 +1,11 @@ | ||||||||
| use sqlx::PgPool; | ||||||||
| use std::collections::{HashMap, HashSet}; | ||||||||
|
|
||||||||
| use crate::bors::comment::CommentTag; | ||||||||
| use crate::bors::{BuildKind, PullRequestStatus, RollupMode}; | ||||||||
| use crate::database::operations::update_pr_auto_build_id; | ||||||||
| use crate::database::operations::{ | ||||||||
| get_nonclosed_rollups, register_rollup_pr_member, update_pr_auto_build_id, | ||||||||
| }; | ||||||||
| use crate::database::{ | ||||||||
| BuildModel, BuildStatus, CommentModel, PullRequestModel, RepoModel, TreeState, WorkflowModel, | ||||||||
| WorkflowStatus, WorkflowType, | ||||||||
|
|
@@ -336,4 +339,29 @@ impl PgDbClient { | |||||||
| pub async fn delete_tagged_bot_comment(&self, comment: &CommentModel) -> anyhow::Result<()> { | ||||||||
| delete_tagged_bot_comment(&self.pool, comment.id).await | ||||||||
| } | ||||||||
|
|
||||||||
| /// Register the contents of a rollup in the DB. | ||||||||
| /// The contents are stored as rollup PR number - member PR number records, scoped under a specific repository. | ||||||||
| /// All records are inserted in a single transaction. | ||||||||
| pub async fn register_rollup_members( | ||||||||
| &self, | ||||||||
| repo: &GithubRepoName, | ||||||||
| rollup_pr_number: &PullRequestNumber, | ||||||||
| member_pr_numbers: &[PullRequestNumber], | ||||||||
| ) -> anyhow::Result<()> { | ||||||||
| let mut tx = self.pool.begin().await?; | ||||||||
| for member_pr_number in member_pr_numbers { | ||||||||
| register_rollup_pr_member(&mut *tx, repo, rollup_pr_number, member_pr_number).await?; | ||||||||
| } | ||||||||
| tx.commit().await?; | ||||||||
| Ok(()) | ||||||||
| } | ||||||||
|
|
||||||||
| /// Returns a map of rollup PR numbers to the set of member PR numbers that are part of that rollup. | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| pub async fn get_nonclosed_rollups( | ||||||||
| &self, | ||||||||
| repo: &GithubRepoName, | ||||||||
| ) -> anyhow::Result<HashMap<PullRequestNumber, HashSet<PullRequestNumber>>> { | ||||||||
| get_nonclosed_rollups(&self.pool, repo).await | ||||||||
| } | ||||||||
| } | ||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||
| use chrono::DateTime; | ||||
| use chrono::Utc; | ||||
| use sqlx::postgres::PgExecutor; | ||||
| use std::collections::{HashMap, HashSet}; | ||||
|
|
||||
| use super::ApprovalInfo; | ||||
| use super::ApprovalStatus; | ||||
|
|
@@ -1064,3 +1065,60 @@ pub(crate) async fn clear_auto_build( | |||
| }) | ||||
| .await | ||||
| } | ||||
|
|
||||
| pub(crate) async fn register_rollup_pr_member( | ||||
| executor: impl PgExecutor<'_>, | ||||
| repo: &GithubRepoName, | ||||
| rollup_pr_number: &PullRequestNumber, | ||||
| member_pr_number: &PullRequestNumber, | ||||
| ) -> anyhow::Result<()> { | ||||
| measure_db_query("register_rollup_pr_member", || async { | ||||
| sqlx::query!( | ||||
| r#" | ||||
| INSERT INTO rollup_member (repository, rollup_pr_number, member_pr_number) | ||||
| VALUES ($1, $2, $3) | ||||
| "#, | ||||
| repo as &GithubRepoName, | ||||
| rollup_pr_number.0 as i32, | ||||
| member_pr_number.0 as i32 | ||||
| ) | ||||
| .execute(executor) | ||||
| .await?; | ||||
| Ok(()) | ||||
| }) | ||||
| .await | ||||
| } | ||||
|
|
||||
| pub(crate) async fn get_nonclosed_rollups( | ||||
| executor: impl PgExecutor<'_>, | ||||
| repo: &GithubRepoName, | ||||
| ) -> anyhow::Result<HashMap<PullRequestNumber, HashSet<PullRequestNumber>>> { | ||||
| measure_db_query("get_nonclosed_rollups", || async { | ||||
| let mut stream = sqlx::query!( | ||||
| r#" | ||||
| SELECT rm.rollup_pr_number, rm.member_pr_number | ||||
| FROM rollup_member rm | ||||
| JOIN pull_request pr | ||||
| ON pr.repository = rm.repository | ||||
| AND pr.number = rm.rollup_pr_number | ||||
| WHERE rm.repository = $1 | ||||
| AND pr.status IN ('open', 'draft') | ||||
| ORDER BY rm.rollup_pr_number; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Doesn't seem to be required, since we store the numbers in a hashmap/hashset anyway? Might help with cache locality for the hashmap, but that's not really needed here, and the DB won't have to sort the rollups. |
||||
| "#, | ||||
| repo as &GithubRepoName, | ||||
| ) | ||||
| .fetch(executor); | ||||
|
|
||||
| let mut rollups_map: HashMap<PullRequestNumber, HashSet<PullRequestNumber>> = | ||||
| HashMap::new(); | ||||
| while let Some(row) = stream.try_next().await? { | ||||
| let rollup_pr = PullRequestNumber(row.rollup_pr_number as u64); | ||||
| let member_pr = PullRequestNumber(row.member_pr_number as u64); | ||||
|
|
||||
| rollups_map.entry(rollup_pr).or_default().insert(member_pr); | ||||
| } | ||||
|
|
||||
| Ok(rollups_map) | ||||
| }) | ||||
| .await | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,7 +183,6 @@ async fn create_rollup( | |
| repo_owner, | ||
| mut pr_nums, | ||
| } = rollup_state; | ||
|
|
||
| // Repository where we will create the PR | ||
| let base_repo = GithubRepoName::new(&repo_owner, &repo_name); | ||
|
|
||
|
|
@@ -388,6 +387,14 @@ async fn create_rollup( | |
| .await | ||
| .context("Cannot create PR")?; | ||
|
|
||
| db.register_rollup_members( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking about (many!) follow-up features that we could implement once this PR lands, and I realized that the problem with the missing PR id can be solved very easily. We can simply pre-insert the PR into the DB immediately here: let pr_db = db.upsert_pull_request(..., pr.into());and then use Since we anyway have to do a Could you please change it to store
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense! Some questions:
I'm always looking for issues to contribute to (mostly during weekends, when i'm not too tired from working in a startup haha). If you want, write down the ideas in GitHub issues and I'll take care of them!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Btw, after this change you can remove Okay, I will create issues for the features, once this PR lands. |
||
| &base_repo, | ||
| &pr.number, | ||
| &successes.iter().map(|pr| pr.number).collect::<Vec<_>>(), | ||
| ) | ||
| .await | ||
| .context("Cannot register rollup member record")?; | ||
|
|
||
| // Set the rollup label | ||
| gh_client | ||
| .add_labels(pr.number, &["rollup".to_string()]) | ||
|
|
@@ -406,6 +413,7 @@ mod tests { | |
| User, default_repo_name, run_test, | ||
| }; | ||
| use http::StatusCode; | ||
| use std::collections::{HashMap, HashSet}; | ||
|
|
||
| #[sqlx::test] | ||
| async fn rollup_missing_fork(pool: sqlx::PgPool) { | ||
|
|
@@ -528,6 +536,17 @@ mod tests { | |
| make_rollup(ctx, &[&pr2, &pr3, &pr4, &pr5]) | ||
| .await? | ||
| .assert_status(StatusCode::SEE_OTHER); | ||
| assert_eq!( | ||
| ctx.db().get_nonclosed_rollups(&default_repo_name()).await?, | ||
| HashMap::from([( | ||
| PullRequestNumber(6), | ||
| HashSet::from_iter(vec![ | ||
| PullRequestNumber(2), | ||
| PullRequestNumber(3), | ||
| PullRequestNumber(5) | ||
| ]) | ||
| )]) | ||
| ); | ||
| Ok(()) | ||
| }) | ||
| .await; | ||
|
|
@@ -650,7 +669,57 @@ mod tests { | |
| insta::assert_snapshot!(pr_url, @"https://github.com/rust-lang/borstest/pull/4"); | ||
|
|
||
| ctx.pr(4).await.expect_added_labels(&["rollup"]); | ||
| assert_eq!( | ||
| ctx.db().get_nonclosed_rollups(&pr2.repo).await?, | ||
| HashMap::from([( | ||
| PullRequestNumber(4), | ||
| HashSet::from_iter(vec![PullRequestNumber(3), PullRequestNumber(2)]) | ||
| )]) | ||
| ); | ||
|
|
||
| Ok(()) | ||
| }) | ||
| .await; | ||
| let repo = gh.get_repo(()); | ||
| insta::assert_snapshot!(repo.lock().get_pr(4).description, @" | ||
| Successful merges: | ||
|
|
||
| - rust-lang/borstest#2 (Title of PR 2) | ||
| - rust-lang/borstest#3 (Title of PR 3) | ||
|
|
||
| r? @ghost | ||
|
|
||
| <!-- homu-ignore:start --> | ||
| [Create a similar rollup](https://bors-test.com/queue/borstest?prs=2,3) | ||
| <!-- homu-ignore:end --> | ||
| "); | ||
| } | ||
|
|
||
| /// Ensure that a PR can be associated to multiple rollups. | ||
| /// This can happen when opening a new rollup similar to an existing one, before closing the old one. | ||
| #[sqlx::test] | ||
| async fn multiple_rollups_same_pr(pool: sqlx::PgPool) { | ||
| let gh = run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { | ||
| let pr2 = ctx.open_pr((), |_| {}).await?; | ||
| let pr3 = ctx.open_pr((), |_| {}).await?; | ||
| ctx.approve(pr2.id()).await?; | ||
| ctx.approve(pr3.id()).await?; | ||
|
|
||
| make_rollup(ctx, &[&pr2, &pr3]).await?; | ||
| make_rollup(ctx, &[&pr3]).await?; | ||
| assert_eq!( | ||
Carbonhell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ctx.db().get_nonclosed_rollups(&pr2.repo).await?, | ||
| HashMap::from([ | ||
| ( | ||
| PullRequestNumber(4), | ||
| HashSet::from_iter(vec![PullRequestNumber(2), PullRequestNumber(3)]) | ||
| ), | ||
| ( | ||
| PullRequestNumber(5), | ||
| HashSet::from_iter(vec![PullRequestNumber(3)]) | ||
| ) | ||
| ]) | ||
| ); | ||
| Ok(()) | ||
| }) | ||
| .await; | ||
|
|
@@ -799,12 +868,16 @@ also include this pls" | |
| prs: &[&PullRequest], | ||
| ) -> anyhow::Result<ApiResponse> { | ||
| let prs = prs.iter().map(|pr| pr.number).collect::<Vec<_>>(); | ||
| ctx.api_request(rollup_request( | ||
| &rollup_user().name, | ||
| default_repo_name(), | ||
| &prs, | ||
| )) | ||
| .await | ||
| let response = ctx | ||
| .api_request(rollup_request( | ||
| &rollup_user().name, | ||
| default_repo_name(), | ||
| &prs, | ||
| )) | ||
| .await; | ||
| // Trigger a refresh so that the new rollup PR can be upserted in the pull_request table | ||
| ctx.refresh_prs().await; | ||
Kobzol marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| response | ||
| } | ||
|
|
||
| fn rollup_state() -> GitHub { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.