From 73e2a99e4505f9a27635255629ebca3e60c1e96a Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Wed, 25 Dec 2024 02:14:00 +0000 Subject: [PATCH 1/8] implement the pr label feature this commit lays the ground work for adding labels to PRs depending on the CI Run Status. --- .../2024-12-25-001112_label_state/down.sql | 1 + .../2024-12-25-001112_label_state/up.sql | 1 + migrations/schema.patch | 6 +- migrations/schema.unpatched.rs | 6 ++ server/src/command/cancel.rs | 7 +- server/src/command/dry_run.rs | 15 +-- server/src/command/merge.rs | 10 +- server/src/command/retry.rs | 10 +- server/src/database/pr.rs | 38 ++++--- server/src/database/schema.rs | 6 ++ server/src/github/label_state.rs | 99 +++++++++++++++++++ server/src/github/merge_workflow.rs | 67 ++++++++----- server/src/github/mod.rs | 1 + server/src/github/repo.rs | 70 ++++++++++++- server/src/webhook/pull_request.rs | 3 +- 15 files changed, 288 insertions(+), 52 deletions(-) create mode 100644 migrations/2024-12-25-001112_label_state/down.sql create mode 100644 migrations/2024-12-25-001112_label_state/up.sql create mode 100644 server/src/github/label_state.rs diff --git a/migrations/2024-12-25-001112_label_state/down.sql b/migrations/2024-12-25-001112_label_state/down.sql new file mode 100644 index 0000000..d50fbf9 --- /dev/null +++ b/migrations/2024-12-25-001112_label_state/down.sql @@ -0,0 +1 @@ +ALTER TABLE github_pr DROP COLUMN added_label; diff --git a/migrations/2024-12-25-001112_label_state/up.sql b/migrations/2024-12-25-001112_label_state/up.sql new file mode 100644 index 0000000..eca2373 --- /dev/null +++ b/migrations/2024-12-25-001112_label_state/up.sql @@ -0,0 +1 @@ +ALTER TABLE github_pr ADD COLUMN added_label TEXT; diff --git a/migrations/schema.patch b/migrations/schema.patch index 29b668d..2e39a9f 100644 --- a/migrations/schema.patch +++ b/migrations/schema.patch @@ -1,5 +1,5 @@ ---- migrations/schema.unpatched.rs 2024-12-21 01:00:50.435153015 +0000 -+++ server/src/database/schema.rs 2024-12-21 00:55:41.850491800 +0000 +--- migrations/schema.unpatched.rs 2024-12-25 01:43:39.324295195 +0000 ++++ server/src/database/schema.rs 2024-12-25 01:43:35.592373517 +0000 @@ -1,38 +1,44 @@ +#![cfg_attr(coverage_nightly, coverage(off))] // @generated automatically by Diesel CLI. @@ -85,7 +85,7 @@ /// /// (Automatically generated by Diesel.) status -> GithubPrStatus, -@@ -320,12 +326,13 @@ +@@ -326,12 +332,13 @@ updated_at -> Timestamptz, } } diff --git a/migrations/schema.unpatched.rs b/migrations/schema.unpatched.rs index ecb201f..8b82e12 100644 --- a/migrations/schema.unpatched.rs +++ b/migrations/schema.unpatched.rs @@ -298,6 +298,12 @@ diesel::table! { /// /// (Automatically generated by Diesel.) default_priority -> Nullable, + /// The `added_label` column of the `github_pr` table. + /// + /// Its SQL type is `Nullable`. + /// + /// (Automatically generated by Diesel.) + added_label -> Nullable, } } diff --git a/server/src/command/cancel.rs b/server/src/command/cancel.rs index 4646106..bc5633e 100644 --- a/server/src/command/cancel.rs +++ b/server/src/command/cancel.rs @@ -23,7 +23,7 @@ async fn handle_with_pr( pr: PullRequest, context: BrawlCommandContext<'_, R>, ) -> anyhow::Result<()> { - Pr::new(&pr, context.user.id, context.repo.id()) + let db_pr = Pr::new(&pr, context.user.id, context.repo.id()) .upsert() .get_result(conn) .await @@ -49,7 +49,7 @@ async fn handle_with_pr( context .repo .merge_workflow() - .cancel(&run, context.repo, conn) + .cancel(&run, context.repo, conn, &db_pr) .await .context("cancel ci run")?; @@ -89,8 +89,9 @@ pub mod tests { run: &CiRun<'_>, repo: &impl GitHubRepoClient, conn: &mut AsyncPgConnection, + pr: &Pr<'_>, ) -> anyhow::Result<()> { - let _ = (run, repo, conn); + let _ = (run, repo, conn, pr); if self .cancelled .compare_exchange( diff --git a/server/src/command/dry_run.rs b/server/src/command/dry_run.rs index 347b99f..d97130c 100644 --- a/server/src/command/dry_run.rs +++ b/server/src/command/dry_run.rs @@ -80,6 +80,12 @@ async fn handle_with_pr( let branch = context.repo.config().try_branch(pr.number); + let db_pr = Pr::new(&pr, context.user.id, context.repo.id()) + .upsert() + .get_result(conn) + .await + .context("update pr")?; + if let Some(run) = CiRun::active(context.repo.id(), pr.number) .get_result(conn) .await @@ -90,7 +96,7 @@ async fn handle_with_pr( context .repo .merge_workflow() - .cancel(&run, context.repo, conn) + .cancel(&run, context.repo, conn, &db_pr) .await .context("cancel ci run")?; } else { @@ -115,12 +121,6 @@ async fn handle_with_pr( } } - let db_pr = Pr::new(&pr, context.user.id, context.repo.id()) - .upsert() - .get_result(conn) - .await - .context("update pr")?; - let run = CiRun::insert(context.repo.id(), pr.number) .base_ref(base) .head_commit_sha(command.head_sha.as_deref().unwrap_or_else(|| pr.head.sha.as_ref()).into()) @@ -199,6 +199,7 @@ mod tests { run: &CiRun<'_>, _: &impl GitHubRepoClient, conn: &mut AsyncPgConnection, + _: &Pr<'_>, ) -> anyhow::Result<()> { if self .cancelled diff --git a/server/src/command/merge.rs b/server/src/command/merge.rs index 7c056b1..9a547ee 100644 --- a/server/src/command/merge.rs +++ b/server/src/command/merge.rs @@ -104,7 +104,7 @@ async fn handle_with_pr( .get_result(conn) .await?; - context.repo.merge_workflow().queued(&run, context.repo).await?; + context.repo.merge_workflow().queued(&run, context.repo, conn, &db_pr).await?; Ok(()) } @@ -131,7 +131,13 @@ mod tests { } impl GitHubMergeWorkflow for MockMergeWorkFlow { - async fn queued(&self, _: &CiRun<'_>, _: &impl GitHubRepoClient) -> anyhow::Result<()> { + async fn queued( + &self, + _: &CiRun<'_>, + _: &impl GitHubRepoClient, + _: &mut AsyncPgConnection, + _: &Pr<'_>, + ) -> anyhow::Result<()> { self.queued .compare_exchange( false, diff --git a/server/src/command/retry.rs b/server/src/command/retry.rs index 87182b0..fd5ff9a 100644 --- a/server/src/command/retry.rs +++ b/server/src/command/retry.rs @@ -83,7 +83,7 @@ async fn handle_with_pr( if run.is_dry_run { context.repo.merge_workflow().start(&run, context.repo, conn, &db_pr).await?; } else { - context.repo.merge_workflow().queued(&run, context.repo).await?; + context.repo.merge_workflow().queued(&run, context.repo, conn, &db_pr).await?; } Ok(()) @@ -132,7 +132,13 @@ mod tests { Ok(true) } - async fn queued(&self, _: &CiRun<'_>, _: &impl GitHubRepoClient) -> anyhow::Result<()> { + async fn queued( + &self, + _: &CiRun<'_>, + _: &impl GitHubRepoClient, + _: &mut AsyncPgConnection, + _: &Pr<'_>, + ) -> anyhow::Result<()> { self.queued .compare_exchange( false, diff --git a/server/src/database/pr.rs b/server/src/database/pr.rs index 7fe8894..f860a89 100644 --- a/server/src/database/pr.rs +++ b/server/src/database/pr.rs @@ -30,6 +30,7 @@ pub struct Pr<'a> { pub target_branch: Cow<'a, str>, pub source_branch: Cow<'a, str>, pub latest_commit_sha: Cow<'a, str>, + pub added_label: Option>, pub created_at: chrono::DateTime, pub updated_at: chrono::DateTime, } @@ -42,6 +43,7 @@ pub struct UpdatePr<'a> { pub github_repo_id: i64, #[builder(start_fn)] pub github_pr_number: i32, + pub added_label: Option>>, pub title: Option>, pub body: Option>, pub merge_status: Option, @@ -110,6 +112,7 @@ impl<'a> Pr<'a> { target_branch: Cow::Borrowed(&pr.base.ref_field), source_branch: Cow::Borrowed(&pr.head.ref_field), latest_commit_sha: Cow::Borrowed(&pr.head.sha), + added_label: None, created_at: chrono::Utc::now(), updated_at: chrono::Utc::now(), } @@ -138,6 +141,7 @@ impl<'a> Pr<'a> { title: Some(Cow::Borrowed(self.title.as_ref())), body: Some(Cow::Borrowed(self.body.as_ref())), merge_status: Some(self.merge_status), + added_label: Some(self.added_label.as_deref().map(Cow::Borrowed)), assigned_ids: Some(self.assigned_ids.clone()), status: Some(self.status), default_priority: Some(self.default_priority), @@ -236,6 +240,7 @@ mod tests { "github_pr"."target_branch", "github_pr"."source_branch", "github_pr"."latest_commit_sha", + "github_pr"."added_label", "github_pr"."created_at", "github_pr"."updated_at" FROM @@ -263,6 +268,7 @@ mod tests { latest_commit_sha: Cow::Borrowed("test"), source_branch: Cow::Borrowed("test"), target_branch: Cow::Borrowed("test"), + added_label: None, merge_status: GithubPrMergeStatus::NotReady, status: GithubPrStatus::Open, merge_commit_sha: None, @@ -283,6 +289,7 @@ mod tests { "target_branch", "source_branch", "latest_commit_sha", + "added_label", "created_at", "updated_at" ) @@ -301,6 +308,7 @@ mod tests { $9, $10, $11, + DEFAULT, $12, $13 ) -- binds: [1, 1, "test", "test", NotReady, 0, [], Open, "test", "test", "test", 2024-06-20T02:40:00Z, 2024-06-20T02:40:00Z] @@ -360,6 +368,7 @@ mod tests { updated_at: chrono::DateTime::from_timestamp_nanos(1718851200000000000), assigned_ids: vec![], author_id: 0, + added_label: None, default_priority: None, latest_commit_sha: Cow::Borrowed("test"), source_branch: Cow::Borrowed("test"), @@ -384,6 +393,7 @@ mod tests { "target_branch", "source_branch", "latest_commit_sha", + "added_label", "created_at", "updated_at" ) @@ -402,21 +412,23 @@ mod tests { $9, $10, $11, + DEFAULT, $12, $13 ) ON CONFLICT ("github_repo_id", "github_pr_number") DO UPDATE SET - "title" = $14, - "body" = $15, - "merge_status" = $16, - "assigned_ids" = $17, - "status" = $18, - "default_priority" = $19, - "merge_commit_sha" = $20, - "target_branch" = $21, - "latest_commit_sha" = $22, - "updated_at" = $23 + "added_label" = $14, + "title" = $15, + "body" = $16, + "merge_status" = $17, + "assigned_ids" = $18, + "status" = $19, + "default_priority" = $20, + "merge_commit_sha" = $21, + "target_branch" = $22, + "latest_commit_sha" = $23, + "updated_at" = $24 RETURNING "github_pr"."github_repo_id", "github_pr"."github_pr_number", @@ -431,8 +443,9 @@ mod tests { "github_pr"."target_branch", "github_pr"."source_branch", "github_pr"."latest_commit_sha", + "github_pr"."added_label", "github_pr"."created_at", - "github_pr"."updated_at" -- binds: [1, 1, "test", "test", NotReady, 0, [], Open, "test", "test", "test", 2024-06-20T02:40:00Z, 2024-06-20T02:40:00Z, "test", "test", NotReady, [], Open, None, None, "test", "test", 2024-06-20T02:40:00Z] + "github_pr"."updated_at" -- binds: [1, 1, "test", "test", NotReady, 0, [], Open, "test", "test", "test", 2024-06-20T02:40:00Z, 2024-06-20T02:40:00Z, None, "test", "test", NotReady, [], Open, None, None, "test", "test", 2024-06-20T02:40:00Z] "#, } @@ -516,6 +529,7 @@ mod tests { title: Cow::Borrowed("test"), assigned_ids: vec![], author_id: 0, + added_label: None, default_priority: None, merge_status: GithubPrMergeStatus::NotReady, status: GithubPrStatus::Open, @@ -547,6 +561,7 @@ mod tests { assigned_ids: vec![], author_id: 0, default_priority: None, + added_label: None, merge_status: GithubPrMergeStatus::NotReady, status: GithubPrStatus::Open, merge_commit_sha: None, @@ -584,6 +599,7 @@ mod tests { title: Cow::Borrowed("test"), assigned_ids: vec![], author_id: 0, + added_label: None, default_priority: None, merge_status: GithubPrMergeStatus::NotReady, status: GithubPrStatus::Open, diff --git a/server/src/database/schema.rs b/server/src/database/schema.rs index 7a72f8c..5666cc4 100644 --- a/server/src/database/schema.rs +++ b/server/src/database/schema.rs @@ -304,6 +304,12 @@ diesel::table! { /// /// (Automatically generated by Diesel.) default_priority -> Nullable, + /// The `added_label` column of the `github_pr` table. + /// + /// Its SQL type is `Nullable`. + /// + /// (Automatically generated by Diesel.) + added_label -> Nullable, } } diff --git a/server/src/github/label_state.rs b/server/src/github/label_state.rs new file mode 100644 index 0000000..6f1f1ad --- /dev/null +++ b/server/src/github/label_state.rs @@ -0,0 +1,99 @@ +use std::borrow::Cow; + +use diesel_async::{AsyncPgConnection, RunQueryDsl}; + +use super::config::GitHubBrawlLabelsConfig; +use super::repo::GitHubRepoClient; +use crate::database::enums::GithubCiRunStatus; +use crate::database::pr::Pr; + +fn desired_labels<'a>(status: GithubCiRunStatus, is_dry_run: bool, config: &'a GitHubBrawlLabelsConfig) -> Option<&'a str> { + match (status, is_dry_run) { + (GithubCiRunStatus::Queued, false) => { + if let Some(on_merge_queued) = config.on_merge_queued.as_deref() { + return Some(on_merge_queued); + } + } + (GithubCiRunStatus::InProgress, true) => { + if let Some(on_try_in_progress) = config.on_try_in_progress.as_deref() { + return Some(on_try_in_progress); + } + } + (GithubCiRunStatus::InProgress, false) => { + if let Some(on_merge_in_progress) = config.on_merge_in_progress.as_deref() { + return Some(on_merge_in_progress); + } + } + (GithubCiRunStatus::Failure, true) => { + if let Some(on_try_failure) = config.on_try_failure.as_deref() { + return Some(on_try_failure); + } + } + (GithubCiRunStatus::Failure, false) => { + if let Some(on_merge_failure) = config.on_merge_failure.as_deref() { + return Some(on_merge_failure); + } + } + (GithubCiRunStatus::Success, false) => { + if let Some(on_merge_success) = config.on_merge_success.as_deref() { + return Some(on_merge_success); + } + } + _ => {} + } + + None +} + +pub async fn update_labels( + conn: &mut AsyncPgConnection, + pr: &Pr<'_>, + status: GithubCiRunStatus, + is_dry_run: bool, + repo_client: &impl GitHubRepoClient, +) -> anyhow::Result<()> { + let config = repo_client.config(); + let desired_label = desired_labels(status, is_dry_run, &config.labels); + + if desired_label == pr.added_label.as_deref() { + return Ok(()); + } + + if let Some(desired_label) = desired_label { + if let Err(e) = repo_client + .add_labels(pr.github_pr_number as u64, &[desired_label.to_string()]) + .await + { + tracing::error!( + repo_id = pr.github_repo_id, + owner = repo_client.owner(), + repo_name = repo_client.name(), + pr_number = pr.github_pr_number, + "failed to add label: {:#}", + e + ) + } + } + + if let Some(added_label) = pr.added_label.as_deref() { + if let Err(e) = repo_client.remove_label(pr.github_pr_number as u64, added_label).await { + tracing::error!( + repo_id = pr.github_repo_id, + owner = repo_client.owner(), + repo_name = repo_client.name(), + pr_number = pr.github_pr_number, + "failed to remove label: {:#}", + e + ) + } + } + + pr.update() + .added_label(desired_label.map(Cow::Borrowed)) + .build() + .query() + .execute(conn) + .await?; + + Ok(()) +} diff --git a/server/src/github/merge_workflow.rs b/server/src/github/merge_workflow.rs index 0b56655..e2fa68d 100644 --- a/server/src/github/merge_workflow.rs +++ b/server/src/github/merge_workflow.rs @@ -13,6 +13,7 @@ use crate::database::ci_run::{Base, CiRun}; use crate::database::ci_run_check::CiCheck; use crate::database::enums::{GithubCiRunStatus, GithubCiRunStatusCheckStatus}; use crate::database::pr::Pr; +use crate::github::label_state::update_labels; use crate::github::repo::MergeResult; use crate::utils::format_fn; @@ -96,8 +97,9 @@ pub trait GitHubMergeWorkflow: Send + Sync { run: &CiRun<'_>, repo: &impl GitHubRepoClient, conn: &mut AsyncPgConnection, + pr: &Pr<'_>, ) -> impl std::future::Future> + Send { - let _ = (run, repo, conn); + let _ = (run, repo, conn, pr); unimplemented!("GitHubMergeWorkflow::cancel"); #[allow(unreachable_code)] std::future::pending() @@ -108,8 +110,10 @@ pub trait GitHubMergeWorkflow: Send + Sync { &self, run: &CiRun<'_>, repo: &impl GitHubRepoClient, + conn: &mut AsyncPgConnection, + pr: &Pr<'_>, ) -> impl std::future::Future> + Send { - let _ = (run, repo); + let _ = (run, repo, conn, pr); unimplemented!("GitHubMergeWorkflow::queued"); #[allow(unreachable_code)] std::future::pending() @@ -148,8 +152,9 @@ impl GitHubMergeWorkflow for &T { run: &CiRun<'_>, repo: &impl GitHubRepoClient, conn: &mut AsyncPgConnection, + pr: &Pr<'_>, ) -> impl std::future::Future> + Send { - (*self).cancel(run, repo, conn) + (*self).cancel(run, repo, conn, pr) } #[cfg_attr(all(test, coverage_nightly), coverage(off))] @@ -157,8 +162,10 @@ impl GitHubMergeWorkflow for &T { &self, run: &CiRun<'_>, repo: &impl GitHubRepoClient, + conn: &mut AsyncPgConnection, + pr: &Pr<'_>, ) -> impl std::future::Future> + Send { - (*self).queued(run, repo) + (*self).queued(run, repo, conn, pr) } #[cfg_attr(all(test, coverage_nightly), coverage(off))] @@ -179,9 +186,9 @@ impl DefaultMergeWorkflow { async fn fail( &self, run: &CiRun<'_>, + pr: &Pr<'_>, repo: &impl GitHubRepoClient, conn: &mut AsyncPgConnection, - pr_number: i32, message: impl Borrow, ) -> anyhow::Result<()> { if CiRun::update(run.id) @@ -197,7 +204,7 @@ impl DefaultMergeWorkflow { anyhow::bail!("run already completed"); } - repo.send_message(pr_number as u64, message.borrow()) + repo.send_message(pr.github_pr_number as u64, message.borrow()) .await .context("send message")?; @@ -214,6 +221,8 @@ impl DefaultMergeWorkflow { repo.delete_branch(&run.ci_branch).await?; + update_labels(conn, pr, GithubCiRunStatus::Failure, run.is_dry_run, repo).await?; + Ok(()) } @@ -225,8 +234,6 @@ impl DefaultMergeWorkflow { pr: &Pr<'_>, checks: &[&CiCheck<'_>], ) -> anyhow::Result<()> { - println!("success"); - if CiRun::update(run.id) .status(GithubCiRunStatus::Success) .completed_at(chrono::Utc::now()) @@ -292,9 +299,9 @@ impl DefaultMergeWorkflow { Err(e) => { self.fail( run, + pr, repo, conn, - run.github_pr_number, &messages::error( format_fn(|f| writeln!(f, "Tests passed but failed to push to branch {}", pr.target_branch)), e, @@ -332,6 +339,8 @@ impl DefaultMergeWorkflow { } } + update_labels(conn, pr, GithubCiRunStatus::Success, run.is_dry_run, repo).await?; + Ok(()) } } @@ -374,9 +383,9 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { if check.status_check_status == GithubCiRunStatusCheckStatus::Failure { self.fail( run, + pr, repo, conn, - run.github_pr_number, &messages::tests_failed(&check.status_check_name, &check.url), ) .await?; @@ -396,9 +405,9 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { { self.fail( run, + pr, repo, conn, - run.github_pr_number, &messages::tests_timeout( repo.config().timeout_minutes, format_fn(|f| { @@ -437,9 +446,9 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { let Some(branch) = repo.get_ref_latest_commit(&Reference::Branch(branch.to_string())).await? else { self.fail( run, + pr, repo, conn, - run.github_pr_number, &messages::error( format_fn(|f| { writeln!(f, "Failed to find branch {branch}") @@ -500,9 +509,9 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { Ok(MergeResult::Conflict) => { self.fail( run, + pr, repo, conn, - run.github_pr_number, messages::merge_conflict( &pr.source_branch, &pr.target_branch, @@ -517,9 +526,9 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { Err(e) => { self.fail( run, + pr, repo, conn, - run.github_pr_number, &messages::error(format_fn(|f| write!(f, "Failed to create merge commit")), e), ) .await?; @@ -541,9 +550,9 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { Err(e) => { self.fail( run, + pr, repo, conn, - run.github_pr_number, &messages::error( format_fn(|f| write!(f, "Failed to push branch {branch}", branch = run.ci_branch)), e, @@ -572,6 +581,8 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { "ci run started", ); + update_labels(conn, pr, GithubCiRunStatus::InProgress, run.is_dry_run, repo).await?; + Ok(true) } @@ -580,6 +591,7 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { run: &CiRun<'_>, repo: &impl GitHubRepoClient, conn: &mut AsyncPgConnection, + pr: &Pr<'_>, ) -> anyhow::Result<()> { if CiRun::update(run.id) .status(GithubCiRunStatus::Cancelled) @@ -651,10 +663,18 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { repo.delete_branch(&run.ci_branch).await?; } + update_labels(conn, pr, GithubCiRunStatus::Cancelled, run.is_dry_run, repo).await?; + Ok(()) } - async fn queued(&self, run: &CiRun<'_>, repo: &impl GitHubRepoClient) -> anyhow::Result<()> { + async fn queued( + &self, + run: &CiRun<'_>, + repo: &impl GitHubRepoClient, + conn: &mut AsyncPgConnection, + pr: &Pr<'_>, + ) -> anyhow::Result<()> { let requested_by = repo.get_user(UserId(run.requested_by_id as u64)).await?; repo.send_message( @@ -670,6 +690,8 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { ) .await?; + update_labels(conn, pr, GithubCiRunStatus::Queued, run.is_dry_run, repo).await?; + Ok(()) } } @@ -810,6 +832,7 @@ mod tests { latest_commit_sha: "latest_commit_sha".into(), created_at: chrono::Utc::now(), updated_at: chrono::Utc::now(), + added_label: None, }; diesel::insert_into(crate::database::schema::github_pr::table) @@ -2191,7 +2214,7 @@ mod tests { #[tokio::test] async fn test_ci_queued() { - let (_, client, _, run, mut rx) = ci_run_test_boilerplate( + let (mut conn, client, pr, run, mut rx) = ci_run_test_boilerplate( InsertCiRun::builder(1, 1) .base_ref(Base::Commit(Cow::Borrowed("sha"))) .head_commit_sha(Cow::Borrowed("head_commit_sha")) @@ -2204,7 +2227,7 @@ mod tests { .await; let task = tokio::spawn(async move { - DefaultMergeWorkflow.queued(&run, &client).await.unwrap(); + DefaultMergeWorkflow.queued(&run, &client, &mut conn, &pr).await.unwrap(); }); match rx.recv().await.unwrap() { @@ -2257,7 +2280,7 @@ mod tests { #[tokio::test] async fn test_ci_run_cancel_not_started() { - let (mut conn, client, _, run, _) = ci_run_test_boilerplate( + let (mut conn, client, pr, run, _) = ci_run_test_boilerplate( InsertCiRun::builder(1, 1) .base_ref(Base::Commit(Cow::Borrowed("sha"))) .head_commit_sha(Cow::Borrowed("head_commit_sha")) @@ -2270,7 +2293,7 @@ mod tests { .await; let task = tokio::spawn(async move { - client.merge_workflow().cancel(&run, &client, &mut conn).await.unwrap(); + client.merge_workflow().cancel(&run, &client, &mut conn, &pr).await.unwrap(); conn }); @@ -2286,7 +2309,7 @@ mod tests { #[tokio::test] async fn test_ci_run_cancel_started() { - let (mut conn, client, _, run, mut rx) = ci_run_test_boilerplate( + let (mut conn, client, pr, run, mut rx) = ci_run_test_boilerplate( InsertCiRun::builder(1, 1) .base_ref(Base::Commit(Cow::Borrowed("sha"))) .head_commit_sha(Cow::Borrowed("head_commit_sha")) @@ -2311,7 +2334,7 @@ mod tests { let run = CiRun::latest(RepositoryId(1), 1).get_result(&mut conn).await.unwrap(); let task = tokio::spawn(async move { - client.merge_workflow().cancel(&run, &client, &mut conn).await.unwrap(); + client.merge_workflow().cancel(&run, &client, &mut conn, &pr).await.unwrap(); conn }); diff --git a/server/src/github/mod.rs b/server/src/github/mod.rs index fdb8639..a702c7a 100644 --- a/server/src/github/mod.rs +++ b/server/src/github/mod.rs @@ -9,6 +9,7 @@ use octocrab::Octocrab; pub mod config; pub mod installation; +pub mod label_state; pub mod merge_workflow; pub mod messages; pub mod models; diff --git a/server/src/github/repo.rs b/server/src/github/repo.rs index 233ed61..e98b479 100644 --- a/server/src/github/repo.rs +++ b/server/src/github/repo.rs @@ -15,7 +15,7 @@ use super::config::{GitHubBrawlRepoConfig, Permission, Role}; use super::installation::UserCache; use super::merge_workflow::{DefaultMergeWorkflow, GitHubMergeWorkflow}; use super::messages::{CommitMessage, IssueMessage}; -use super::models::{Commit, PullRequest, Repository, Review, User}; +use super::models::{Commit, Label, PullRequest, Repository, Review, User}; pub struct RepoClient { pub(super) repo: ArcSwap, @@ -127,6 +127,20 @@ pub trait GitHubRepoClient: Send + Sync { /// Delete a branch from the repository fn delete_branch(&self, branch: &str) -> impl std::future::Future> + Send; + /// Add labels to a pull request + fn add_labels( + &self, + issue_number: u64, + labels: &[String], + ) -> impl std::future::Future>> + Send; + + /// Remove a label from a pull request + fn remove_label( + &self, + issue_number: u64, + labels: &str, + ) -> impl std::future::Future>> + Send; + /// Check if a user has a permission fn has_permission( &self, @@ -353,6 +367,24 @@ impl GitHubRepoClient for RepoClient { } } + async fn add_labels(&self, issue_number: u64, labels: &[String]) -> anyhow::Result> { + self.client + .issues_by_id(self.id()) + .add_labels(issue_number, labels) + .await + .context("add labels") + .map(|labels| labels.into_iter().map(Label::from).collect()) + } + + async fn remove_label(&self, issue_number: u64, label: &str) -> anyhow::Result> { + self.client + .issues_by_id(self.id()) + .remove_label(issue_number, label) + .await + .context("remove label") + .map(|labels| labels.into_iter().map(Label::from).collect()) + } + async fn get_commit(&self, sha: &str) -> anyhow::Result> { match self .client @@ -552,6 +584,16 @@ pub mod test_utils { permissions: Vec, result: tokio::sync::oneshot::Sender>, }, + AddLabels { + issue_number: u64, + labels: Vec, + result: tokio::sync::oneshot::Sender>>, + }, + RemoveLabel { + issue_number: u64, + label: String, + result: tokio::sync::oneshot::Sender>>, + }, } impl GitHubRepoClient for MockRepoClient { @@ -724,6 +766,32 @@ pub mod test_utils { .expect("send has permission"); rx.await.expect("recv has permission") } + + async fn add_labels(&self, issue_number: u64, labels: &[String]) -> anyhow::Result> { + let (tx, rx) = tokio::sync::oneshot::channel(); + self.actions + .send(MockRepoAction::AddLabels { + issue_number, + labels: labels.to_vec(), + result: tx, + }) + .await + .expect("send add labels"); + rx.await.expect("recv add labels") + } + + async fn remove_label(&self, issue_number: u64, label: &str) -> anyhow::Result> { + let (tx, rx) = tokio::sync::oneshot::channel(); + self.actions + .send(MockRepoAction::RemoveLabel { + issue_number, + label: label.to_owned(), + result: tx, + }) + .await + .expect("send remove label"); + rx.await.expect("recv remove label") + } } } diff --git a/server/src/webhook/pull_request.rs b/server/src/webhook/pull_request.rs index 7705198..1269a86 100644 --- a/server/src/webhook/pull_request.rs +++ b/server/src/webhook/pull_request.rs @@ -45,7 +45,7 @@ pub async fn handle_with_pr( match run { Some(run) if !run.is_dry_run => { - repo.merge_workflow().cancel(&run, repo, conn).await?; + repo.merge_workflow().cancel(&run, repo, conn, ¤t).await?; repo.send_message( run.github_pr_number as u64, &messages::error_no_body(format!( @@ -99,6 +99,7 @@ mod tests { run: &CiRun<'_>, _: &impl GitHubRepoClient, conn: &mut AsyncPgConnection, + _: &Pr<'_>, ) -> anyhow::Result<()> { self.cancel.store(true, std::sync::atomic::Ordering::Relaxed); From 64150b5c1e955a170ecba3f3547397784e302cd4 Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Wed, 25 Dec 2024 02:58:19 +0000 Subject: [PATCH 2/8] allow for multiple labels to be added / removed at once --- .../2024-12-25-001112_label_state/down.sql | 2 +- .../2024-12-25-001112_label_state/up.sql | 2 +- server/src/database/pr.rs | 20 ++--- server/src/database/schema.rs | 8 +- server/src/github/config.rs | 77 +++++++++++++----- server/src/github/label_state.rs | 81 +++++++++++-------- server/src/github/merge_workflow.rs | 2 +- server/src/lib.rs | 2 + 8 files changed, 122 insertions(+), 72 deletions(-) diff --git a/migrations/2024-12-25-001112_label_state/down.sql b/migrations/2024-12-25-001112_label_state/down.sql index d50fbf9..08d97c7 100644 --- a/migrations/2024-12-25-001112_label_state/down.sql +++ b/migrations/2024-12-25-001112_label_state/down.sql @@ -1 +1 @@ -ALTER TABLE github_pr DROP COLUMN added_label; +ALTER TABLE github_pr DROP COLUMN added_labels; diff --git a/migrations/2024-12-25-001112_label_state/up.sql b/migrations/2024-12-25-001112_label_state/up.sql index eca2373..18a3545 100644 --- a/migrations/2024-12-25-001112_label_state/up.sql +++ b/migrations/2024-12-25-001112_label_state/up.sql @@ -1 +1 @@ -ALTER TABLE github_pr ADD COLUMN added_label TEXT; +ALTER TABLE github_pr ADD COLUMN added_labels TEXT[] NOT NULL DEFAULT '{}' CHECK (array_position(added_labels, NULL) IS NULL); diff --git a/server/src/database/pr.rs b/server/src/database/pr.rs index f860a89..3a1c2a6 100644 --- a/server/src/database/pr.rs +++ b/server/src/database/pr.rs @@ -30,7 +30,7 @@ pub struct Pr<'a> { pub target_branch: Cow<'a, str>, pub source_branch: Cow<'a, str>, pub latest_commit_sha: Cow<'a, str>, - pub added_label: Option>, + pub added_labels: Vec>, pub created_at: chrono::DateTime, pub updated_at: chrono::DateTime, } @@ -43,7 +43,7 @@ pub struct UpdatePr<'a> { pub github_repo_id: i64, #[builder(start_fn)] pub github_pr_number: i32, - pub added_label: Option>>, + pub added_labels: Option>>, pub title: Option>, pub body: Option>, pub merge_status: Option, @@ -112,7 +112,7 @@ impl<'a> Pr<'a> { target_branch: Cow::Borrowed(&pr.base.ref_field), source_branch: Cow::Borrowed(&pr.head.ref_field), latest_commit_sha: Cow::Borrowed(&pr.head.sha), - added_label: None, + added_labels: Vec::new(), created_at: chrono::Utc::now(), updated_at: chrono::Utc::now(), } @@ -141,7 +141,7 @@ impl<'a> Pr<'a> { title: Some(Cow::Borrowed(self.title.as_ref())), body: Some(Cow::Borrowed(self.body.as_ref())), merge_status: Some(self.merge_status), - added_label: Some(self.added_label.as_deref().map(Cow::Borrowed)), + added_labels: Some(self.added_labels.clone()), assigned_ids: Some(self.assigned_ids.clone()), status: Some(self.status), default_priority: Some(self.default_priority), @@ -268,7 +268,7 @@ mod tests { latest_commit_sha: Cow::Borrowed("test"), source_branch: Cow::Borrowed("test"), target_branch: Cow::Borrowed("test"), - added_label: None, + added_labels: vec![], merge_status: GithubPrMergeStatus::NotReady, status: GithubPrStatus::Open, merge_commit_sha: None, @@ -368,7 +368,7 @@ mod tests { updated_at: chrono::DateTime::from_timestamp_nanos(1718851200000000000), assigned_ids: vec![], author_id: 0, - added_label: None, + added_labels: vec![], default_priority: None, latest_commit_sha: Cow::Borrowed("test"), source_branch: Cow::Borrowed("test"), @@ -529,7 +529,7 @@ mod tests { title: Cow::Borrowed("test"), assigned_ids: vec![], author_id: 0, - added_label: None, + added_labels: vec![], default_priority: None, merge_status: GithubPrMergeStatus::NotReady, status: GithubPrStatus::Open, @@ -561,7 +561,7 @@ mod tests { assigned_ids: vec![], author_id: 0, default_priority: None, - added_label: None, + added_labels: vec![], merge_status: GithubPrMergeStatus::NotReady, status: GithubPrStatus::Open, merge_commit_sha: None, @@ -599,7 +599,7 @@ mod tests { title: Cow::Borrowed("test"), assigned_ids: vec![], author_id: 0, - added_label: None, + added_labels: vec![], default_priority: None, merge_status: GithubPrMergeStatus::NotReady, status: GithubPrStatus::Open, @@ -690,7 +690,7 @@ mod tests { .unwrap(); pr.update() - .body("test2".into()) + .body(Cow::Borrowed("test2")) .build() .query() .execute(&mut conn) diff --git a/server/src/database/schema.rs b/server/src/database/schema.rs index 5666cc4..d2c6e6f 100644 --- a/server/src/database/schema.rs +++ b/server/src/database/schema.rs @@ -304,12 +304,12 @@ diesel::table! { /// /// (Automatically generated by Diesel.) default_priority -> Nullable, - /// The `added_label` column of the `github_pr` table. + /// The `added_labels` column of the `github_pr` table. /// - /// Its SQL type is `Nullable`. + /// Its SQL type is `Array`. /// - /// (Automatically generated by Diesel.) - added_label -> Nullable, + /// (Manually changed from `Array>` to `Array`) + added_labels -> Array, } } diff --git a/server/src/github/config.rs b/server/src/github/config.rs index 4e78da8..2d93b0d 100644 --- a/server/src/github/config.rs +++ b/server/src/github/config.rs @@ -91,23 +91,54 @@ impl GitHubBrawlRepoConfig { #[serde(default)] pub struct GitHubBrawlLabelsConfig { /// The label to attach to PRs when they are in the merge queue - #[serde(skip_serializing_if = "Option::is_none")] - pub on_merge_queued: Option, + #[serde(skip_serializing_if = "Vec::is_empty", deserialize_with = "string_or_vec")] + pub on_merge_queued: Vec, /// The label to attach to PRs when they are being merged - #[serde(skip_serializing_if = "Option::is_none")] - pub on_merge_in_progress: Option, + #[serde(skip_serializing_if = "Vec::is_empty", deserialize_with = "string_or_vec")] + pub on_merge_in_progress: Vec, /// The label to attach to PRs when they fail to merge - #[serde(skip_serializing_if = "Option::is_none")] - pub on_merge_failure: Option, + #[serde(skip_serializing_if = "Vec::is_empty", deserialize_with = "string_or_vec")] + pub on_merge_failure: Vec, /// The label to attach to PRs when they are merged - #[serde(skip_serializing_if = "Option::is_none")] - pub on_merge_success: Option, + #[serde(skip_serializing_if = "Vec::is_empty", deserialize_with = "string_or_vec")] + pub on_merge_success: Vec, /// The label to attach to PRs when they are being tried - #[serde(skip_serializing_if = "Option::is_none")] - pub on_try_in_progress: Option, + #[serde(skip_serializing_if = "Vec::is_empty", deserialize_with = "string_or_vec")] + pub on_try_in_progress: Vec, /// The label to attach to PRs when they fail to try - #[serde(skip_serializing_if = "Option::is_none")] - pub on_try_failure: Option, + #[serde(skip_serializing_if = "Vec::is_empty", deserialize_with = "string_or_vec")] + pub on_try_failure: Vec, +} + +fn string_or_vec<'de, D: Deserializer<'de>>(s: D) -> Result, D::Error> { + use serde::de::SeqAccess; + + struct StringOrVecVisitor; + + impl<'de> serde::de::Visitor<'de> for StringOrVecVisitor { + type Value = Vec; + + fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { + formatter.write_str("a string or a vector of strings") + } + + fn visit_str(self, v: &str) -> Result { + Ok(vec![v.to_string()]) + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: SeqAccess<'de>, + { + let mut vec = Vec::new(); + while let Some(value) = seq.next_element()? { + vec.push(value); + } + Ok(vec) + } + } + + s.deserialize_any(StringOrVecVisitor) } #[derive(Debug, Clone, PartialEq, Eq)] @@ -325,10 +356,10 @@ mod tests { [labels] on_merge_queued = "merge-queued" - on_merge_in_progress = "merge-in-progress" + on_merge_in_progress = ["merge-in-progress", "waiting-on-brawl"] on_merge_failure = "merge-failed" on_merge_success = "merge-success" - on_try_in_progress = "try-in-progress" + on_try_in_progress = ["try-in-progress", "waiting-on-brawl"] on_try_failure = "try-failed" "#; @@ -342,12 +373,18 @@ mod tests { assert_eq!(config.required_status_checks, vec!["brawl-done"]); assert_eq!(config.timeout_minutes, 60); assert_eq!(config.max_reviewers, 10); - assert_eq!(config.labels.on_merge_queued, Some("merge-queued".to_string())); - assert_eq!(config.labels.on_merge_in_progress, Some("merge-in-progress".to_string())); - assert_eq!(config.labels.on_merge_failure, Some("merge-failed".to_string())); - assert_eq!(config.labels.on_merge_success, Some("merge-success".to_string())); - assert_eq!(config.labels.on_try_in_progress, Some("try-in-progress".to_string())); - assert_eq!(config.labels.on_try_failure, Some("try-failed".to_string())); + assert_eq!(config.labels.on_merge_queued, vec!["merge-queued".to_string()]); + assert_eq!( + config.labels.on_merge_in_progress, + vec!["merge-in-progress".to_string(), "waiting-on-brawl".to_string()] + ); + assert_eq!(config.labels.on_merge_failure, vec!["merge-failed".to_string()]); + assert_eq!(config.labels.on_merge_success, vec!["merge-success".to_string()]); + assert_eq!( + config.labels.on_try_in_progress, + vec!["try-in-progress".to_string(), "waiting-on-brawl".to_string()] + ); + assert_eq!(config.labels.on_try_failure, vec!["try-failed".to_string()]); } #[test] diff --git a/server/src/github/label_state.rs b/server/src/github/label_state.rs index 6f1f1ad..1301921 100644 --- a/server/src/github/label_state.rs +++ b/server/src/github/label_state.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::collections::HashSet; use diesel_async::{AsyncPgConnection, RunQueryDsl}; @@ -7,42 +8,30 @@ use super::repo::GitHubRepoClient; use crate::database::enums::GithubCiRunStatus; use crate::database::pr::Pr; -fn desired_labels<'a>(status: GithubCiRunStatus, is_dry_run: bool, config: &'a GitHubBrawlLabelsConfig) -> Option<&'a str> { +fn desired_labels(status: GithubCiRunStatus, is_dry_run: bool, config: &GitHubBrawlLabelsConfig) -> &[String] { match (status, is_dry_run) { (GithubCiRunStatus::Queued, false) => { - if let Some(on_merge_queued) = config.on_merge_queued.as_deref() { - return Some(on_merge_queued); - } + return &config.on_merge_queued; } (GithubCiRunStatus::InProgress, true) => { - if let Some(on_try_in_progress) = config.on_try_in_progress.as_deref() { - return Some(on_try_in_progress); - } + return &config.on_try_in_progress; } (GithubCiRunStatus::InProgress, false) => { - if let Some(on_merge_in_progress) = config.on_merge_in_progress.as_deref() { - return Some(on_merge_in_progress); - } + return &config.on_merge_in_progress; } (GithubCiRunStatus::Failure, true) => { - if let Some(on_try_failure) = config.on_try_failure.as_deref() { - return Some(on_try_failure); - } + return &config.on_try_failure; } (GithubCiRunStatus::Failure, false) => { - if let Some(on_merge_failure) = config.on_merge_failure.as_deref() { - return Some(on_merge_failure); - } + return &config.on_merge_failure; } (GithubCiRunStatus::Success, false) => { - if let Some(on_merge_success) = config.on_merge_success.as_deref() { - return Some(on_merge_success); - } + return &config.on_merge_success; } _ => {} } - None + &[] } pub async fn update_labels( @@ -53,17 +42,44 @@ pub async fn update_labels( repo_client: &impl GitHubRepoClient, ) -> anyhow::Result<()> { let config = repo_client.config(); - let desired_label = desired_labels(status, is_dry_run, &config.labels); + let mut desired_labels = desired_labels(status, is_dry_run, &config.labels) + .iter() + .map(|s| Cow::Borrowed(s.as_ref())) + .collect::>>(); - if desired_label == pr.added_label.as_deref() { + desired_labels.sort(); + desired_labels.dedup(); + + let desired_labels_set = desired_labels + .iter() + .map(|s| Cow::Borrowed(s.as_ref())) + .collect::>>(); + + let pr_labels_set = pr + .added_labels + .iter() + .map(|l| Cow::Borrowed(l.as_ref())) + .collect::>>(); + + let labels_to_add = desired_labels_set + .difference(&pr_labels_set) + .map(|s| Cow::Borrowed(s.as_ref())) + .collect::>(); + let labels_to_remove = pr_labels_set + .difference(&desired_labels_set) + .map(|s| Cow::Borrowed(s.as_ref())) + .collect::>(); + + if labels_to_add.is_empty() && labels_to_remove.is_empty() { return Ok(()); } - if let Some(desired_label) = desired_label { - if let Err(e) = repo_client - .add_labels(pr.github_pr_number as u64, &[desired_label.to_string()]) - .await - { + // I am not sure if we need to make an API call for each label + // The reason I do this is because each call can fail if the label does not + // exist or already has been added In which case I don't want to fail the + // entire update + for label in &labels_to_add { + if let Err(e) = repo_client.add_labels(pr.github_pr_number as u64, &[label.to_string()]).await { tracing::error!( repo_id = pr.github_repo_id, owner = repo_client.owner(), @@ -75,8 +91,8 @@ pub async fn update_labels( } } - if let Some(added_label) = pr.added_label.as_deref() { - if let Err(e) = repo_client.remove_label(pr.github_pr_number as u64, added_label).await { + for label in &labels_to_remove { + if let Err(e) = repo_client.remove_label(pr.github_pr_number as u64, label.as_ref()).await { tracing::error!( repo_id = pr.github_repo_id, owner = repo_client.owner(), @@ -88,12 +104,7 @@ pub async fn update_labels( } } - pr.update() - .added_label(desired_label.map(Cow::Borrowed)) - .build() - .query() - .execute(conn) - .await?; + pr.update().added_labels(desired_labels).build().query().execute(conn).await?; Ok(()) } diff --git a/server/src/github/merge_workflow.rs b/server/src/github/merge_workflow.rs index e2fa68d..a6bde16 100644 --- a/server/src/github/merge_workflow.rs +++ b/server/src/github/merge_workflow.rs @@ -832,7 +832,7 @@ mod tests { latest_commit_sha: "latest_commit_sha".into(), created_at: chrono::Utc::now(), updated_at: chrono::Utc::now(), - added_label: None, + added_labels: vec![], }; diesel::insert_into(crate::database::schema::github_pr::table) diff --git a/server/src/lib.rs b/server/src/lib.rs index 24f7bbc..aed0e47 100644 --- a/server/src/lib.rs +++ b/server/src/lib.rs @@ -1,3 +1,5 @@ +#![cfg_attr(coverage_nightly, feature(coverage_attribute))] + pub mod auto_start; pub mod command; pub mod database; From c54eb6cee84e2d8b7c59ba6e67b9e2994de50067 Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Wed, 25 Dec 2024 03:01:38 +0000 Subject: [PATCH 3/8] fix schema --- migrations/schema.patch | 24 ++++++++++++++++++++++-- migrations/schema.unpatched.rs | 6 +++--- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/migrations/schema.patch b/migrations/schema.patch index 2e39a9f..e0c042c 100644 --- a/migrations/schema.patch +++ b/migrations/schema.patch @@ -1,5 +1,5 @@ ---- migrations/schema.unpatched.rs 2024-12-25 01:43:39.324295195 +0000 -+++ server/src/database/schema.rs 2024-12-25 01:43:35.592373517 +0000 +--- migrations/schema.unpatched.rs 2024-12-25 03:01:31.500330121 +0000 ++++ server/src/database/schema.rs 2024-12-25 03:01:20.600359108 +0000 @@ -1,38 +1,44 @@ +#![cfg_attr(coverage_nightly, coverage(off))] // @generated automatically by Diesel CLI. @@ -85,6 +85,26 @@ /// /// (Automatically generated by Diesel.) status -> GithubPrStatus, +@@ -297,16 +303,16 @@ + /// Its SQL type is `Nullable`. + /// + /// (Automatically generated by Diesel.) + default_priority -> Nullable, + /// The `added_labels` column of the `github_pr` table. + /// +- /// Its SQL type is `Array>`. ++ /// Its SQL type is `Array`. + /// +- /// (Automatically generated by Diesel.) +- added_labels -> Array>, ++ /// (Manually changed from `Array>` to `Array`) ++ added_labels -> Array, + } + } + + diesel::table! { + /// Representation of the `health_check` table. + /// @@ -326,12 +332,13 @@ updated_at -> Timestamptz, } diff --git a/migrations/schema.unpatched.rs b/migrations/schema.unpatched.rs index 8b82e12..ae597d5 100644 --- a/migrations/schema.unpatched.rs +++ b/migrations/schema.unpatched.rs @@ -298,12 +298,12 @@ diesel::table! { /// /// (Automatically generated by Diesel.) default_priority -> Nullable, - /// The `added_label` column of the `github_pr` table. + /// The `added_labels` column of the `github_pr` table. /// - /// Its SQL type is `Nullable`. + /// Its SQL type is `Array>`. /// /// (Automatically generated by Diesel.) - added_label -> Nullable, + added_labels -> Array>, } } From b14e12e3fbb99cd2b0232e14238f5f576a1f7e21 Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Wed, 25 Dec 2024 03:05:13 +0000 Subject: [PATCH 4/8] fix tests --- server/src/bin/server.rs | 1 + server/src/database/pr.rs | 42 +++++++++++++++++++-------------------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/server/src/bin/server.rs b/server/src/bin/server.rs index a3ab831..ddde1d7 100644 --- a/server/src/bin/server.rs +++ b/server/src/bin/server.rs @@ -1,3 +1,4 @@ +#![cfg_attr(coverage_nightly, feature(coverage_attribute))] #![cfg_attr(all(coverage_nightly, test), coverage(off))] use std::net::SocketAddr; diff --git a/server/src/database/pr.rs b/server/src/database/pr.rs index 3a1c2a6..c8351ce 100644 --- a/server/src/database/pr.rs +++ b/server/src/database/pr.rs @@ -240,7 +240,7 @@ mod tests { "github_pr"."target_branch", "github_pr"."source_branch", "github_pr"."latest_commit_sha", - "github_pr"."added_label", + "github_pr"."added_labels", "github_pr"."created_at", "github_pr"."updated_at" FROM @@ -289,7 +289,7 @@ mod tests { "target_branch", "source_branch", "latest_commit_sha", - "added_label", + "added_labels", "created_at", "updated_at" ) @@ -308,10 +308,10 @@ mod tests { $9, $10, $11, - DEFAULT, $12, - $13 - ) -- binds: [1, 1, "test", "test", NotReady, 0, [], Open, "test", "test", "test", 2024-06-20T02:40:00Z, 2024-06-20T02:40:00Z] + $13, + $14 + ) -- binds: [1, 1, "test", "test", NotReady, 0, [], Open, "test", "test", "test", [], 2024-06-20T02:40:00Z, 2024-06-20T02:40:00Z] "#, } @@ -393,7 +393,7 @@ mod tests { "target_branch", "source_branch", "latest_commit_sha", - "added_label", + "added_labels", "created_at", "updated_at" ) @@ -412,23 +412,23 @@ mod tests { $9, $10, $11, - DEFAULT, $12, - $13 + $13, + $14 ) ON CONFLICT ("github_repo_id", "github_pr_number") DO UPDATE SET - "added_label" = $14, - "title" = $15, - "body" = $16, - "merge_status" = $17, - "assigned_ids" = $18, - "status" = $19, - "default_priority" = $20, - "merge_commit_sha" = $21, - "target_branch" = $22, - "latest_commit_sha" = $23, - "updated_at" = $24 + "added_labels" = $15, + "title" = $16, + "body" = $17, + "merge_status" = $18, + "assigned_ids" = $19, + "status" = $20, + "default_priority" = $21, + "merge_commit_sha" = $22, + "target_branch" = $23, + "latest_commit_sha" = $24, + "updated_at" = $25 RETURNING "github_pr"."github_repo_id", "github_pr"."github_pr_number", @@ -443,9 +443,9 @@ mod tests { "github_pr"."target_branch", "github_pr"."source_branch", "github_pr"."latest_commit_sha", - "github_pr"."added_label", + "github_pr"."added_labels", "github_pr"."created_at", - "github_pr"."updated_at" -- binds: [1, 1, "test", "test", NotReady, 0, [], Open, "test", "test", "test", 2024-06-20T02:40:00Z, 2024-06-20T02:40:00Z, None, "test", "test", NotReady, [], Open, None, None, "test", "test", 2024-06-20T02:40:00Z] + "github_pr"."updated_at" -- binds: [1, 1, "test", "test", NotReady, 0, [], Open, "test", "test", "test", [], 2024-06-20T02:40:00Z, 2024-06-20T02:40:00Z, [], "test", "test", NotReady, [], Open, None, None, "test", "test", 2024-06-20T02:40:00Z] "#, } From 49e606ace78431438b38aaf08a0c082b6f894989 Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Thu, 26 Dec 2024 02:26:15 +0000 Subject: [PATCH 5/8] add unit tests for labels --- server/src/bin/server.rs | 16 +- server/src/github/label_state.rs | 256 +++++++++++++++++++++++++--- server/src/github/merge_workflow.rs | 96 ++++++++++- 3 files changed, 331 insertions(+), 37 deletions(-) diff --git a/server/src/bin/server.rs b/server/src/bin/server.rs index ddde1d7..4f0c86e 100644 --- a/server/src/bin/server.rs +++ b/server/src/bin/server.rs @@ -163,14 +163,14 @@ impl scuffle_bootstrap_telemetry::TelemetryConfig for Global { } impl scuffle_brawl::webhook::WebhookConfig for Global { - fn bind_address(&self) -> Option { - Some(self.config.github.webhook_bind) - } - fn webhook_secret(&self) -> &str { &self.config.github.webhook_secret } + fn bind_address(&self) -> Option { + Some(self.config.github.webhook_bind) + } + fn get_repo( &self, installation_id: InstallationId, @@ -197,13 +197,13 @@ impl scuffle_brawl::webhook::WebhookConfig for Global { Ok(()) } - fn delete_installation(&self, installation_id: InstallationId) -> anyhow::Result<()> { - self.github_service.delete_installation(installation_id); + async fn update_installation(&self, installation: Installation) -> anyhow::Result<()> { + self.github_service.update_installation(installation).await?; Ok(()) } - async fn update_installation(&self, installation: Installation) -> anyhow::Result<()> { - self.github_service.update_installation(installation).await?; + fn delete_installation(&self, installation_id: InstallationId) -> anyhow::Result<()> { + self.github_service.delete_installation(installation_id); Ok(()) } diff --git a/server/src/github/label_state.rs b/server/src/github/label_state.rs index 1301921..8d5c23d 100644 --- a/server/src/github/label_state.rs +++ b/server/src/github/label_state.rs @@ -34,41 +34,61 @@ fn desired_labels(status: GithubCiRunStatus, is_dry_run: bool, config: &GitHubBr &[] } -pub async fn update_labels( - conn: &mut AsyncPgConnection, - pr: &Pr<'_>, +struct LabelsAdjustments<'a> { + desired_labels: Vec>, + labels_to_add: Vec>, + labels_to_remove: Vec>, +} + +fn get_adjustments<'a>( + pr: &'a Pr<'a>, status: GithubCiRunStatus, is_dry_run: bool, - repo_client: &impl GitHubRepoClient, -) -> anyhow::Result<()> { - let config = repo_client.config(); - let mut desired_labels = desired_labels(status, is_dry_run, &config.labels) + config: &'a GitHubBrawlLabelsConfig, +) -> LabelsAdjustments<'a> { + let mut desired_labels = desired_labels(status, is_dry_run, config) .iter() .map(|s| Cow::Borrowed(s.as_ref())) - .collect::>>(); + .collect::>>(); desired_labels.sort(); desired_labels.dedup(); - let desired_labels_set = desired_labels - .iter() - .map(|s| Cow::Borrowed(s.as_ref())) - .collect::>>(); + let desired_labels_set = desired_labels.iter().cloned().collect::>>(); let pr_labels_set = pr .added_labels .iter() .map(|l| Cow::Borrowed(l.as_ref())) - .collect::>>(); + .collect::>>(); - let labels_to_add = desired_labels_set - .difference(&pr_labels_set) - .map(|s| Cow::Borrowed(s.as_ref())) - .collect::>(); - let labels_to_remove = pr_labels_set - .difference(&desired_labels_set) - .map(|s| Cow::Borrowed(s.as_ref())) - .collect::>(); + let mut labels_to_add = desired_labels_set.difference(&pr_labels_set).cloned().collect::>(); + let mut labels_to_remove = pr_labels_set.difference(&desired_labels_set).cloned().collect::>(); + + labels_to_add.sort(); + labels_to_remove.sort(); + + LabelsAdjustments { + desired_labels, + labels_to_add, + labels_to_remove, + } +} + +pub async fn update_labels( + conn: &mut AsyncPgConnection, + pr: &Pr<'_>, + status: GithubCiRunStatus, + is_dry_run: bool, + repo_client: &impl GitHubRepoClient, +) -> anyhow::Result<()> { + let config = repo_client.config(); + + let LabelsAdjustments { + desired_labels, + labels_to_add, + labels_to_remove, + } = get_adjustments(pr, status, is_dry_run, &config.labels); if labels_to_add.is_empty() && labels_to_remove.is_empty() { return Ok(()); @@ -85,9 +105,10 @@ pub async fn update_labels( owner = repo_client.owner(), repo_name = repo_client.name(), pr_number = pr.github_pr_number, + label = %label, "failed to add label: {:#}", e - ) + ); } } @@ -98,9 +119,10 @@ pub async fn update_labels( owner = repo_client.owner(), repo_name = repo_client.name(), pr_number = pr.github_pr_number, + label = %label, "failed to remove label: {:#}", e - ) + ); } } @@ -108,3 +130,191 @@ pub async fn update_labels( Ok(()) } + +#[cfg(test)] +#[cfg_attr(coverage_nightly, coverage(off))] +mod tests { + use octocrab::models::{RepositoryId, UserId}; + + use super::*; + use crate::{database::get_test_connection, github::{merge_workflow::DefaultMergeWorkflow, models::{Label, PullRequest}, repo::test_utils::{MockRepoAction, MockRepoClient}}}; + + #[test] + fn test_desired_labels() { + let config = GitHubBrawlLabelsConfig { + on_merge_queued: vec!["queued".to_string()], + on_try_in_progress: vec!["try".to_string(), "in_progress".to_string()], + on_merge_in_progress: vec!["merge".to_string(), "in_progress".to_string()], + on_try_failure: vec!["try".to_string(), "failure".to_string()], + on_merge_failure: vec!["merge".to_string(), "failure".to_string()], + on_merge_success: vec!["merge".to_string(), "success".to_string()], + }; + + let cases: &[(GithubCiRunStatus, bool, &[&str])] = &[ + (GithubCiRunStatus::Queued, false, &["queued"]), + (GithubCiRunStatus::InProgress, false, &["merge", "in_progress"]), + (GithubCiRunStatus::InProgress, true, &["try", "in_progress"]), + (GithubCiRunStatus::Failure, false, &["merge", "failure"]), + (GithubCiRunStatus::Failure, true, &["try", "failure"]), + (GithubCiRunStatus::Success, false, &["merge", "success"]), + (GithubCiRunStatus::Cancelled, false, &[]), + (GithubCiRunStatus::Cancelled, true, &[]), + ]; + + for (status, is_dry_run, expected) in cases { + assert_eq!(desired_labels(*status, *is_dry_run, &config), *expected); + } + } + + #[test] + fn test_labels_adjustments() { + let pr = PullRequest::default(); + let mut pr = Pr::new(&pr, UserId(1), RepositoryId(1)); + + let config = GitHubBrawlLabelsConfig { + on_merge_queued: vec!["queued".to_string()], + on_try_in_progress: vec!["try".to_string(), "in_progress".to_string()], + on_merge_in_progress: vec!["merge".to_string(), "in_progress".to_string()], + on_try_failure: vec!["try".to_string(), "failure".to_string()], + on_merge_failure: vec!["merge".to_string(), "failure".to_string()], + on_merge_success: vec!["merge".to_string(), "success".to_string()], + }; + + let LabelsAdjustments { + desired_labels, + labels_to_add, + labels_to_remove, + } = get_adjustments(&pr, GithubCiRunStatus::Queued, false, &config); + assert_eq!(desired_labels, &["queued"]); + assert_eq!(labels_to_add, &["queued"]); + assert!(labels_to_remove.is_empty()); + + pr.added_labels = vec![Cow::Borrowed("queued")]; + let LabelsAdjustments { + desired_labels, + labels_to_add, + labels_to_remove, + } = get_adjustments(&pr, GithubCiRunStatus::Queued, false, &config); + assert_eq!(desired_labels, &["queued"]); + assert!(labels_to_add.is_empty()); + assert!(labels_to_remove.is_empty()); + + pr.added_labels = vec![Cow::Borrowed("queued"), Cow::Borrowed("merge")]; + let LabelsAdjustments { + desired_labels, + labels_to_add, + labels_to_remove, + } = get_adjustments(&pr, GithubCiRunStatus::Queued, false, &config); + assert_eq!(desired_labels, &["queued"]); + assert!(labels_to_add.is_empty()); + assert_eq!(labels_to_remove, &["merge"]); + + pr.added_labels = vec![Cow::Borrowed("queued"), Cow::Borrowed("merge")]; + let LabelsAdjustments { + desired_labels, + labels_to_add, + labels_to_remove, + } = get_adjustments(&pr, GithubCiRunStatus::InProgress, false, &config); + assert_eq!(desired_labels, &["in_progress", "merge"]); + assert_eq!(labels_to_add, &["in_progress"]); + assert_eq!(labels_to_remove, &["queued"]); + } + + #[tokio::test] + async fn test_update_labels() { + let mut conn = get_test_connection().await; + let pr = PullRequest::default(); + let pr = Pr::new(&pr, UserId(1), RepositoryId(1)); + + pr.insert().execute(&mut conn).await.unwrap(); + + let pr = Pr::find(RepositoryId(1), pr.github_pr_number as u64).get_result(&mut conn).await.unwrap(); + + let (mut repo_client, mut rx) = MockRepoClient::new(DefaultMergeWorkflow); + + repo_client.config.labels = GitHubBrawlLabelsConfig { + on_merge_queued: vec!["queued".to_string()], + on_try_in_progress: vec!["try".to_string(), "in_progress".to_string()], + on_merge_in_progress: vec!["merge".to_string(), "in_progress".to_string()], + on_try_failure: vec!["try".to_string(), "failure".to_string()], + on_merge_failure: vec!["merge".to_string(), "failure".to_string()], + on_merge_success: vec!["merge".to_string(), "success".to_string()], + }; + + let pr_number = pr.github_pr_number as u64; + + let task = tokio::spawn(async move { + update_labels(&mut conn, &pr, GithubCiRunStatus::Queued, false, &repo_client).await.unwrap(); + (conn, repo_client) + }); + + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { + assert_eq!(issue_number, pr_number); + assert_eq!(labels, vec!["queued".to_string()]); + result.send(Ok(vec![Label { name: "queued".to_string() }])).expect("failed to send result"); + } + _ => panic!("expected AddLabels event"), + } + + let (mut conn, repo_client) = task.await.unwrap(); + + let pr = Pr::find(RepositoryId(1), pr_number).get_result(&mut conn).await.unwrap(); + assert_eq!(pr.added_labels, vec!["queued"]); + + let task = tokio::spawn(async move { + update_labels(&mut conn, &pr, GithubCiRunStatus::InProgress, false, &repo_client).await.unwrap(); + (conn, repo_client) + }); + + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { + assert_eq!(issue_number, pr_number); + assert_eq!(labels, vec!["in_progress".to_string()]); + result.send(Ok(vec![Label { name: "in_progress".to_string() }, Label { name: "queued".to_string() }])).expect("failed to send result"); + } + _ => panic!("expected AddLabels event"), + } + + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { + assert_eq!(issue_number, pr_number); + assert_eq!(labels, vec!["merge".to_string()]); + result.send(Ok(vec![Label { name: "merge".to_string() }, Label { name: "in_progress".to_string() }, Label { name: "queued".to_string() }])).expect("failed to send result"); + } + _ => panic!("expected AddLabels event"), + } + + match rx.recv().await.unwrap() { + MockRepoAction::RemoveLabel { + issue_number, + label, + result, + } => { + assert_eq!(issue_number, pr_number); + assert_eq!(label, "queued"); + result.send(Ok(vec![Label { name: "merge".to_string() }, Label { name: "in_progress".to_string() }])).expect("failed to send result"); + } + _ => panic!("expected RemoveLabel event"), + } + + let (mut conn, _) = task.await.unwrap(); + + let pr = Pr::find(RepositoryId(1), pr_number).get_result(&mut conn).await.unwrap(); + assert_eq!(pr.added_labels, vec!["in_progress", "merge"]); + } +} + + diff --git a/server/src/github/merge_workflow.rs b/server/src/github/merge_workflow.rs index a6bde16..48300af 100644 --- a/server/src/github/merge_workflow.rs +++ b/server/src/github/merge_workflow.rs @@ -708,7 +708,7 @@ mod tests { use super::*; use crate::database::ci_run::{InsertCiRun, UpdateCiRun}; use crate::database::enums::{GithubPrMergeStatus, GithubPrStatus}; - use crate::github::config::GitHubBrawlRepoConfig; + use crate::github::config::{GitHubBrawlLabelsConfig, GitHubBrawlRepoConfig}; use crate::github::models::{CheckRunConclusion, CheckRunEvent, CheckRunStatus, Commit}; use crate::github::repo::test_utils::{MockRepoAction, MockRepoClient}; @@ -895,7 +895,7 @@ mod tests { #[tokio::test] async fn test_ci_run_refesh_merge_success() { - let (mut conn, client, pr, run, mut rx) = ci_run_test_boilerplate( + let (mut conn, mut client, pr, run, mut rx) = ci_run_test_boilerplate( InsertCiRun::builder(1, 1) .base_ref(Base::Commit(Cow::Borrowed("sha"))) .head_commit_sha(Cow::Borrowed("head_commit_sha")) @@ -907,6 +907,15 @@ mod tests { ) .await; + client.config.labels = GitHubBrawlLabelsConfig { + on_merge_queued: vec!["merge_queued".to_string()], + on_merge_in_progress: vec!["merge_in_progress".to_string()], + on_merge_success: vec!["merge_success".to_string()], + on_merge_failure: vec!["merge_failure".to_string()], + on_try_in_progress: vec!["try_in_progress".to_string()], + on_try_failure: vec!["try_failure".to_string()], + }; + UpdateCiRun::builder(run.id) .status(GithubCiRunStatus::InProgress) .started_at(chrono::Utc::now()) @@ -1017,12 +1026,24 @@ mod tests { r => panic!("unexpected action: {:?} expected delete branch", r), } + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { issue_number, labels, result } => { + assert_eq!(issue_number, 1); + assert_eq!(labels, vec!["merge_success"]); + result.send(Ok(vec![])).unwrap(); + } + r => panic!("unexpected action: {:?} expected update labels", r), + } + let mut conn = task.await.unwrap(); let run = CiRun::latest(RepositoryId(1), 1).get_result(&mut conn).await.unwrap(); assert_eq!(run.status, GithubCiRunStatus::Success); assert!(run.completed_at.is_some()); assert!(run.run_commit_sha.is_some()); + + let pr = Pr::find(RepositoryId(1), 1).get_result(&mut conn).await.unwrap(); + assert_eq!(pr.added_labels, vec!["merge_success"]); } #[tokio::test] @@ -1310,7 +1331,7 @@ mod tests { #[tokio::test] async fn test_ci_run_refesh_failure() { - let (mut conn, client, pr, run, mut rx) = ci_run_test_boilerplate( + let (mut conn, mut client, pr, run, mut rx) = ci_run_test_boilerplate( InsertCiRun::builder(1, 1) .base_ref(Base::Commit(Cow::Borrowed("sha"))) .head_commit_sha(Cow::Borrowed("head_commit_sha")) @@ -1322,6 +1343,15 @@ mod tests { ) .await; + client.config.labels = GitHubBrawlLabelsConfig { + on_merge_queued: vec!["merge_queued".to_string()], + on_merge_in_progress: vec!["merge_in_progress".to_string()], + on_merge_success: vec!["merge_success".to_string()], + on_merge_failure: vec!["merge_failure".to_string()], + on_try_in_progress: vec!["try_in_progress".to_string()], + on_try_failure: vec!["try_failure".to_string()], + }; + UpdateCiRun::builder(run.id) .status(GithubCiRunStatus::InProgress) .started_at(chrono::Utc::now()) @@ -1397,12 +1427,24 @@ mod tests { r => panic!("unexpected action: {:?} expected delete branch", r), } + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { issue_number, labels, result } => { + assert_eq!(issue_number, 1); + assert_eq!(labels, vec!["try_failure"]); + result.send(Ok(vec![])).unwrap(); + } + r => panic!("unexpected action: {:?} expected add labels", r), + } + let mut conn = task.await.unwrap(); let run = CiRun::latest(RepositoryId(1), 1).get_result(&mut conn).await.unwrap(); assert_eq!(run.status, GithubCiRunStatus::Failure); assert!(run.completed_at.is_some()); assert!(run.run_commit_sha.is_some()); + + let pr = Pr::find(RepositoryId(1), 1).get_result(&mut conn).await.unwrap(); + assert_eq!(pr.added_labels, vec!["try_failure"]); } #[tokio::test] @@ -1499,7 +1541,7 @@ mod tests { #[tokio::test] async fn test_ci_run_start_base_commit_success() { - let (mut conn, client, pr, run, mut rx) = ci_run_test_boilerplate( + let (mut conn, mut client, pr, run, mut rx) = ci_run_test_boilerplate( InsertCiRun::builder(1, 1) .base_ref(Base::Commit(Cow::Borrowed("sha"))) .head_commit_sha(Cow::Borrowed("head_commit_sha")) @@ -1511,6 +1553,15 @@ mod tests { ) .await; + client.config.labels = GitHubBrawlLabelsConfig { + on_merge_queued: vec!["merge_queued".to_string()], + on_merge_in_progress: vec!["merge_in_progress".to_string()], + on_merge_success: vec!["merge_success".to_string()], + on_merge_failure: vec!["merge_failure".to_string()], + on_try_in_progress: vec!["try_in_progress".to_string()], + on_try_failure: vec!["try_failure".to_string()], + }; + let task = tokio::spawn(async move { let status = tokio::time::timeout( Duration::from_secs(1), @@ -1609,6 +1660,15 @@ mod tests { r => panic!("unexpected action: {:?} expected send message", r), } + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { issue_number, labels, result } => { + assert_eq!(issue_number, 1); + assert_eq!(labels, vec!["try_in_progress"]); + result.send(Ok(vec![])).unwrap(); + } + r => panic!("unexpected action: {:?} expected add labels", r), + } + let mut conn = task.await.unwrap(); let run = CiRun::latest(RepositoryId(1), 1).get_result(&mut conn).await.unwrap(); @@ -1616,6 +1676,9 @@ mod tests { assert!(run.completed_at.is_none()); assert_eq!(run.run_commit_sha, Some(Cow::Borrowed("merge_commit_sha"))); assert!(run.started_at.is_some()); + + let pr = Pr::find(RepositoryId(1), 1).get_result(&mut conn).await.unwrap(); + assert_eq!(pr.added_labels, vec!["try_in_progress"]); } #[tokio::test] @@ -2214,7 +2277,7 @@ mod tests { #[tokio::test] async fn test_ci_queued() { - let (mut conn, client, pr, run, mut rx) = ci_run_test_boilerplate( + let (mut conn, mut client, pr, run, mut rx) = ci_run_test_boilerplate( InsertCiRun::builder(1, 1) .base_ref(Base::Commit(Cow::Borrowed("sha"))) .head_commit_sha(Cow::Borrowed("head_commit_sha")) @@ -2226,8 +2289,18 @@ mod tests { ) .await; + client.config.labels = GitHubBrawlLabelsConfig { + on_merge_queued: vec!["merge_queued".to_string()], + on_merge_in_progress: vec!["merge_in_progress".to_string()], + on_merge_success: vec!["merge_success".to_string()], + on_merge_failure: vec!["merge_failure".to_string()], + on_try_in_progress: vec!["try_in_progress".to_string()], + on_try_failure: vec!["try_failure".to_string()], + }; + let task = tokio::spawn(async move { DefaultMergeWorkflow.queued(&run, &client, &mut conn, &pr).await.unwrap(); + conn }); match rx.recv().await.unwrap() { @@ -2275,7 +2348,18 @@ mod tests { r => panic!("unexpected action: {:?} expected send message", r), } - task.await.unwrap(); + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { issue_number, labels, result } => { + assert_eq!(issue_number, 1); + assert_eq!(labels, vec!["merge_queued"]); + result.send(Ok(vec![])).unwrap(); + } + r => panic!("unexpected action: {:?} expected add labels", r), + } + + let mut conn = task.await.unwrap(); + let pr = Pr::find(RepositoryId(1), 1).get_result(&mut conn).await.unwrap(); + assert_eq!(pr.added_labels, vec!["merge_queued"]); } #[tokio::test] From e89a7c754987d45273dd87c0fc8bc909e9d83d66 Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Thu, 26 Dec 2024 02:27:47 +0000 Subject: [PATCH 6/8] fix formatting --- server/src/github/label_state.rs | 66 +++++++++++++++++++++++------ server/src/github/merge_workflow.rs | 24 +++++++++-- 2 files changed, 74 insertions(+), 16 deletions(-) diff --git a/server/src/github/label_state.rs b/server/src/github/label_state.rs index 8d5c23d..1e12f76 100644 --- a/server/src/github/label_state.rs +++ b/server/src/github/label_state.rs @@ -137,7 +137,10 @@ mod tests { use octocrab::models::{RepositoryId, UserId}; use super::*; - use crate::{database::get_test_connection, github::{merge_workflow::DefaultMergeWorkflow, models::{Label, PullRequest}, repo::test_utils::{MockRepoAction, MockRepoClient}}}; + use crate::database::get_test_connection; + use crate::github::merge_workflow::DefaultMergeWorkflow; + use crate::github::models::{Label, PullRequest}; + use crate::github::repo::test_utils::{MockRepoAction, MockRepoClient}; #[test] fn test_desired_labels() { @@ -225,10 +228,13 @@ mod tests { let mut conn = get_test_connection().await; let pr = PullRequest::default(); let pr = Pr::new(&pr, UserId(1), RepositoryId(1)); - + pr.insert().execute(&mut conn).await.unwrap(); - let pr = Pr::find(RepositoryId(1), pr.github_pr_number as u64).get_result(&mut conn).await.unwrap(); + let pr = Pr::find(RepositoryId(1), pr.github_pr_number as u64) + .get_result(&mut conn) + .await + .unwrap(); let (mut repo_client, mut rx) = MockRepoClient::new(DefaultMergeWorkflow); @@ -244,7 +250,9 @@ mod tests { let pr_number = pr.github_pr_number as u64; let task = tokio::spawn(async move { - update_labels(&mut conn, &pr, GithubCiRunStatus::Queued, false, &repo_client).await.unwrap(); + update_labels(&mut conn, &pr, GithubCiRunStatus::Queued, false, &repo_client) + .await + .unwrap(); (conn, repo_client) }); @@ -256,7 +264,11 @@ mod tests { } => { assert_eq!(issue_number, pr_number); assert_eq!(labels, vec!["queued".to_string()]); - result.send(Ok(vec![Label { name: "queued".to_string() }])).expect("failed to send result"); + result + .send(Ok(vec![Label { + name: "queued".to_string(), + }])) + .expect("failed to send result"); } _ => panic!("expected AddLabels event"), } @@ -267,7 +279,9 @@ mod tests { assert_eq!(pr.added_labels, vec!["queued"]); let task = tokio::spawn(async move { - update_labels(&mut conn, &pr, GithubCiRunStatus::InProgress, false, &repo_client).await.unwrap(); + update_labels(&mut conn, &pr, GithubCiRunStatus::InProgress, false, &repo_client) + .await + .unwrap(); (conn, repo_client) }); @@ -279,7 +293,16 @@ mod tests { } => { assert_eq!(issue_number, pr_number); assert_eq!(labels, vec!["in_progress".to_string()]); - result.send(Ok(vec![Label { name: "in_progress".to_string() }, Label { name: "queued".to_string() }])).expect("failed to send result"); + result + .send(Ok(vec![ + Label { + name: "in_progress".to_string(), + }, + Label { + name: "queued".to_string(), + }, + ])) + .expect("failed to send result"); } _ => panic!("expected AddLabels event"), } @@ -292,11 +315,23 @@ mod tests { } => { assert_eq!(issue_number, pr_number); assert_eq!(labels, vec!["merge".to_string()]); - result.send(Ok(vec![Label { name: "merge".to_string() }, Label { name: "in_progress".to_string() }, Label { name: "queued".to_string() }])).expect("failed to send result"); + result + .send(Ok(vec![ + Label { + name: "merge".to_string(), + }, + Label { + name: "in_progress".to_string(), + }, + Label { + name: "queued".to_string(), + }, + ])) + .expect("failed to send result"); } _ => panic!("expected AddLabels event"), } - + match rx.recv().await.unwrap() { MockRepoAction::RemoveLabel { issue_number, @@ -305,7 +340,16 @@ mod tests { } => { assert_eq!(issue_number, pr_number); assert_eq!(label, "queued"); - result.send(Ok(vec![Label { name: "merge".to_string() }, Label { name: "in_progress".to_string() }])).expect("failed to send result"); + result + .send(Ok(vec![ + Label { + name: "merge".to_string(), + }, + Label { + name: "in_progress".to_string(), + }, + ])) + .expect("failed to send result"); } _ => panic!("expected RemoveLabel event"), } @@ -316,5 +360,3 @@ mod tests { assert_eq!(pr.added_labels, vec!["in_progress", "merge"]); } } - - diff --git a/server/src/github/merge_workflow.rs b/server/src/github/merge_workflow.rs index 48300af..fc0e0cf 100644 --- a/server/src/github/merge_workflow.rs +++ b/server/src/github/merge_workflow.rs @@ -1027,7 +1027,11 @@ mod tests { } match rx.recv().await.unwrap() { - MockRepoAction::AddLabels { issue_number, labels, result } => { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { assert_eq!(issue_number, 1); assert_eq!(labels, vec!["merge_success"]); result.send(Ok(vec![])).unwrap(); @@ -1428,7 +1432,11 @@ mod tests { } match rx.recv().await.unwrap() { - MockRepoAction::AddLabels { issue_number, labels, result } => { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { assert_eq!(issue_number, 1); assert_eq!(labels, vec!["try_failure"]); result.send(Ok(vec![])).unwrap(); @@ -1661,7 +1669,11 @@ mod tests { } match rx.recv().await.unwrap() { - MockRepoAction::AddLabels { issue_number, labels, result } => { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { assert_eq!(issue_number, 1); assert_eq!(labels, vec!["try_in_progress"]); result.send(Ok(vec![])).unwrap(); @@ -2349,7 +2361,11 @@ mod tests { } match rx.recv().await.unwrap() { - MockRepoAction::AddLabels { issue_number, labels, result } => { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { assert_eq!(issue_number, 1); assert_eq!(labels, vec!["merge_queued"]); result.send(Ok(vec![])).unwrap(); From 8d046c3725e5c4802b2df10828ec0ddba3a0761a Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Thu, 26 Dec 2024 02:41:42 +0000 Subject: [PATCH 7/8] add repo client tests --- server/src/github/config.rs | 1 + server/src/github/label_state.rs | 97 ++++++++++++++++++++++++ server/src/github/mock/add_labels.json | 20 +++++ server/src/github/mock/remove_label.json | 11 +++ server/src/github/repo.rs | 84 ++++++++++++++++++++ 5 files changed, 213 insertions(+) create mode 100644 server/src/github/mock/add_labels.json create mode 100644 server/src/github/mock/remove_label.json diff --git a/server/src/github/config.rs b/server/src/github/config.rs index 2d93b0d..ae034d9 100644 --- a/server/src/github/config.rs +++ b/server/src/github/config.rs @@ -118,6 +118,7 @@ fn string_or_vec<'de, D: Deserializer<'de>>(s: D) -> Result, D::Erro impl<'de> serde::de::Visitor<'de> for StringOrVecVisitor { type Value = Vec; + #[cfg_attr(coverage_nightly, coverage(off))] fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { formatter.write_str("a string or a vector of strings") } diff --git a/server/src/github/label_state.rs b/server/src/github/label_state.rs index 1e12f76..eaf82af 100644 --- a/server/src/github/label_state.rs +++ b/server/src/github/label_state.rs @@ -359,4 +359,101 @@ mod tests { let pr = Pr::find(RepositoryId(1), pr_number).get_result(&mut conn).await.unwrap(); assert_eq!(pr.added_labels, vec!["in_progress", "merge"]); } + + #[tokio::test] + async fn test_update_labels_failure() { + let mut conn = get_test_connection().await; + let pr = PullRequest::default(); + let mut pr = Pr::new(&pr, UserId(1), RepositoryId(1)); + + pr.added_labels = vec![Cow::Borrowed("some_label"), Cow::Borrowed("other_label")]; + + pr.insert().execute(&mut conn).await.unwrap(); + + let pr = Pr::find(RepositoryId(1), pr.github_pr_number as u64) + .get_result(&mut conn) + .await + .unwrap(); + + let (mut repo_client, mut rx) = MockRepoClient::new(DefaultMergeWorkflow); + + repo_client.config.labels = GitHubBrawlLabelsConfig { + on_merge_queued: vec!["queued".to_string()], + on_try_in_progress: vec!["try".to_string(), "in_progress".to_string()], + on_merge_in_progress: vec!["merge".to_string(), "in_progress".to_string()], + on_try_failure: vec!["try".to_string(), "failure".to_string()], + on_merge_failure: vec!["merge".to_string(), "failure".to_string()], + on_merge_success: vec!["merge".to_string(), "success".to_string()], + }; + + let pr_number = pr.github_pr_number as u64; + + let task = tokio::spawn(async move { + update_labels(&mut conn, &pr, GithubCiRunStatus::InProgress, false, &repo_client) + .await + .unwrap(); + (conn, repo_client) + }); + + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { + assert_eq!(issue_number, pr_number); + assert_eq!(labels, vec!["in_progress".to_string()]); + result + .send(Err(anyhow::anyhow!("failed to add labels"))) + .expect("failed to send result"); + } + _ => panic!("expected AddLabels event"), + } + + match rx.recv().await.unwrap() { + MockRepoAction::AddLabels { + issue_number, + labels, + result, + } => { + assert_eq!(issue_number, pr_number); + assert_eq!(labels, vec!["merge".to_string()]); + result.send(Ok(Vec::new())).expect("failed to send result"); + } + _ => panic!("expected AddLabels event"), + } + + match rx.recv().await.unwrap() { + MockRepoAction::RemoveLabel { + issue_number, + label, + result, + } => { + assert_eq!(issue_number, pr_number); + assert_eq!(label, "other_label"); + result.send(Ok(Vec::new())).expect("failed to send result"); + } + _ => panic!("expected RemoveLabel event"), + } + + match rx.recv().await.unwrap() { + MockRepoAction::RemoveLabel { + issue_number, + label, + result, + } => { + assert_eq!(issue_number, pr_number); + assert_eq!(label, "some_label"); + result + .send(Err(anyhow::anyhow!("failed to remove label"))) + .expect("failed to send result"); + } + _ => panic!("expected RemoveLabel event"), + } + + let (mut conn, _) = task.await.unwrap(); + + let pr = Pr::find(RepositoryId(1), pr_number).get_result(&mut conn).await.unwrap(); + assert_eq!(pr.added_labels, vec!["in_progress", "merge"]); + } } diff --git a/server/src/github/mock/add_labels.json b/server/src/github/mock/add_labels.json new file mode 100644 index 0000000..b166434 --- /dev/null +++ b/server/src/github/mock/add_labels.json @@ -0,0 +1,20 @@ +[ + { + "id": 208045946, + "node_id": "MDU6TGFiZWwyMDgwNDU5NDY=", + "url": "https://api.github.com/repos/octocat/Hello-World/labels/bug", + "name": "bug", + "description": "Something isn't working", + "color": "f29513", + "default": true + }, + { + "id": 208045947, + "node_id": "MDU6TGFiZWwyMDgwNDU5NDc=", + "url": "https://api.github.com/repos/octocat/Hello-World/labels/enhancement", + "name": "enhancement", + "description": "New feature or request", + "color": "a2eeef", + "default": false + } +] diff --git a/server/src/github/mock/remove_label.json b/server/src/github/mock/remove_label.json new file mode 100644 index 0000000..4bab85e --- /dev/null +++ b/server/src/github/mock/remove_label.json @@ -0,0 +1,11 @@ +[ + { + "id": 208045946, + "node_id": "MDU6TGFiZWwyMDgwNDU5NDY=", + "url": "https://api.github.com/repos/octocat/Hello-World/labels/bug", + "name": "bug", + "description": "Something isn't working", + "color": "f29513", + "default": true + } +] diff --git a/server/src/github/repo.rs b/server/src/github/repo.rs index e98b479..ad3050d 100644 --- a/server/src/github/repo.rs +++ b/server/src/github/repo.rs @@ -1842,4 +1842,88 @@ mod tests { task.await.unwrap(); } + + #[tokio::test] + async fn test_repo_client_add_labels() { + let (octocrab, mut handle) = mock_octocrab(); + let repo_client = mock_repo_client( + octocrab, + default_repo(), + GitHubBrawlRepoConfig::default(), + MockMergeWorkflow(1), + ); + + let task = tokio::spawn(async move { + repo_client.add_labels(1, &["queued".to_string()]).await.unwrap(); + repo_client + }); + + let (req, resp) = handle.next_request().await.unwrap(); + insta::assert_debug_snapshot!(debug_req(req).await, @r#" + DebugReq { + method: POST, + uri: "/repositories/899726767/issues/1/labels", + headers: [ + ( + "content-type", + "application/json", + ), + ( + "authorization", + "REDACTED", + ), + ], + body: Some( + Object { + "labels": Array [ + String("queued"), + ], + }, + ), + } + "#); + + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/add_labels.json"))); + + task.await.unwrap(); + } + + #[tokio::test] + async fn test_repo_client_remove_labels() { + let (octocrab, mut handle) = mock_octocrab(); + let repo_client = mock_repo_client( + octocrab, + default_repo(), + GitHubBrawlRepoConfig::default(), + MockMergeWorkflow(1), + ); + + let task = tokio::spawn(async move { + repo_client.remove_label(1, "some_label").await.unwrap(); + repo_client + }); + + let (req, resp) = handle.next_request().await.unwrap(); + insta::assert_debug_snapshot!(debug_req(req).await, @r#" + DebugReq { + method: DELETE, + uri: "/repositories/899726767/issues/1/labels/some%5Flabel", + headers: [ + ( + "content-length", + "0", + ), + ( + "authorization", + "REDACTED", + ), + ], + body: None, + } + "#); + + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/remove_label.json"))); + + task.await.unwrap(); + } } From 726102058401a06e7d1436a908eb7a62fa925ee2 Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Thu, 26 Dec 2024 02:47:33 +0000 Subject: [PATCH 8/8] fix codecov ci --- .github/workflows/ci.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index e58be27..b750050 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -154,11 +154,13 @@ jobs: - name: Run tests run: cargo +nightly llvm-cov nextest --no-fail-fast --all-features --lcov --output-path ./lcov.info --profile ci - - name: PR Override + - name: Codecov Override if: ${{ startsWith(github.ref, 'refs/heads/automation/brawl/try/') }} run: | PR_NUMBER=$(echo ${{ github.ref }} | sed -n 's/^refs\/heads\/automation\/brawl\/try\/\([0-9]*\)$/\1/p') echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV + RUN_COMMIT_SHA=$(git log -1 --pretty=format:%H) + echo "RUN_COMMIT_SHA=$RUN_COMMIT_SHA" >> $GITHUB_ENV - uses: codecov/codecov-action@v5 with: @@ -166,6 +168,7 @@ jobs: files: ./lcov.info token: ${{ secrets.CODECOV_TOKEN }} override_pr: ${{ env.PR_NUMBER || github.event.pull_request.number || '' }} + override_commit: ${{ env.RUN_COMMIT_SHA || github.sha }} verbose: true - name: Upload test results to Codecov @@ -174,6 +177,7 @@ jobs: with: files: ./target/nextest/ci/junit.xml override_pr: ${{ env.PR_NUMBER || github.event.pull_request.number || '' }} + override_commit: ${{ env.RUN_COMMIT_SHA || github.sha }} token: ${{ secrets.CODECOV_TOKEN }} brawl-done: