From 0834d42c0aeaf0f4c9b4312a28dfd915e606c66d Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Fri, 21 Mar 2025 03:11:45 +0000 Subject: [PATCH 1/8] Add `approved_at` to approval info --- ...50321031005_add_approved_at_to_pr.down.sql | 2 ++ ...0250321031005_add_approved_at_to_pr.up.sql | 2 ++ src/bors/handlers/review.rs | 3 ++ src/database/mod.rs | 30 ++++++++++++------- src/database/operations.rs | 18 ++++++----- 5 files changed, 37 insertions(+), 18 deletions(-) create mode 100644 migrations/20250321031005_add_approved_at_to_pr.down.sql create mode 100644 migrations/20250321031005_add_approved_at_to_pr.up.sql diff --git a/migrations/20250321031005_add_approved_at_to_pr.down.sql b/migrations/20250321031005_add_approved_at_to_pr.down.sql new file mode 100644 index 0000000..2ca4c3b --- /dev/null +++ b/migrations/20250321031005_add_approved_at_to_pr.down.sql @@ -0,0 +1,2 @@ +-- Add down migration script here +ALTER TABLE pull_request DROP COLUMN approved_at; \ No newline at end of file diff --git a/migrations/20250321031005_add_approved_at_to_pr.up.sql b/migrations/20250321031005_add_approved_at_to_pr.up.sql new file mode 100644 index 0000000..0c8079d --- /dev/null +++ b/migrations/20250321031005_add_approved_at_to_pr.up.sql @@ -0,0 +1,2 @@ +-- Add up migration script here +ALTER TABLE pull_request ADD COLUMN approved_at TIMESTAMPTZ; \ No newline at end of file diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 8bd5c42..9719912 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -1,5 +1,7 @@ use std::sync::Arc; +use chrono::Utc; + use crate::PgDbClient; use crate::bors::Comment; use crate::bors::RepositoryState; @@ -38,6 +40,7 @@ pub(super) async fn command_approve( let approval_info = ApprovalInfo { approver: approver.clone(), sha: pr.head.sha.to_string(), + approved_at: Utc::now(), }; let pr_model = db .get_or_create_pull_request( diff --git a/src/database/mod.rs b/src/database/mod.rs index f4e58d0..b8c1c82 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -139,6 +139,8 @@ pub struct ApprovalInfo { pub approver: String, /// The SHA of the commit that was approved. pub sha: String, + /// When the pull request was approved. + pub approved_at: DateTime, } /// Represents the approval status of a pull request. @@ -150,23 +152,29 @@ pub enum ApprovalStatus { impl sqlx::Type for ApprovalStatus { fn type_info() -> sqlx::postgres::PgTypeInfo { - <(Option, Option) as sqlx::Type>::type_info() + <(Option, Option, Option>) as sqlx::Type>::type_info() } } impl<'r> sqlx::Decode<'r, sqlx::Postgres> for ApprovalStatus { fn decode(value: sqlx::postgres::PgValueRef<'r>) -> Result { - let (approver, sha) = - <(Option, Option) as sqlx::Decode>::decode(value)?; - - match (approver, sha) { - (Some(approver), Some(sha)) => { - Ok(ApprovalStatus::Approved(ApprovalInfo { approver, sha })) + let (approver, sha, approved_at) = + <(Option, Option, Option>) as sqlx::Decode< + sqlx::Postgres, + >>::decode(value)?; + + match (approver, sha, approved_at) { + (Some(approver), Some(sha), Some(approved_at)) => { + Ok(ApprovalStatus::Approved(ApprovalInfo { + approver, + sha, + approved_at, + })) } - (None, None) => Ok(ApprovalStatus::NotApproved), - (approver, sha) => Err(format!( - "Inconsistent approval state: approver={:?}, sha={:?}", - approver, sha + (None, None, None) => Ok(ApprovalStatus::NotApproved), + (approver, sha, approved_at) => Err(format!( + "Inconsistent approval state: approver={:?}, sha={:?}, approved_at={:?}", + approver, sha, approved_at ) .into()), } diff --git a/src/database/operations.rs b/src/database/operations.rs index 903d630..41ea751 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -37,9 +37,10 @@ pub(crate) async fn get_pull_request( pr.number as "number!: i64", ( pr.approved_by, - pr.approved_sha + pr.approved_sha, + pr.approved_at ) AS "approval_status!: ApprovalStatus", - pr.status as "pr_status: PullRequestStatus", + pr.status as "pr_status: PullRequestStatus", pr.priority, pr.rollup as "rollup: RollupMode", pr.delegated, @@ -135,9 +136,10 @@ pub(crate) async fn upsert_pull_request( pr.number as "number!: i64", ( pr.approved_by, - pr.approved_sha + pr.approved_sha, + pr.approved_at ) AS "approval_status!: ApprovalStatus", - pr.status as "pr_status: PullRequestStatus", + pr.status as "pr_status: PullRequestStatus", pr.priority, pr.rollup as "rollup: RollupMode", pr.delegated, @@ -204,6 +206,7 @@ pub(crate) async fn approve_pull_request( UPDATE pull_request SET approved_by = $1, approved_sha = $2, + approved_at = NOW(), priority = COALESCE($3, priority), rollup = COALESCE($4, rollup) WHERE id = $5 @@ -227,7 +230,7 @@ pub(crate) async fn unapprove_pull_request( ) -> anyhow::Result<()> { measure_db_query("unapprove_pull_request", || async { sqlx::query!( - "UPDATE pull_request SET approved_by = NULL, approved_sha = NULL WHERE id = $1", + "UPDATE pull_request SET approved_by = NULL, approved_sha = NULL, approved_at = NULL WHERE id = $1", pr_id ) .execute(executor) @@ -283,9 +286,10 @@ SELECT pr.number as "number!: i64", ( pr.approved_by, - pr.approved_sha + pr.approved_sha, + pr.approved_at ) AS "approval_status!: ApprovalStatus", - pr.status as "pr_status: PullRequestStatus", + pr.status as "pr_status: PullRequestStatus", pr.delegated, pr.priority, pr.base_branch, From d0a9f6bd338769d9a96a5c0babf970087bce797f Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Fri, 21 Mar 2025 03:16:38 +0000 Subject: [PATCH 2/8] Add `ApprovalStatus::ApprovalPending` variant --- ...031245_add_approval_pending_to_pr.down.sql | 2 + ...21031245_add_approval_pending_to_pr.up.sql | 2 + src/database/mod.rs | 45 +++++++++++++------ src/database/operations.rs | 12 +++-- 4 files changed, 44 insertions(+), 17 deletions(-) create mode 100644 migrations/20250321031245_add_approval_pending_to_pr.down.sql create mode 100644 migrations/20250321031245_add_approval_pending_to_pr.up.sql diff --git a/migrations/20250321031245_add_approval_pending_to_pr.down.sql b/migrations/20250321031245_add_approval_pending_to_pr.down.sql new file mode 100644 index 0000000..97a024a --- /dev/null +++ b/migrations/20250321031245_add_approval_pending_to_pr.down.sql @@ -0,0 +1,2 @@ +-- Add down migration script here +ALTER TABLE pull_request DROP COLUMN approval_pending; \ No newline at end of file diff --git a/migrations/20250321031245_add_approval_pending_to_pr.up.sql b/migrations/20250321031245_add_approval_pending_to_pr.up.sql new file mode 100644 index 0000000..8971dc2 --- /dev/null +++ b/migrations/20250321031245_add_approval_pending_to_pr.up.sql @@ -0,0 +1,2 @@ +-- Add up migration script here +ALTER TABLE pull_request ADD COLUMN approval_pending BOOLEAN DEFAULT FALSE; \ No newline at end of file diff --git a/src/database/mod.rs b/src/database/mod.rs index b8c1c82..4605728 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -147,6 +147,7 @@ pub struct ApprovalInfo { #[derive(Debug, Clone, PartialEq)] pub enum ApprovalStatus { NotApproved, + ApprovalPending(ApprovalInfo), Approved(ApprovalInfo), } @@ -158,23 +159,33 @@ impl sqlx::Type for ApprovalStatus { impl<'r> sqlx::Decode<'r, sqlx::Postgres> for ApprovalStatus { fn decode(value: sqlx::postgres::PgValueRef<'r>) -> Result { - let (approver, sha, approved_at) = - <(Option, Option, Option>) as sqlx::Decode< - sqlx::Postgres, - >>::decode(value)?; - - match (approver, sha, approved_at) { - (Some(approver), Some(sha), Some(approved_at)) => { + let (approver, sha, approved_at, is_pending) = + <( + Option, + Option, + Option>, + Option, + ) as sqlx::Decode>::decode(value)?; + + match (approver, sha, approved_at, is_pending) { + (Some(approver), Some(sha), Some(approved_at), Some(true)) => { + Ok(ApprovalStatus::ApprovalPending(ApprovalInfo { + approver, + sha, + approved_at, + })) + } + (Some(approver), Some(sha), Some(approved_at), Some(false) | None) => { Ok(ApprovalStatus::Approved(ApprovalInfo { approver, sha, approved_at, })) } - (None, None, None) => Ok(ApprovalStatus::NotApproved), - (approver, sha, approved_at) => Err(format!( - "Inconsistent approval state: approver={:?}, sha={:?}, approved_at={:?}", - approver, sha, approved_at + (None, None, None, _) => Ok(ApprovalStatus::NotApproved), + (approver, sha, approved_at, is_pending) => Err(format!( + "Inconsistent approval state: approver={:?}, sha={:?}, approved_at={:?}, is_pending={:?}", + approver, sha, approved_at, is_pending ) .into()), } @@ -258,16 +269,24 @@ impl PullRequestModel { matches!(self.approval_status, ApprovalStatus::Approved(_)) } + pub fn is_pending_approval(&self) -> bool { + matches!(self.approval_status, ApprovalStatus::ApprovalPending(_)) + } + pub fn approver(&self) -> Option<&str> { match &self.approval_status { - ApprovalStatus::Approved(info) => Some(info.approver.as_str()), + ApprovalStatus::Approved(info) | ApprovalStatus::ApprovalPending(info) => { + Some(info.approver.as_str()) + } ApprovalStatus::NotApproved => None, } } pub fn approved_sha(&self) -> Option<&str> { match &self.approval_status { - ApprovalStatus::Approved(info) => Some(info.sha.as_str()), + ApprovalStatus::Approved(info) | ApprovalStatus::ApprovalPending(info) => { + Some(info.sha.as_str()) + } ApprovalStatus::NotApproved => None, } } diff --git a/src/database/operations.rs b/src/database/operations.rs index 41ea751..a1ff99b 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -38,7 +38,8 @@ pub(crate) async fn get_pull_request( ( pr.approved_by, pr.approved_sha, - pr.approved_at + pr.approved_at, + pr.approval_pending ) AS "approval_status!: ApprovalStatus", pr.status as "pr_status: PullRequestStatus", pr.priority, @@ -137,7 +138,8 @@ pub(crate) async fn upsert_pull_request( ( pr.approved_by, pr.approved_sha, - pr.approved_at + pr.approved_at, + pr.approval_pending ) AS "approval_status!: ApprovalStatus", pr.status as "pr_status: PullRequestStatus", pr.priority, @@ -207,6 +209,7 @@ UPDATE pull_request SET approved_by = $1, approved_sha = $2, approved_at = NOW(), + approval_pending = false, priority = COALESCE($3, priority), rollup = COALESCE($4, rollup) WHERE id = $5 @@ -230,7 +233,7 @@ pub(crate) async fn unapprove_pull_request( ) -> anyhow::Result<()> { measure_db_query("unapprove_pull_request", || async { sqlx::query!( - "UPDATE pull_request SET approved_by = NULL, approved_sha = NULL, approved_at = NULL WHERE id = $1", + "UPDATE pull_request SET approved_by = NULL, approved_sha = NULL, approved_at = NULL, approval_pending = false WHERE id = $1", pr_id ) .execute(executor) @@ -287,7 +290,8 @@ SELECT ( pr.approved_by, pr.approved_sha, - pr.approved_at + pr.approved_at, + pr.approval_pending ) AS "approval_status!: ApprovalStatus", pr.status as "pr_status: PullRequestStatus", pr.delegated, From 88b7f5bc4973001808b9b8b30b5109214f97c9ed Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Fri, 21 Mar 2025 03:22:21 +0000 Subject: [PATCH 3/8] Generate .sqlx files --- ...3d49e8c3dd2b785e2d8ca3b18884b525f063a50bc7f5fc88dbb0.json} | 4 ++-- ...f241c95b21ef055c6cc2a6d525530f3282e3b452ba830828d60c.json} | 4 ++-- ...5fda11badc9ed841bcf530fa9d294c4358450c76f8d88e4c7c69.json} | 4 ++-- ...b2056a4b19a8f1ed92ace54c917ff83e08ff3e55fc66d1d582f3.json} | 4 ++-- ...dc9a73774ec7d9b10fa366b353eab35b8ee3d87450d36fb522e6.json} | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) rename .sqlx/{query-9e8a84ab79912f65d2e2db8060bd5a0f2b98c91675d42fa0c767c13f9338b4ff.json => query-36a842b58baa3d49e8c3dd2b785e2d8ca3b18884b525f063a50bc7f5fc88dbb0.json} (78%) rename .sqlx/{query-1e5d97de9e3b8c34ac2cfa4c00a6dd191dc70a3db77def03371d8bdfde2ae5e6.json => query-a835822627d3f241c95b21ef055c6cc2a6d525530f3282e3b452ba830828d60c.json} (79%) rename .sqlx/{query-b442044623f3fddd437ef8aa5750d0cb222493180c665e1d6223438a653939ef.json => query-bd583b50a0605fda11badc9ed841bcf530fa9d294c4358450c76f8d88e4c7c69.json} (80%) rename .sqlx/{query-d9ad32b76bb43454ff5621205314fc26fdcf5a17acb5e9f8ca406a67b645f026.json => query-d82cdaa923a4b2056a4b19a8f1ed92ace54c917ff83e08ff3e55fc66d1d582f3.json} (59%) rename .sqlx/{query-8b1b5771a0ae3683308438798c4885b1e593b38f6b16617bc9d52dda33115076.json => query-ecd70eca28b5dc9a73774ec7d9b10fa366b353eab35b8ee3d87450d36fb522e6.json} (53%) diff --git a/.sqlx/query-9e8a84ab79912f65d2e2db8060bd5a0f2b98c91675d42fa0c767c13f9338b4ff.json b/.sqlx/query-36a842b58baa3d49e8c3dd2b785e2d8ca3b18884b525f063a50bc7f5fc88dbb0.json similarity index 78% rename from .sqlx/query-9e8a84ab79912f65d2e2db8060bd5a0f2b98c91675d42fa0c767c13f9338b4ff.json rename to .sqlx/query-36a842b58baa3d49e8c3dd2b785e2d8ca3b18884b525f063a50bc7f5fc88dbb0.json index b59ee74..411646b 100644 --- a/.sqlx/query-9e8a84ab79912f65d2e2db8060bd5a0f2b98c91675d42fa0c767c13f9338b4ff.json +++ b/.sqlx/query-36a842b58baa3d49e8c3dd2b785e2d8ca3b18884b525f063a50bc7f5fc88dbb0.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n (\n pr.approved_by,\n pr.approved_sha\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"pr_status: PullRequestStatus\", \n pr.priority,\n pr.rollup as \"rollup: RollupMode\",\n pr.delegated,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.created_at as \"created_at: DateTime\",\n build AS \"try_build: BuildModel\"\n FROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\n WHERE pr.repository = $1 AND\n pr.number = $2\n ", + "query": "\n SELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n (\n pr.approved_by,\n pr.approved_sha,\n pr.approved_at,\n pr.approval_pending\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"pr_status: PullRequestStatus\",\n pr.priority,\n pr.rollup as \"rollup: RollupMode\",\n pr.delegated,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.created_at as \"created_at: DateTime\",\n build AS \"try_build: BuildModel\"\n FROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\n WHERE pr.repository = $1 AND\n pr.number = $2\n ", "describe": { "columns": [ { @@ -121,5 +121,5 @@ null ] }, - "hash": "9e8a84ab79912f65d2e2db8060bd5a0f2b98c91675d42fa0c767c13f9338b4ff" + "hash": "36a842b58baa3d49e8c3dd2b785e2d8ca3b18884b525f063a50bc7f5fc88dbb0" } diff --git a/.sqlx/query-1e5d97de9e3b8c34ac2cfa4c00a6dd191dc70a3db77def03371d8bdfde2ae5e6.json b/.sqlx/query-a835822627d3f241c95b21ef055c6cc2a6d525530f3282e3b452ba830828d60c.json similarity index 79% rename from .sqlx/query-1e5d97de9e3b8c34ac2cfa4c00a6dd191dc70a3db77def03371d8bdfde2ae5e6.json rename to .sqlx/query-a835822627d3f241c95b21ef055c6cc2a6d525530f3282e3b452ba830828d60c.json index 2e0772b..cc1438a 100644 --- a/.sqlx/query-1e5d97de9e3b8c34ac2cfa4c00a6dd191dc70a3db77def03371d8bdfde2ae5e6.json +++ b/.sqlx/query-a835822627d3f241c95b21ef055c6cc2a6d525530f3282e3b452ba830828d60c.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\nSELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n (\n pr.approved_by,\n pr.approved_sha\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"pr_status: PullRequestStatus\", \n pr.delegated,\n pr.priority,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.rollup as \"rollup: RollupMode\",\n pr.created_at as \"created_at: DateTime\",\n build AS \"try_build: BuildModel\"\nFROM pull_request as pr\nLEFT JOIN build ON pr.build_id = build.id\nWHERE build.id = $1\n", + "query": "\nSELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n (\n pr.approved_by,\n pr.approved_sha,\n pr.approved_at,\n pr.approval_pending\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"pr_status: PullRequestStatus\",\n pr.delegated,\n pr.priority,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.rollup as \"rollup: RollupMode\",\n pr.created_at as \"created_at: DateTime\",\n build AS \"try_build: BuildModel\"\nFROM pull_request as pr\nLEFT JOIN build ON pr.build_id = build.id\nWHERE build.id = $1\n", "describe": { "columns": [ { @@ -120,5 +120,5 @@ null ] }, - "hash": "1e5d97de9e3b8c34ac2cfa4c00a6dd191dc70a3db77def03371d8bdfde2ae5e6" + "hash": "a835822627d3f241c95b21ef055c6cc2a6d525530f3282e3b452ba830828d60c" } diff --git a/.sqlx/query-b442044623f3fddd437ef8aa5750d0cb222493180c665e1d6223438a653939ef.json b/.sqlx/query-bd583b50a0605fda11badc9ed841bcf530fa9d294c4358450c76f8d88e4c7c69.json similarity index 80% rename from .sqlx/query-b442044623f3fddd437ef8aa5750d0cb222493180c665e1d6223438a653939ef.json rename to .sqlx/query-bd583b50a0605fda11badc9ed841bcf530fa9d294c4358450c76f8d88e4c7c69.json index eec2da0..8dc6be8 100644 --- a/.sqlx/query-b442044623f3fddd437ef8aa5750d0cb222493180c665e1d6223438a653939ef.json +++ b/.sqlx/query-bd583b50a0605fda11badc9ed841bcf530fa9d294c4358450c76f8d88e4c7c69.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n WITH upserted_pr AS (\n INSERT INTO pull_request (repository, number, base_branch, mergeable_state, status)\n VALUES ($1, $2, $3, $4, $5)\n ON CONFLICT (repository, number)\n DO UPDATE SET\n base_branch = $3,\n mergeable_state = $4\n RETURNING *\n )\n SELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n (\n pr.approved_by,\n pr.approved_sha\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"pr_status: PullRequestStatus\", \n pr.priority,\n pr.rollup as \"rollup: RollupMode\",\n pr.delegated,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.created_at as \"created_at: DateTime\",\n build AS \"try_build: BuildModel\"\n FROM upserted_pr as pr\n LEFT JOIN build ON pr.build_id = build.id\n ", + "query": "\n WITH upserted_pr AS (\n INSERT INTO pull_request (repository, number, base_branch, mergeable_state, status)\n VALUES ($1, $2, $3, $4, $5)\n ON CONFLICT (repository, number)\n DO UPDATE SET\n base_branch = $3,\n mergeable_state = $4\n RETURNING *\n )\n SELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n (\n pr.approved_by,\n pr.approved_sha,\n pr.approved_at,\n pr.approval_pending\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"pr_status: PullRequestStatus\",\n pr.priority,\n pr.rollup as \"rollup: RollupMode\",\n pr.delegated,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.created_at as \"created_at: DateTime\",\n build AS \"try_build: BuildModel\"\n FROM upserted_pr as pr\n LEFT JOIN build ON pr.build_id = build.id\n ", "describe": { "columns": [ { @@ -124,5 +124,5 @@ true ] }, - "hash": "b442044623f3fddd437ef8aa5750d0cb222493180c665e1d6223438a653939ef" + "hash": "bd583b50a0605fda11badc9ed841bcf530fa9d294c4358450c76f8d88e4c7c69" } diff --git a/.sqlx/query-d9ad32b76bb43454ff5621205314fc26fdcf5a17acb5e9f8ca406a67b645f026.json b/.sqlx/query-d82cdaa923a4b2056a4b19a8f1ed92ace54c917ff83e08ff3e55fc66d1d582f3.json similarity index 59% rename from .sqlx/query-d9ad32b76bb43454ff5621205314fc26fdcf5a17acb5e9f8ca406a67b645f026.json rename to .sqlx/query-d82cdaa923a4b2056a4b19a8f1ed92ace54c917ff83e08ff3e55fc66d1d582f3.json index ad675ed..e18d376 100644 --- a/.sqlx/query-d9ad32b76bb43454ff5621205314fc26fdcf5a17acb5e9f8ca406a67b645f026.json +++ b/.sqlx/query-d82cdaa923a4b2056a4b19a8f1ed92ace54c917ff83e08ff3e55fc66d1d582f3.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "UPDATE pull_request SET approved_by = NULL, approved_sha = NULL WHERE id = $1", + "query": "UPDATE pull_request SET approved_by = NULL, approved_sha = NULL, approved_at = NULL, approval_pending = false WHERE id = $1", "describe": { "columns": [], "parameters": { @@ -10,5 +10,5 @@ }, "nullable": [] }, - "hash": "d9ad32b76bb43454ff5621205314fc26fdcf5a17acb5e9f8ca406a67b645f026" + "hash": "d82cdaa923a4b2056a4b19a8f1ed92ace54c917ff83e08ff3e55fc66d1d582f3" } diff --git a/.sqlx/query-8b1b5771a0ae3683308438798c4885b1e593b38f6b16617bc9d52dda33115076.json b/.sqlx/query-ecd70eca28b5dc9a73774ec7d9b10fa366b353eab35b8ee3d87450d36fb522e6.json similarity index 53% rename from .sqlx/query-8b1b5771a0ae3683308438798c4885b1e593b38f6b16617bc9d52dda33115076.json rename to .sqlx/query-ecd70eca28b5dc9a73774ec7d9b10fa366b353eab35b8ee3d87450d36fb522e6.json index 491dfc9..dcd5963 100644 --- a/.sqlx/query-8b1b5771a0ae3683308438798c4885b1e593b38f6b16617bc9d52dda33115076.json +++ b/.sqlx/query-ecd70eca28b5dc9a73774ec7d9b10fa366b353eab35b8ee3d87450d36fb522e6.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\nUPDATE pull_request\nSET approved_by = $1,\n approved_sha = $2,\n priority = COALESCE($3, priority),\n rollup = COALESCE($4, rollup)\nWHERE id = $5\n", + "query": "\nUPDATE pull_request\nSET approved_by = $1,\n approved_sha = $2,\n approved_at = NOW(),\n approval_pending = false,\n priority = COALESCE($3, priority),\n rollup = COALESCE($4, rollup)\nWHERE id = $5\n", "describe": { "columns": [], "parameters": { @@ -14,5 +14,5 @@ }, "nullable": [] }, - "hash": "8b1b5771a0ae3683308438798c4885b1e593b38f6b16617bc9d52dda33115076" + "hash": "ecd70eca28b5dc9a73774ec7d9b10fa366b353eab35b8ee3d87450d36fb522e6" } From 7552ba2540436cbf775df5a5c2c3f92cd37881c9 Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Sun, 23 Mar 2025 02:39:39 +0000 Subject: [PATCH 4/8] Wait for CI on approval CI pass => approve CI fail => do not approve CI pending => wait Check suite complete => approve pending PRs --- ...393a632236ef3066b595bf643a8f15e3d4792.json | 125 ++++++++++++ ...a91118f9e7a6f24de574f401e6f76c6509976.json | 14 ++ ...eae4ae508c4a6c942daccdd027a5405897ac9.json | 18 ++ src/bors/handlers/review.rs | 180 +++++++++++++++++- src/bors/handlers/trybuild.rs | 6 +- src/bors/handlers/workflow.rs | 61 ++++++ src/database/client.rs | 27 ++- src/database/operations.rs | 94 +++++++++ src/github/labels.rs | 1 + src/tests/mocks/bors.rs | 5 + src/tests/mocks/repository.rs | 7 +- 11 files changed, 530 insertions(+), 8 deletions(-) create mode 100644 .sqlx/query-6256f53e79a08ccb0c725315ce0393a632236ef3066b595bf643a8f15e3d4792.json create mode 100644 .sqlx/query-849f31e0ecab6f668aa1ff0392ba91118f9e7a6f24de574f401e6f76c6509976.json create mode 100644 .sqlx/query-a0ed3ba08b567741270c2be9e6eeae4ae508c4a6c942daccdd027a5405897ac9.json diff --git a/.sqlx/query-6256f53e79a08ccb0c725315ce0393a632236ef3066b595bf643a8f15e3d4792.json b/.sqlx/query-6256f53e79a08ccb0c725315ce0393a632236ef3066b595bf643a8f15e3d4792.json new file mode 100644 index 0000000..ee1b838 --- /dev/null +++ b/.sqlx/query-6256f53e79a08ccb0c725315ce0393a632236ef3066b595bf643a8f15e3d4792.json @@ -0,0 +1,125 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n (\n pr.approved_by,\n pr.approved_sha,\n pr.approved_at,\n pr.approval_pending\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"pr_status: PullRequestStatus\",\n pr.priority,\n pr.rollup as \"rollup: RollupMode\",\n pr.delegated,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.created_at as \"created_at: DateTime\",\n build AS \"try_build: BuildModel\"\n FROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\n WHERE pr.repository = $1 AND\n pr.approved_sha = $2 AND\n pr.approval_pending = true\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Int4" + }, + { + "ordinal": 1, + "name": "repository: GithubRepoName", + "type_info": "Text" + }, + { + "ordinal": 2, + "name": "number!: i64", + "type_info": "Int8" + }, + { + "ordinal": 3, + "name": "approval_status!: ApprovalStatus", + "type_info": "Record" + }, + { + "ordinal": 4, + "name": "pr_status: PullRequestStatus", + "type_info": "Text" + }, + { + "ordinal": 5, + "name": "priority", + "type_info": "Int4" + }, + { + "ordinal": 6, + "name": "rollup: RollupMode", + "type_info": "Text" + }, + { + "ordinal": 7, + "name": "delegated", + "type_info": "Bool" + }, + { + "ordinal": 8, + "name": "base_branch", + "type_info": "Text" + }, + { + "ordinal": 9, + "name": "mergeable_state: MergeableState", + "type_info": "Text" + }, + { + "ordinal": 10, + "name": "created_at: DateTime", + "type_info": "Timestamptz" + }, + { + "ordinal": 11, + "name": "try_build: BuildModel", + "type_info": { + "Custom": { + "name": "build", + "kind": { + "Composite": [ + [ + "id", + "Int4" + ], + [ + "repository", + "Text" + ], + [ + "branch", + "Text" + ], + [ + "commit_sha", + "Text" + ], + [ + "status", + "Text" + ], + [ + "parent", + "Text" + ], + [ + "created_at", + "Timestamptz" + ] + ] + } + } + } + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + false, + false, + null, + false, + true, + true, + false, + false, + false, + false, + null + ] + }, + "hash": "6256f53e79a08ccb0c725315ce0393a632236ef3066b595bf643a8f15e3d4792" +} diff --git a/.sqlx/query-849f31e0ecab6f668aa1ff0392ba91118f9e7a6f24de574f401e6f76c6509976.json b/.sqlx/query-849f31e0ecab6f668aa1ff0392ba91118f9e7a6f24de574f401e6f76c6509976.json new file mode 100644 index 0000000..06dcb37 --- /dev/null +++ b/.sqlx/query-849f31e0ecab6f668aa1ff0392ba91118f9e7a6f24de574f401e6f76c6509976.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET approval_pending = false WHERE id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [] + }, + "hash": "849f31e0ecab6f668aa1ff0392ba91118f9e7a6f24de574f401e6f76c6509976" +} diff --git a/.sqlx/query-a0ed3ba08b567741270c2be9e6eeae4ae508c4a6c942daccdd027a5405897ac9.json b/.sqlx/query-a0ed3ba08b567741270c2be9e6eeae4ae508c4a6c942daccdd027a5405897ac9.json new file mode 100644 index 0000000..9a571e9 --- /dev/null +++ b/.sqlx/query-a0ed3ba08b567741270c2be9e6eeae4ae508c4a6c942daccdd027a5405897ac9.json @@ -0,0 +1,18 @@ +{ + "db_name": "PostgreSQL", + "query": "\nUPDATE pull_request\nSET approved_by = $1,\n approved_sha = $2,\n approved_at = NOW(),\n approval_pending = true,\n priority = COALESCE($3, priority),\n rollup = COALESCE($4, rollup)\nWHERE id = $5\n", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Int4", + "Text", + "Int4" + ] + }, + "nullable": [] + }, + "hash": "a0ed3ba08b567741270c2be9e6eeae4ae508c4a6c942daccdd027a5405897ac9" +} diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 9719912..6617267 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use chrono::Utc; use crate::PgDbClient; +use crate::bors::CheckSuiteStatus; use crate::bors::Comment; use crate::bors::RepositoryState; use crate::bors::command::Approver; @@ -29,10 +30,12 @@ pub(super) async fn command_approve( rollup: Option, ) -> anyhow::Result<()> { tracing::info!("Approving PR {}", pr.number); + if !has_permission(&repo_state, author, pr, &db, PermissionType::Review).await? { deny_request(&repo_state, pr, author, PermissionType::Review).await?; return Ok(()); }; + let approver = match approver { Approver::Myself => author.username.clone(), Approver::Specified(approver) => approver.clone(), @@ -52,10 +55,38 @@ pub(super) async fn command_approve( ) .await?; - db.approve(&pr_model, approval_info, priority, rollup) - .await?; - handle_label_trigger(&repo_state, pr.number, LabelTrigger::Approved).await?; - notify_of_approval(&repo_state, pr, approver.as_str()).await + let should_approve_immediately = if priority.is_some() || pr_model.priority.is_some() { + true + } else { + let check_suites = repo_state + .client + .get_check_suites_for_commit(&pr.head.name, &pr.head.sha) + .await?; + + if check_suites + .iter() + .any(|check| matches!(check.status, CheckSuiteStatus::Failure)) + { + return notify_of_failed_approval(&repo_state, pr).await; + } + + check_suites.is_empty() + || check_suites + .iter() + .all(|check| matches!(check.status, CheckSuiteStatus::Success)) + }; + + if should_approve_immediately { + db.approve(&pr_model, approval_info, priority, rollup) + .await?; + handle_label_trigger(&repo_state, pr.number, LabelTrigger::Approved).await?; + notify_of_approval(&repo_state, pr, approver.as_str()).await + } else { + db.set_approval_pending(&pr_model, approval_info, priority, rollup) + .await?; + handle_label_trigger(&repo_state, pr.number, LabelTrigger::ApprovalPending).await?; + notify_of_pending_approval(&repo_state, pr, &approver).await + } } /// Unapprove a pull request. @@ -275,6 +306,34 @@ async fn notify_of_unapproval(repo: &RepositoryState, pr: &PullRequest) -> anyho .await } +async fn notify_of_pending_approval( + repo: &RepositoryState, + pr: &PullRequest, + approver: &str, +) -> anyhow::Result<()> { + repo.client + .post_comment( + pr.number, + Comment::new(format!( + ":hourglass_flowing_sand: Commit {} has received approval from `{}`, but is waiting for CI to pass", + pr.head.sha, approver + )), + ) + .await +} + +async fn notify_of_failed_approval(repo: &RepositoryState, pr: &PullRequest) -> anyhow::Result<()> { + repo.client + .post_comment( + pr.number, + Comment::new(format!( + "CI checks for commit {} have previously failed. Please push fixes or retry the build.", + pr.head.sha + )), + ) + .await +} + async fn notify_of_approval( repo: &RepositoryState, pr: &PullRequest, @@ -307,6 +366,7 @@ async fn notify_of_delegation( #[cfg(test)] mod tests { use crate::database::TreeState; + use crate::tests::mocks::TestWorkflowStatus; use crate::{ bors::{ RollupMode, @@ -1037,4 +1097,116 @@ mod tests { }) .await; } + + #[sqlx::test] + async fn approve_with_pending_ci(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.get_branch_mut("pr-1").expect_suites(1); + tester.post_comment("@bors r+").await?; + insta::assert_snapshot!( + tester.get_comment().await?, + @":hourglass_flowing_sand: Commit pr-1-sha has received approval from `default-user`, but is waiting for CI to pass" + ); + + let pr = tester.default_pr_db().await?.unwrap(); + assert!(pr.is_pending_approval()); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_with_failed_ci(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + { + let mut branch = tester.get_branch_mut("pr-1"); + branch.expect_suites(1); + branch.suite_finished(TestWorkflowStatus::Failure); + } + + tester.post_comment("@bors r+").await?; + + insta::assert_snapshot!( + tester.get_comment().await?, + @"CI checks for commit pr-1-sha have previously failed. Please push fixes or retry the build." + ); + + let pr = tester.default_pr_db().await?.unwrap(); + assert!(!pr.is_approved()); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_with_priority_ignores_ci(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.get_branch_mut("pr-1").expect_suites(1); + tester.post_comment("@bors r+ p=10").await?; + insta::assert_snapshot!( + tester.get_comment().await?, + @"Commit pr-1-sha has been approved by `default-user`" + ); + + let pr = tester.default_pr_db().await?.unwrap(); + assert!(pr.is_approved()); + assert_eq!(pr.priority, Some(10)); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_with_existing_priority_ignores_ci(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors p=5").await?; + tester.get_branch_mut("pr-1").expect_suites(1); + tester.post_comment("@bors r+").await?; + insta::assert_snapshot!( + tester.get_comment().await?, + @"Commit pr-1-sha has been approved by `default-user`" + ); + + let pr = tester.default_pr_db().await?.unwrap(); + assert!(pr.is_approved()); + assert_eq!(pr.priority, Some(5)); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn reapprove_after_ci_success(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + { + let mut branch = tester.get_branch_mut("pr-1"); + branch.expect_suites(1); + branch.suite_finished(TestWorkflowStatus::Failure); + } + + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + let pr = tester.default_pr_db().await?.unwrap(); + assert!(!pr.is_approved()); + + { + let mut branch = tester.get_branch_mut("pr-1"); + branch.expect_suites(1); + branch.suite_finished(TestWorkflowStatus::Success); + } + + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + tester.default_pr().await.expect_approved_by("default-user"); + + Ok(tester) + }) + .await; + } } diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index 7ac734a..64bca04 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -479,7 +479,9 @@ mod tests { .post_comment("@bors try parent=ea9c1b050cc8b420c2c211d2177811e564a4dc60") .await?; tester.expect_comments(1).await; - tester.workflow_success(tester.try_branch()).await?; + let try_branch = tester.try_branch(); + tester.get_branch_mut(&try_branch.get_name()).expect_suites(1); + tester.workflow_success(try_branch).await?; tester.expect_comments(1).await; tester.post_comment("@bors try parent=last").await?; insta::assert_snapshot!(tester.get_comment().await?, @":hourglass: Trying commit pr-1-sha with merge merge-ea9c1b050cc8b420c2c211d2177811e564a4dc60-pr-1-sha-1…"); @@ -589,6 +591,7 @@ mod tests { #[sqlx::test] async fn try_again_after_checks_finish(pool: sqlx::PgPool) { run_test(pool, |mut tester| async { + tester.create_branch(TRY_BRANCH_NAME).expect_suites(1); tester.post_comment("@bors try").await?; tester.expect_comments(1).await; tester @@ -772,6 +775,7 @@ try = ["+foo", "+bar", "-baz"] try_succeed = ["+foo", "+bar", "-baz"] "#)) .run_test(|mut tester| async { + tester.create_branch(TRY_BRANCH_NAME).expect_suites(1); tester.post_comment("@bors try").await?; insta::assert_snapshot!(tester.get_comment().await?, @":hourglass: Trying commit pr-1-sha with merge merge-main-sha1-pr-1-sha-0…"); let repo = tester.default_repo(); diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index 6b7d7e4..07fb1e7 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -103,6 +103,8 @@ pub(super) async fn handle_check_suite_completed( db: Arc, payload: CheckSuiteCompleted, ) -> anyhow::Result<()> { + finalize_pending_approvals(&repo, &db, &payload).await?; + if !is_bors_observed_branch(&payload.branch) { return Ok(()); } @@ -112,6 +114,7 @@ pub(super) async fn handle_check_suite_completed( payload.branch, payload.commit_sha ); + try_complete_build(repo.as_ref(), db.as_ref(), payload).await } @@ -205,6 +208,39 @@ async fn try_complete_build( Ok(()) } +async fn finalize_pending_approvals( + repo: &RepositoryState, + db: &PgDbClient, + payload: &CheckSuiteCompleted, +) -> anyhow::Result<()> { + let pending_prs = db + .find_pending_approval_prs(repo.repository(), &payload.commit_sha) + .await?; + + tracing::info!("Found {} PR(s) with pending approval", pending_prs.len()); + + if pending_prs.is_empty() { + return Ok(()); + } + + let check_suites = repo + .client + .get_check_suites_for_commit(&payload.branch, &payload.commit_sha) + .await?; + + if check_suites + .iter() + .all(|check| matches!(check.status, CheckSuiteStatus::Success)) + { + for pr in pending_prs { + db.finalize_approval(&pr).await?; + handle_label_trigger(repo, pr.number, LabelTrigger::Approved).await?; + } + } + + Ok(()) +} + #[cfg(test)] mod tests { use crate::bors::handlers::trybuild::TRY_BRANCH_NAME; @@ -390,4 +426,29 @@ mod tests { }) .await; } + + #[sqlx::test] + async fn pending_approval_finalized_on_ci_pass(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.get_branch_mut("pr-1").expect_suites(1); + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + let branch = tester.get_branch("pr-1"); + tester.workflow_success(branch.clone()).await?; + tester.check_suite(CheckSuite::completed(branch)).await?; + + tester + .wait_for(|| async { + Ok(tester + .default_pr_db() + .await? + .map_or(false, |pr| pr.is_approved())) + }) + .await?; + + Ok(tester) + }) + .await; + } } diff --git a/src/database/client.rs b/src/database/client.rs index 9521247..919c2a7 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -10,8 +10,9 @@ use crate::github::{CommitSha, GithubRepoName}; use super::operations::{ approve_pull_request, create_build, create_pull_request, create_workflow, - delegate_pull_request, find_build, find_pr_by_build, get_pull_request, get_repository, - get_running_builds, get_workflow_urls_for_build, get_workflows_for_build, set_pr_priority, + delegate_pull_request, finalize_approval, find_build, find_pending_approval_prs, + find_pr_by_build, get_pull_request, get_repository, get_running_builds, + get_workflow_urls_for_build, get_workflows_for_build, set_approval_pending, set_pr_priority, set_pr_rollup, set_pr_status, unapprove_pull_request, undelegate_pull_request, update_build_status, update_mergeable_states_by_base_branch, update_pr_build_id, update_workflow_status, upsert_pull_request, upsert_repository, @@ -119,6 +120,28 @@ impl PgDbClient { set_pr_status(&self.pool, repo, pr_number, pr_status).await } + pub async fn set_approval_pending( + &self, + pr: &PullRequestModel, + approval_info: ApprovalInfo, + priority: Option, + rollup: Option, + ) -> anyhow::Result<()> { + set_approval_pending(&self.pool, pr.id, approval_info, priority, rollup).await + } + + pub async fn find_pending_approval_prs( + &self, + repo: &GithubRepoName, + commit_sha: &CommitSha, + ) -> anyhow::Result> { + find_pending_approval_prs(&self.pool, repo, commit_sha).await + } + + pub async fn finalize_approval(&self, pr: &PullRequestModel) -> anyhow::Result<()> { + finalize_approval(&self.pool, pr.id).await + } + pub async fn find_pr_by_build( &self, build: &BuildModel, diff --git a/src/database/operations.rs b/src/database/operations.rs index a1ff99b..417c30a 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -193,6 +193,100 @@ pub(crate) async fn update_mergeable_states_by_base_branch( .await } +pub(crate) async fn set_approval_pending( + executor: impl PgExecutor<'_>, + pr_id: i32, + approval_info: ApprovalInfo, + priority: Option, + rollup: Option, +) -> anyhow::Result<()> { + let priority_i32 = priority.map(|p| p as i32); + + measure_db_query("set_approval_pending", || async { + sqlx::query!( + r#" +UPDATE pull_request +SET approved_by = $1, + approved_sha = $2, + approved_at = NOW(), + approval_pending = true, + priority = COALESCE($3, priority), + rollup = COALESCE($4, rollup) +WHERE id = $5 +"#, + approval_info.approver, + approval_info.sha, + priority_i32, + rollup as Option, + pr_id, + ) + .execute(executor) + .await?; + Ok(()) + }) + .await +} + +pub(crate) async fn find_pending_approval_prs( + executor: impl PgExecutor<'_>, + repo: &GithubRepoName, + commit_sha: &CommitSha, +) -> anyhow::Result> { + measure_db_query("find_pending_approval_prs", || async { + let records = sqlx::query_as!( + PullRequestModel, + r#" + SELECT + pr.id, + pr.repository as "repository: GithubRepoName", + pr.number as "number!: i64", + ( + pr.approved_by, + pr.approved_sha, + pr.approved_at, + pr.approval_pending + ) AS "approval_status!: ApprovalStatus", + pr.status as "pr_status: PullRequestStatus", + pr.priority, + pr.rollup as "rollup: RollupMode", + pr.delegated, + pr.base_branch, + pr.mergeable_state as "mergeable_state: MergeableState", + pr.created_at as "created_at: DateTime", + build AS "try_build: BuildModel" + FROM pull_request as pr + LEFT JOIN build ON pr.build_id = build.id + WHERE pr.repository = $1 AND + pr.approved_sha = $2 AND + pr.approval_pending = true + "#, + repo as &GithubRepoName, + commit_sha.0 + ) + .fetch_all(executor) + .await?; + + Ok(records) + }) + .await +} + +pub(crate) async fn finalize_approval( + executor: impl PgExecutor<'_>, + pr_id: i32, +) -> anyhow::Result<()> { + measure_db_query("finalize_approval", || async { + sqlx::query!( + "UPDATE pull_request SET approval_pending = false WHERE id = $1", + pr_id + ) + .execute(executor) + .await?; + Ok(()) + }) + .await +} + pub(crate) async fn approve_pull_request( executor: impl PgExecutor<'_>, pr_id: i32, diff --git a/src/github/labels.rs b/src/github/labels.rs index 337bb33..ca0f471 100644 --- a/src/github/labels.rs +++ b/src/github/labels.rs @@ -2,6 +2,7 @@ #[derive(Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] pub enum LabelTrigger { Approved, + ApprovalPending, Unapproved, TryBuildStarted, TryBuildSucceeded, diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs index 4ec2d74..f5f9717 100644 --- a/src/tests/mocks/bors.rs +++ b/src/tests/mocks/bors.rs @@ -459,6 +459,11 @@ impl BorsTester { let mut repo = repo.lock(); let counter = repo.get_next_pr_push_counter(); + let new_sha = format!("pr-{pr_number}-commit-{counter}"); + + if let Some(branch) = repo.get_branch_by_name(&format!("pr-{pr_number}")) { + branch.set_to_sha(&new_sha); + } let pr = repo .pull_requests diff --git a/src/tests/mocks/repository.rs b/src/tests/mocks/repository.rs index 07b6120..f65a146 100644 --- a/src/tests/mocks/repository.rs +++ b/src/tests/mocks/repository.rs @@ -35,6 +35,7 @@ pub struct PullRequest { pub head_sha: String, pub author: User, pub base_branch: Branch, + pub head_branch: Branch, pub mergeable_state: MergeableState, pub status: PullRequestStatus, pub merged_at: Option>, @@ -42,6 +43,8 @@ pub struct PullRequest { impl PullRequest { pub fn new(repo: GithubRepoName, number: u64, author: User, is_draft: bool) -> Self { + let head_branch = Branch::new(&format!("pr-{number}"), &format!("pr-{number}-sha")); + Self { number: PullRequestNumber(number), repo, @@ -49,6 +52,7 @@ impl PullRequest { removed_labels: Vec::new(), comment_counter: 0, head_sha: format!("pr-{number}-sha"), + head_branch, author, base_branch: Branch::default(), mergeable_state: MergeableState::Clean, @@ -139,6 +143,7 @@ impl Repo { } pub fn with_pr(mut self, pull_request: PullRequest) -> Self { + self.branches.push(pull_request.head_branch.clone()); self.pull_requests .insert(pull_request.number.0, pull_request); self @@ -224,7 +229,7 @@ impl Branch { sha: sha.to_string(), commit_message: format!("Commit {sha}"), sha_history: vec![], - suite_statuses: vec![CheckSuiteStatus::Pending], + suite_statuses: vec![CheckSuiteStatus::Success], merge_counter: 0, merge_conflict: false, } From 38666ee10508e85e25796fc38fde05483eff1427 Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Sun, 23 Mar 2025 15:54:30 +0000 Subject: [PATCH 5/8] Add pending approvals refresh check --- ...c886226c4fd3b721fd70008da5ef96119ff30.json | 14 ++ ...f8c37745f7a7110fff3c5460fd1630f599064.json | 124 ++++++++++++++ rust-bors.example.toml | 3 +- src/bors/handlers/refresh.rs | 151 +++++++++++++++++- src/bors/handlers/review.rs | 5 +- src/bors/handlers/workflow.rs | 2 +- src/config.rs | 2 + src/database/client.rs | 28 +++- src/database/mod.rs | 9 ++ src/database/operations.rs | 59 ++++++- src/tests/mocks/bors.rs | 7 + src/tests/mocks/repository.rs | 1 + 12 files changed, 390 insertions(+), 15 deletions(-) create mode 100644 .sqlx/query-8fb46f16253d048e2e2ee92ae60c886226c4fd3b721fd70008da5ef96119ff30.json create mode 100644 .sqlx/query-e20dfabc05c7e2557ffc512d6c6f8c37745f7a7110fff3c5460fd1630f599064.json diff --git a/.sqlx/query-8fb46f16253d048e2e2ee92ae60c886226c4fd3b721fd70008da5ef96119ff30.json b/.sqlx/query-8fb46f16253d048e2e2ee92ae60c886226c4fd3b721fd70008da5ef96119ff30.json new file mode 100644 index 0000000..dd339d5 --- /dev/null +++ b/.sqlx/query-8fb46f16253d048e2e2ee92ae60c886226c4fd3b721fd70008da5ef96119ff30.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET approved_by = NULL, approved_sha = NULL, approved_at = NULL, approval_pending = false WHERE id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [] + }, + "hash": "8fb46f16253d048e2e2ee92ae60c886226c4fd3b721fd70008da5ef96119ff30" +} diff --git a/.sqlx/query-e20dfabc05c7e2557ffc512d6c6f8c37745f7a7110fff3c5460fd1630f599064.json b/.sqlx/query-e20dfabc05c7e2557ffc512d6c6f8c37745f7a7110fff3c5460fd1630f599064.json new file mode 100644 index 0000000..2c94c43 --- /dev/null +++ b/.sqlx/query-e20dfabc05c7e2557ffc512d6c6f8c37745f7a7110fff3c5460fd1630f599064.json @@ -0,0 +1,124 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n (\n pr.approved_by,\n pr.approved_sha,\n pr.approved_at,\n pr.approval_pending\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"pr_status: PullRequestStatus\",\n pr.priority,\n pr.rollup as \"rollup: RollupMode\",\n pr.delegated,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.created_at as \"created_at: DateTime\",\n build AS \"try_build: BuildModel\"\n FROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\n WHERE pr.repository = $1 AND\n pr.approval_pending = true\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Int4" + }, + { + "ordinal": 1, + "name": "repository: GithubRepoName", + "type_info": "Text" + }, + { + "ordinal": 2, + "name": "number!: i64", + "type_info": "Int8" + }, + { + "ordinal": 3, + "name": "approval_status!: ApprovalStatus", + "type_info": "Record" + }, + { + "ordinal": 4, + "name": "pr_status: PullRequestStatus", + "type_info": "Text" + }, + { + "ordinal": 5, + "name": "priority", + "type_info": "Int4" + }, + { + "ordinal": 6, + "name": "rollup: RollupMode", + "type_info": "Text" + }, + { + "ordinal": 7, + "name": "delegated", + "type_info": "Bool" + }, + { + "ordinal": 8, + "name": "base_branch", + "type_info": "Text" + }, + { + "ordinal": 9, + "name": "mergeable_state: MergeableState", + "type_info": "Text" + }, + { + "ordinal": 10, + "name": "created_at: DateTime", + "type_info": "Timestamptz" + }, + { + "ordinal": 11, + "name": "try_build: BuildModel", + "type_info": { + "Custom": { + "name": "build", + "kind": { + "Composite": [ + [ + "id", + "Int4" + ], + [ + "repository", + "Text" + ], + [ + "branch", + "Text" + ], + [ + "commit_sha", + "Text" + ], + [ + "status", + "Text" + ], + [ + "parent", + "Text" + ], + [ + "created_at", + "Timestamptz" + ] + ] + } + } + } + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false, + false, + false, + null, + false, + true, + true, + false, + false, + false, + false, + null + ] + }, + "hash": "e20dfabc05c7e2557ffc512d6c6f8c37745f7a7110fff3c5460fd1630f599064" +} diff --git a/rust-bors.example.toml b/rust-bors.example.toml index 61a1425..6b4d7c5 100644 --- a/rust-bors.example.toml +++ b/rust-bors.example.toml @@ -10,7 +10,8 @@ timeout = 3600 # - try_failed: Try build has failed # (Optional) [labels] -approve = ["+approved"] +approve = ["+approved", "-approved_awaiting_ci"] try = ["+foo", "-bar"] try_succeed = ["+foobar", "+foo", "+baz"] try_failed = [] +approval_pending = ["+approved_awaiting_ci"] diff --git a/src/bors/handlers/refresh.rs b/src/bors/handlers/refresh.rs index 6dcb1ea..c53035a 100644 --- a/src/bors/handlers/refresh.rs +++ b/src/bors/handlers/refresh.rs @@ -4,10 +4,14 @@ use std::time::Duration; use anyhow::Context; use chrono::{DateTime, Utc}; +use crate::bors::CheckSuiteStatus; use crate::bors::Comment; use crate::bors::RepositoryState; +use crate::bors::handlers::labels::handle_label_trigger; +use crate::bors::handlers::review::notify_of_failed_approval; use crate::bors::handlers::trybuild::cancel_build_workflows; use crate::database::BuildStatus; +use crate::github::LabelTrigger; use crate::{PgDbClient, TeamApiClient}; pub async fn refresh_repository( @@ -16,10 +20,11 @@ pub async fn refresh_repository( team_api_client: &TeamApiClient, ) -> anyhow::Result<()> { let repo = repo.as_ref(); - if let (Ok(_), _, Ok(_)) = tokio::join!( + if let (Ok(_), _, Ok(_), Ok(_)) = tokio::join!( cancel_timed_out_builds(repo, db.as_ref()), reload_permission(repo, team_api_client), - reload_config(repo) + reload_config(repo), + check_pending_approvals(repo, db.as_ref()) ) { Ok(()) } else { @@ -85,6 +90,69 @@ async fn reload_config(repo: &RepositoryState) -> anyhow::Result<()> { Ok(()) } +async fn check_pending_approvals(repo: &RepositoryState, db: &PgDbClient) -> anyhow::Result<()> { + let pending_prs = db.find_prs_pending_approval(repo.repository()).await?; + + tracing::info!("Found {} PR(s) with pending approval", pending_prs.len()); + + if pending_prs.is_empty() { + return Ok(()); + } + + for pr_model in pending_prs { + let timeout = repo.config.load().timeout; + let approved_at = pr_model.approved_at(); + + if approved_at.is_none() { + tracing::error!("PR with pending approval has no `approved_at` timestamp"); + continue; + } + + let pr = repo.client.get_pull_request(pr_model.number).await?; + + let check_suites = repo + .client + .get_check_suites_for_commit(&pr.head.name, &pr.head.sha) + .await?; + + if check_suites + .iter() + .any(|check| matches!(check.status, CheckSuiteStatus::Failure)) + { + db.remove_pending_approval(&pr_model).await?; + handle_label_trigger(repo, pr.number, LabelTrigger::ApprovalPending).await?; + notify_of_failed_approval(repo, &pr).await?; + continue; + } + + if check_suites + .iter() + .all(|check| matches!(check.status, CheckSuiteStatus::Success)) + { + db.finalize_approval(&pr_model).await?; + handle_label_trigger(repo, pr.number, LabelTrigger::Approved).await?; + continue; + } + + if elapsed_time(approved_at.unwrap()) >= timeout { + tracing::info!("Cancelling PR approval: {}", pr.number); + + db.remove_pending_approval(&pr_model).await?; + repo.client + .post_comment( + pr.number, + Comment::new(format!( + ":boom: CI checks for commit {} timed out. Removing pending approval.", + pr.head.sha + )), + ) + .await?; + } + } + + Ok(()) +} + #[cfg(not(test))] fn now() -> DateTime { Utc::now() @@ -110,7 +178,7 @@ mod tests { use crate::bors::handlers::WAIT_FOR_WORKFLOW_STARTED; use crate::bors::handlers::refresh::MOCK_TIME; use crate::tests::mocks::{ - BorsBuilder, GitHubState, WorkflowEvent, default_repo_name, run_test, + BorsBuilder, CheckSuite, GitHubState, WorkflowEvent, default_repo_name, run_test, }; use chrono::Utc; use std::future::Future; @@ -207,6 +275,83 @@ timeout = 3600 gh.check_cancelled_workflows(default_repo_name(), &[1]); } + #[sqlx::test] + async fn approve_finalized_on_success(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.get_branch_mut("pr-1").expect_suites(1); + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + tester.default_pr().await.expect_approval_pending(); + + let branch = tester.get_branch("pr-1"); + tester.workflow_success(branch.clone()).await?; + tester.check_suite(CheckSuite::completed(branch)).await?; + + tester + .wait_for(|| async { + Ok(tester + .default_pr_db() + .await? + .map_or(false, |pr| pr.is_approved())) + }) + .await?; + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn pending_approval_times_out(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async move { + tester.get_branch_mut("pr-1").expect_suites(1); + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + tester.default_pr().await.expect_approval_pending(); + + with_mocked_time(Duration::from_secs(4000), async { + tester.refresh().await; + }) + .await; + + insta::assert_snapshot!( + tester.get_comment().await?, + @r###":boom: CI checks for commit pr-1-sha timed out. Removing pending approval."### + ); + + let pr = tester.default_pr_db().await?.unwrap(); + assert!(!pr.is_approved()); + assert!(!pr.is_pending_approval()); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn pending_approval_not_timed_out_before_timeout(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async move { + tester.get_branch_mut("pr-1").expect_suites(1); + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + let pr = tester.default_pr_db().await?.unwrap(); + assert!(pr.is_pending_approval()); + + with_mocked_time(Duration::from_secs(1800), async { + tester.refresh().await; + }) + .await; + + tester.default_pr().await.expect_approval_pending(); + + Ok(tester) + }) + .await; + } + async fn with_mocked_time>(in_future: Duration, future: Fut) { // It is important to use this function only with a single threaded runtime, // otherwise the `MOCK_TIME` variable might get mixed up between different threads. diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 6617267..2b00b17 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -322,7 +322,10 @@ async fn notify_of_pending_approval( .await } -async fn notify_of_failed_approval(repo: &RepositoryState, pr: &PullRequest) -> anyhow::Result<()> { +pub async fn notify_of_failed_approval( + repo: &RepositoryState, + pr: &PullRequest, +) -> anyhow::Result<()> { repo.client .post_comment( pr.number, diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index 07fb1e7..c18dde8 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -214,7 +214,7 @@ async fn finalize_pending_approvals( payload: &CheckSuiteCompleted, ) -> anyhow::Result<()> { let pending_prs = db - .find_pending_approval_prs(repo.repository(), &payload.commit_sha) + .find_prs_pending_approval_with_sha(repo.repository(), &payload.commit_sha) .await?; tracing::info!("Found {} PR(s) with pending approval", pending_prs.len()); diff --git a/src/config.rs b/src/config.rs index ad31448..e05a344 100644 --- a/src/config.rs +++ b/src/config.rs @@ -55,6 +55,7 @@ where enum Trigger { Approve, Unapprove, + ApprovalPending, Try, TrySucceed, TryFailed, @@ -65,6 +66,7 @@ where match value { Trigger::Approve => LabelTrigger::Approved, Trigger::Unapprove => LabelTrigger::Unapproved, + Trigger::ApprovalPending => LabelTrigger::ApprovalPending, Trigger::Try => LabelTrigger::TryBuildStarted, Trigger::TrySucceed => LabelTrigger::TryBuildSucceeded, Trigger::TryFailed => LabelTrigger::TryBuildFailed, diff --git a/src/database/client.rs b/src/database/client.rs index 919c2a7..e606bf5 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -10,12 +10,13 @@ use crate::github::{CommitSha, GithubRepoName}; use super::operations::{ approve_pull_request, create_build, create_pull_request, create_workflow, - delegate_pull_request, finalize_approval, find_build, find_pending_approval_prs, - find_pr_by_build, get_pull_request, get_repository, get_running_builds, - get_workflow_urls_for_build, get_workflows_for_build, set_approval_pending, set_pr_priority, - set_pr_rollup, set_pr_status, unapprove_pull_request, undelegate_pull_request, - update_build_status, update_mergeable_states_by_base_branch, update_pr_build_id, - update_workflow_status, upsert_pull_request, upsert_repository, + delegate_pull_request, finalize_approval, find_build, find_pr_by_build, + find_prs_pending_approval, find_prs_pending_approval_with_sha, get_pull_request, + get_repository, get_running_builds, get_workflow_urls_for_build, get_workflows_for_build, + remove_pending_approval, set_approval_pending, set_pr_priority, set_pr_rollup, set_pr_status, + unapprove_pull_request, undelegate_pull_request, update_build_status, + update_mergeable_states_by_base_branch, update_pr_build_id, update_workflow_status, + upsert_pull_request, upsert_repository, }; use super::{ApprovalInfo, MergeableState, RunId}; @@ -130,12 +131,23 @@ impl PgDbClient { set_approval_pending(&self.pool, pr.id, approval_info, priority, rollup).await } - pub async fn find_pending_approval_prs( + pub async fn remove_pending_approval(&self, pr: &PullRequestModel) -> anyhow::Result<()> { + remove_pending_approval(&self.pool, pr.id).await + } + + pub async fn find_prs_pending_approval_with_sha( &self, repo: &GithubRepoName, commit_sha: &CommitSha, ) -> anyhow::Result> { - find_pending_approval_prs(&self.pool, repo, commit_sha).await + find_prs_pending_approval_with_sha(&self.pool, repo, commit_sha).await + } + + pub async fn find_prs_pending_approval( + &self, + repo: &GithubRepoName, + ) -> anyhow::Result> { + find_prs_pending_approval(&self.pool, repo).await } pub async fn finalize_approval(&self, pr: &PullRequestModel) -> anyhow::Result<()> { diff --git a/src/database/mod.rs b/src/database/mod.rs index 4605728..544ec83 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -290,6 +290,15 @@ impl PullRequestModel { ApprovalStatus::NotApproved => None, } } + + pub fn approved_at(&self) -> Option> { + match &self.approval_status { + ApprovalStatus::Approved(info) | ApprovalStatus::ApprovalPending(info) => { + Some(info.approved_at) + } + ApprovalStatus::NotApproved => None, + } + } } /// Describes whether a workflow is a Github Actions workflow or if it's a job from some external diff --git a/src/database/operations.rs b/src/database/operations.rs index 417c30a..4ff0f8a 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -227,7 +227,7 @@ WHERE id = $5 .await } -pub(crate) async fn find_pending_approval_prs( +pub(crate) async fn find_prs_pending_approval_with_sha( executor: impl PgExecutor<'_>, repo: &GithubRepoName, commit_sha: &CommitSha, @@ -271,6 +271,63 @@ pub(crate) async fn find_pending_approval_prs( .await } +pub(crate) async fn find_prs_pending_approval( + executor: impl PgExecutor<'_>, + repo: &GithubRepoName, +) -> anyhow::Result> { + measure_db_query("get_pending_approval_prs", || async { + let records = sqlx::query_as!( + PullRequestModel, + r#" + SELECT + pr.id, + pr.repository as "repository: GithubRepoName", + pr.number as "number!: i64", + ( + pr.approved_by, + pr.approved_sha, + pr.approved_at, + pr.approval_pending + ) AS "approval_status!: ApprovalStatus", + pr.status as "pr_status: PullRequestStatus", + pr.priority, + pr.rollup as "rollup: RollupMode", + pr.delegated, + pr.base_branch, + pr.mergeable_state as "mergeable_state: MergeableState", + pr.created_at as "created_at: DateTime", + build AS "try_build: BuildModel" + FROM pull_request as pr + LEFT JOIN build ON pr.build_id = build.id + WHERE pr.repository = $1 AND + pr.approval_pending = true + "#, + repo as &GithubRepoName, + ) + .fetch_all(executor) + .await?; + + Ok(records) + }) + .await +} + +pub(crate) async fn remove_pending_approval( + executor: impl PgExecutor<'_>, + pr_id: i32, +) -> anyhow::Result<()> { + measure_db_query("remove_pending_approval", || async { + sqlx::query!( + "UPDATE pull_request SET approved_by = NULL, approved_sha = NULL, approved_at = NULL, approval_pending = false WHERE id = $1", + pr_id + ) + .execute(executor) + .await?; + Ok(()) + }) + .await +} + pub(crate) async fn finalize_approval( executor: impl PgExecutor<'_>, pr_id: i32, diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs index f5f9717..c6650ae 100644 --- a/src/tests/mocks/bors.rs +++ b/src/tests/mocks/bors.rs @@ -627,6 +627,13 @@ impl PullRequestProxy { self } + #[track_caller] + pub fn expect_approval_pending(&self) -> &Self { + assert!(self.require_db_pr().is_pending_approval()); + self.gh_pr.check_added_labels(&["approval_pending"]); + self + } + #[track_caller] pub fn expect_approved_by(&self, approved_by: &str) -> &Self { assert_eq!(self.require_db_pr().approver(), Some(approved_by)); diff --git a/src/tests/mocks/repository.rs b/src/tests/mocks/repository.rs index f65a146..2bb5ba9 100644 --- a/src/tests/mocks/repository.rs +++ b/src/tests/mocks/repository.rs @@ -188,6 +188,7 @@ timeout = 3600 # Set labels on PR approvals [labels] approve = ["+approved"] +approval_pending = ["+approval_pending"] "# .to_string(); From 38c104fff826d54b909eecc988285bfebd86d6ff Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Sun, 23 Mar 2025 16:32:52 +0000 Subject: [PATCH 6/8] Refactor to use helper methods: `default_pr_head_branch` and `default_pr_head_branch_mut` --- src/bors/handlers/refresh.rs | 8 ++++---- src/bors/handlers/review.rs | 12 ++++++------ src/bors/handlers/workflow.rs | 4 ++-- src/tests/mocks/bors.rs | 8 ++++++++ 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/bors/handlers/refresh.rs b/src/bors/handlers/refresh.rs index c53035a..45d07f5 100644 --- a/src/bors/handlers/refresh.rs +++ b/src/bors/handlers/refresh.rs @@ -278,13 +278,13 @@ timeout = 3600 #[sqlx::test] async fn approve_finalized_on_success(pool: sqlx::PgPool) { run_test(pool, |mut tester| async { - tester.get_branch_mut("pr-1").expect_suites(1); + tester.default_pr_head_branch_mut().expect_suites(1); tester.post_comment("@bors r+").await?; tester.expect_comments(1).await; tester.default_pr().await.expect_approval_pending(); - let branch = tester.get_branch("pr-1"); + let branch = tester.default_pr_head_branch(); tester.workflow_success(branch.clone()).await?; tester.check_suite(CheckSuite::completed(branch)).await?; @@ -305,7 +305,7 @@ timeout = 3600 #[sqlx::test] async fn pending_approval_times_out(pool: sqlx::PgPool) { run_test(pool, |mut tester| async move { - tester.get_branch_mut("pr-1").expect_suites(1); + tester.default_pr_head_branch_mut().expect_suites(1); tester.post_comment("@bors r+").await?; tester.expect_comments(1).await; @@ -333,7 +333,7 @@ timeout = 3600 #[sqlx::test] async fn pending_approval_not_timed_out_before_timeout(pool: sqlx::PgPool) { run_test(pool, |mut tester| async move { - tester.get_branch_mut("pr-1").expect_suites(1); + tester.default_pr_head_branch_mut().expect_suites(1); tester.post_comment("@bors r+").await?; tester.expect_comments(1).await; diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 2b00b17..f6a89f9 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -1104,7 +1104,7 @@ mod tests { #[sqlx::test] async fn approve_with_pending_ci(pool: sqlx::PgPool) { run_test(pool, |mut tester| async { - tester.get_branch_mut("pr-1").expect_suites(1); + tester.default_pr_head_branch_mut().expect_suites(1); tester.post_comment("@bors r+").await?; insta::assert_snapshot!( tester.get_comment().await?, @@ -1123,7 +1123,7 @@ mod tests { async fn approve_with_failed_ci(pool: sqlx::PgPool) { run_test(pool, |mut tester| async { { - let mut branch = tester.get_branch_mut("pr-1"); + let mut branch = tester.default_pr_head_branch_mut(); branch.expect_suites(1); branch.suite_finished(TestWorkflowStatus::Failure); } @@ -1146,7 +1146,7 @@ mod tests { #[sqlx::test] async fn approve_with_priority_ignores_ci(pool: sqlx::PgPool) { run_test(pool, |mut tester| async { - tester.get_branch_mut("pr-1").expect_suites(1); + tester.default_pr_head_branch().expect_suites(1); tester.post_comment("@bors r+ p=10").await?; insta::assert_snapshot!( tester.get_comment().await?, @@ -1166,7 +1166,7 @@ mod tests { async fn approve_with_existing_priority_ignores_ci(pool: sqlx::PgPool) { run_test(pool, |mut tester| async { tester.post_comment("@bors p=5").await?; - tester.get_branch_mut("pr-1").expect_suites(1); + tester.default_pr_head_branch().expect_suites(1); tester.post_comment("@bors r+").await?; insta::assert_snapshot!( tester.get_comment().await?, @@ -1186,7 +1186,7 @@ mod tests { async fn reapprove_after_ci_success(pool: sqlx::PgPool) { run_test(pool, |mut tester| async { { - let mut branch = tester.get_branch_mut("pr-1"); + let mut branch = tester.default_pr_head_branch_mut(); branch.expect_suites(1); branch.suite_finished(TestWorkflowStatus::Failure); } @@ -1198,7 +1198,7 @@ mod tests { assert!(!pr.is_approved()); { - let mut branch = tester.get_branch_mut("pr-1"); + let mut branch = tester.default_pr_head_branch_mut(); branch.expect_suites(1); branch.suite_finished(TestWorkflowStatus::Success); } diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index c18dde8..737f3e9 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -430,11 +430,11 @@ mod tests { #[sqlx::test] async fn pending_approval_finalized_on_ci_pass(pool: sqlx::PgPool) { run_test(pool, |mut tester| async { - tester.get_branch_mut("pr-1").expect_suites(1); + tester.default_pr_head_branch_mut().expect_suites(1); tester.post_comment("@bors r+").await?; tester.expect_comments(1).await; - let branch = tester.get_branch("pr-1"); + let branch = tester.default_pr_head_branch(); tester.workflow_success(branch.clone()).await?; tester.check_suite(CheckSuite::completed(branch)).await?; diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs index c6650ae..c0ff297 100644 --- a/src/tests/mocks/bors.rs +++ b/src/tests/mocks/bors.rs @@ -165,6 +165,14 @@ impl BorsTester { .await } + pub fn default_pr_head_branch_mut(&mut self) -> MappedMutexGuard { + self.get_branch_mut(&format!("pr-{}", default_pr_number())) + } + + pub fn default_pr_head_branch(&self) -> Branch { + self.get_branch(&format!("pr-{}", default_pr_number())) + } + pub async fn default_pr_db(&self) -> anyhow::Result> { self.pr_db(default_repo_name(), default_pr_number()).await } From 49dcc4fd01cb350e8d543b3272071ff2e3b38793 Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Sun, 23 Mar 2025 16:41:36 +0000 Subject: [PATCH 7/8] Cleanup --- src/bors/handlers/refresh.rs | 3 +-- src/bors/handlers/review.rs | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/bors/handlers/refresh.rs b/src/bors/handlers/refresh.rs index 45d07f5..f8f01b5 100644 --- a/src/bors/handlers/refresh.rs +++ b/src/bors/handlers/refresh.rs @@ -337,8 +337,7 @@ timeout = 3600 tester.post_comment("@bors r+").await?; tester.expect_comments(1).await; - let pr = tester.default_pr_db().await?.unwrap(); - assert!(pr.is_pending_approval()); + tester.default_pr().await.expect_approval_pending(); with_mocked_time(Duration::from_secs(1800), async { tester.refresh().await; diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index f6a89f9..cd37668 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -1111,9 +1111,7 @@ mod tests { @":hourglass_flowing_sand: Commit pr-1-sha has received approval from `default-user`, but is waiting for CI to pass" ); - let pr = tester.default_pr_db().await?.unwrap(); - assert!(pr.is_pending_approval()); - + tester.default_pr().await.expect_approval_pending(); Ok(tester) }) .await; From 151e5f0ff71e93261c095d6825301c5bfb3ce6e5 Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Sun, 23 Mar 2025 16:46:21 +0000 Subject: [PATCH 8/8] Cleanup --- src/bors/handlers/review.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index cd37668..8bc064c 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -1151,9 +1151,11 @@ mod tests { @"Commit pr-1-sha has been approved by `default-user`" ); - let pr = tester.default_pr_db().await?.unwrap(); - assert!(pr.is_approved()); - assert_eq!(pr.priority, Some(10)); + tester + .default_pr() + .await + .expect_approved_by("default-user") + .expect_priority(Some(10)); Ok(tester) }) @@ -1170,11 +1172,11 @@ mod tests { tester.get_comment().await?, @"Commit pr-1-sha has been approved by `default-user`" ); - - let pr = tester.default_pr_db().await?.unwrap(); - assert!(pr.is_approved()); - assert_eq!(pr.priority, Some(5)); - + tester + .default_pr() + .await + .expect_approved_by("default-user") + .expect_priority(Some(5)); Ok(tester) }) .await; @@ -1188,13 +1190,11 @@ mod tests { branch.expect_suites(1); branch.suite_finished(TestWorkflowStatus::Failure); } - tester.post_comment("@bors r+").await?; tester.expect_comments(1).await; let pr = tester.default_pr_db().await?.unwrap(); assert!(!pr.is_approved()); - { let mut branch = tester.default_pr_head_branch_mut(); branch.expect_suites(1); @@ -1205,7 +1205,6 @@ mod tests { tester.expect_comments(1).await; tester.default_pr().await.expect_approved_by("default-user"); - Ok(tester) }) .await;