From 8e279bf3f970e857fb185395fb495ec4e08ae6ee Mon Sep 17 00:00:00 2001 From: Rhythm1710 Date: Tue, 11 Mar 2025 22:32:03 +0530 Subject: [PATCH 1/3] tree_opening_closing --- ...3a08c483e5a6238344c7ac969a0602674e888.json | 16 +++ ...44d4bc654978836fe5ceda2f0041aeba68e64.json | 46 +++++++ ...250304172534_add_repository_model.down.sql | 2 + ...20250304172534_add_repository_model.up.sql | 9 ++ src/bors/command/mod.rs | 4 + src/bors/command/parser.rs | 65 +++++++++- src/bors/handlers/help.rs | 11 +- src/bors/handlers/mod.rs | 17 ++- src/bors/handlers/review.rs | 122 ++++++++++++++++++ src/database/client.rs | 23 +++- src/database/mod.rs | 22 ++++ src/database/operations.rs | 64 +++++++++ src/permissions.rs | 4 +- 13 files changed, 395 insertions(+), 10 deletions(-) create mode 100644 .sqlx/query-6a911b59abd89bdb10e283902ea3a08c483e5a6238344c7ac969a0602674e888.json create mode 100644 .sqlx/query-6d68067c7121438630d77590ed644d4bc654978836fe5ceda2f0041aeba68e64.json create mode 100644 migrations/20250304172534_add_repository_model.down.sql create mode 100644 migrations/20250304172534_add_repository_model.up.sql diff --git a/.sqlx/query-6a911b59abd89bdb10e283902ea3a08c483e5a6238344c7ac969a0602674e888.json b/.sqlx/query-6a911b59abd89bdb10e283902ea3a08c483e5a6238344c7ac969a0602674e888.json new file mode 100644 index 00000000..1364818a --- /dev/null +++ b/.sqlx/query-6a911b59abd89bdb10e283902ea3a08c483e5a6238344c7ac969a0602674e888.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "\n INSERT INTO repository (name, tree_state, treeclosed_src)\n VALUES ($1, $2, $3)\n ON CONFLICT (name)\n DO UPDATE SET tree_state = EXCLUDED.tree_state, treeclosed_src = EXCLUDED.treeclosed_src\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Int4", + "Text" + ] + }, + "nullable": [] + }, + "hash": "6a911b59abd89bdb10e283902ea3a08c483e5a6238344c7ac969a0602674e888" +} diff --git a/.sqlx/query-6d68067c7121438630d77590ed644d4bc654978836fe5ceda2f0041aeba68e64.json b/.sqlx/query-6d68067c7121438630d77590ed644d4bc654978836fe5ceda2f0041aeba68e64.json new file mode 100644 index 00000000..a58dff99 --- /dev/null +++ b/.sqlx/query-6d68067c7121438630d77590ed644d4bc654978836fe5ceda2f0041aeba68e64.json @@ -0,0 +1,46 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT id, name as \"name: GithubRepoName\", tree_state, treeclosed_src, created_at\n FROM repository\n WHERE name = $1\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Int4" + }, + { + "ordinal": 1, + "name": "name: GithubRepoName", + "type_info": "Text" + }, + { + "ordinal": 2, + "name": "tree_state", + "type_info": "Int4" + }, + { + "ordinal": 3, + "name": "treeclosed_src", + "type_info": "Text" + }, + { + "ordinal": 4, + "name": "created_at", + "type_info": "Timestamptz" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false, + false, + true, + true, + false + ] + }, + "hash": "6d68067c7121438630d77590ed644d4bc654978836fe5ceda2f0041aeba68e64" +} diff --git a/migrations/20250304172534_add_repository_model.down.sql b/migrations/20250304172534_add_repository_model.down.sql new file mode 100644 index 00000000..8d5d7af3 --- /dev/null +++ b/migrations/20250304172534_add_repository_model.down.sql @@ -0,0 +1,2 @@ +-- Add down migration script here +DROP TABLE IF EXISTS repository; \ No newline at end of file diff --git a/migrations/20250304172534_add_repository_model.up.sql b/migrations/20250304172534_add_repository_model.up.sql new file mode 100644 index 00000000..70489ab8 --- /dev/null +++ b/migrations/20250304172534_add_repository_model.up.sql @@ -0,0 +1,9 @@ +-- Add up migration script here +CREATE TABLE repository +( + id SERIAL PRIMARY KEY, + name TEXT NOT NULL UNIQUE, + tree_state INT NULL, + treeclosed_src TEXT, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); \ No newline at end of file diff --git a/src/bors/command/mod.rs b/src/bors/command/mod.rs index d8f8a405..28dfa7d6 100644 --- a/src/bors/command/mod.rs +++ b/src/bors/command/mod.rs @@ -99,4 +99,8 @@ pub enum BorsCommand { Undelegate, /// Set the rollup mode of a PRstatus. SetRollupMode(RollupMode), + /// Open the repository tree for merging. + OpenTree, + /// Set the tree closed with a priority level. + TreeClosed(Priority), } diff --git a/src/bors/command/parser.rs b/src/bors/command/parser.rs index 518994ba..08bc9d6c 100644 --- a/src/bors/command/parser.rs +++ b/src/bors/command/parser.rs @@ -19,7 +19,7 @@ pub enum CommandParseError<'a> { } /// Part of a command, either a bare string like `try` or a key value like `parent=`. -#[derive(PartialEq)] +#[derive(PartialEq, Copy, Clone)] enum CommandPart<'a> { Bare(&'a str), KeyValue { key: &'a str, value: &'a str }, @@ -68,6 +68,7 @@ const PARSERS: &[for<'b> fn(&CommandPart<'b>, &[CommandPart<'b>]) -> ParseResult parser_info, parser_help, parser_ping, + parser_tree_ops, ]; fn parse_command(input: &str) -> ParseResult { @@ -313,6 +314,24 @@ fn parser_info<'a>(command: &CommandPart<'a>, _parts: &[CommandPart<'a>]) -> Par } } +/// Parses `@bors treeclosed-` and `@bors treeclosed=` +fn parser_tree_ops<'a>(command: &CommandPart<'a>, _parts: &[CommandPart<'a>]) -> ParseResult<'a> { + match command { + CommandPart::Bare("treeclosed-") => Some(Ok(BorsCommand::OpenTree)), + CommandPart::KeyValue { + key: "treeclosed", + value, + } => { + let priority = match parse_priority_value(value) { + Ok(p) => p, + Err(error) => return Some(Err(error)), + }; + Some(Ok(BorsCommand::TreeClosed(priority))) + } + _ => None, + } +} + #[cfg(test)] mod tests { use crate::bors::command::parser::{CommandParseError, CommandParser}; @@ -1010,6 +1029,50 @@ line two assert_eq!(cmds[0], Ok(BorsCommand::Delegate)); } + #[test] + fn parse_tree_closed() { + let cmds = parse_commands("@bors treeclosed=5"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::TreeClosed(5))); + } + + #[test] + fn parse_tree_closed_invalid() { + let cmds = parse_commands("@bors treeclosed=abc"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::ValidationError(_)) + )); + } + + #[test] + fn parse_tree_closed_empty() { + let cmds = parse_commands("@bors treeclosed="); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::MissingArgValue { arg: "treeclosed" }) + )); + } + + #[test] + fn parse_tree_closed_minus() { + let cmds = parse_commands("@bors treeclosed-"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::OpenTree)); + } + + #[test] + fn parse_tree_closed_unknown_command() { + let cmds = parse_commands("@bors tree closed 5"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::UnknownCommand("tree")) + )); + } + fn parse_commands(text: &str) -> Vec> { CommandParser::new("@bors".to_string()).parse_commands(text) } diff --git a/src/bors/handlers/help.rs b/src/bors/handlers/help.rs index b209e877..e2b9e8ab 100644 --- a/src/bors/handlers/help.rs +++ b/src/bors/handlers/help.rs @@ -32,6 +32,8 @@ pub(super) async fn command_help( BorsCommand::Info, BorsCommand::Ping, BorsCommand::Help, + BorsCommand::OpenTree, + BorsCommand::TreeClosed(0), ] .into_iter() .map(|help| format!("- {}", get_command_help(help))) @@ -83,7 +85,12 @@ fn get_command_help(command: BorsCommand) -> String { BorsCommand::Info => { "`info`: Get information about the current PR including delegation, priority, merge status, and try build status" } - + BorsCommand::OpenTree => { + "`treeclosed-`: Open the repository tree for merging" + } + BorsCommand::TreeClosed(_) => { + "`treeclosed=`: Close the tree for PRs with priority less than ``" + } }; help.to_string() } @@ -109,6 +116,8 @@ mod tests { - `info`: Get information about the current PR including delegation, priority, merge status, and try build status - `ping`: Check if the bot is alive - `help`: Print this help message + - `treeclosed-`: Open the repository tree for merging + - `treeclosed=`: Close the tree for PRs with priority less than `` "); Ok(tester) }) diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 95d7cc6d..481ab455 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -6,7 +6,9 @@ use crate::bors::handlers::help::command_help; use crate::bors::handlers::info::command_info; use crate::bors::handlers::ping::command_ping; use crate::bors::handlers::refresh::refresh_repository; -use crate::bors::handlers::review::{command_approve, command_unapprove}; +use crate::bors::handlers::review::{ + command_approve, command_close_tree, command_open_tree, command_unapprove, +}; use crate::bors::handlers::trybuild::{command_try_build, command_try_cancel, TRY_BRANCH_NAME}; use crate::bors::handlers::workflow::{ handle_check_suite_completed, handle_workflow_completed, handle_workflow_started, @@ -86,7 +88,6 @@ pub async fn handle_bors_repository_event( return Err(error.context("Cannot perform command")); } } - BorsRepositoryEvent::WorkflowStarted(payload) => { let span = tracing::info_span!( "Workflow started", @@ -227,6 +228,18 @@ async fn handle_comment( .instrument(span) .await } + BorsCommand::OpenTree => { + let span = tracing::info_span!("TreeOpen"); + command_open_tree(repo, database, &pull_request, &comment.author) + .instrument(span) + .await + } + BorsCommand::TreeClosed(priority) => { + let span = tracing::info_span!("TreeClosed"); + command_close_tree(repo, database, &pull_request, &comment.author, priority) + .instrument(span) + .await + } BorsCommand::Unapprove => { let span = tracing::info_span!("Unapprove"); command_unapprove(repo, database, &pull_request, &comment.author) diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 88ad5f2e..2549f1c5 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -7,6 +7,7 @@ use crate::bors::handlers::has_permission; use crate::bors::handlers::labels::handle_label_trigger; use crate::bors::Comment; use crate::bors::RepositoryState; +use crate::database::TreeState; use crate::github::GithubUser; use crate::github::LabelTrigger; use crate::github::PullRequest; @@ -129,6 +130,78 @@ pub(super) async fn command_set_rollup( db.set_rollup(repo_state.repository(), pr.number, rollup) .await } +pub(super) async fn command_close_tree( + repo_state: Arc, + db: Arc, + pr: &PullRequest, + author: &GithubUser, + priority: u32, +) -> anyhow::Result<()> { + if !sufficient_approve_permission(repo_state.clone(), author) { + deny_request(&repo_state, pr, author, PermissionType::Review).await?; + return Ok(()); + }; + db.upsert_repository( + repo_state.repository(), + TreeState::Closed { + priority, + source: format!( + "https://github.com/{}/pull/{}", + repo_state.repository(), + pr.number + ), + }, + ) + .await?; + notify_of_tree_closed(&repo_state, pr, priority).await +} + +pub(super) async fn command_open_tree( + repo_state: Arc, + db: Arc, + pr: &PullRequest, + author: &GithubUser, +) -> anyhow::Result<()> { + if !sufficient_delegate_permission(repo_state.clone(), author) { + deny_request(&repo_state, pr, author, PermissionType::Review).await?; + return Ok(()); + } + + db.upsert_repository(repo_state.repository(), TreeState::Open) + .await?; + notify_of_tree_open(&repo_state, pr).await +} + +async fn notify_of_tree_closed( + repo: &RepositoryState, + pr: &PullRequest, + priority: u32, +) -> anyhow::Result<()> { + repo.client + .post_comment( + pr.number, + Comment::new(format!( + "Tree closed for PRs with priority less than {}", + priority + )), + ) + .await +} + +async fn notify_of_tree_open(repo: &RepositoryState, pr: &PullRequest) -> anyhow::Result<()> { + repo.client + .post_comment( + pr.number, + Comment::new("Tree is now open for merging".to_string()), + ) + .await +} + +fn sufficient_approve_permission(repo: Arc, author: &GithubUser) -> bool { + repo.permissions + .load() + .has_permission(author.id, PermissionType::Review) +} fn sufficient_delegate_permission(repo: Arc, author: &GithubUser) -> bool { repo.permissions @@ -176,6 +249,7 @@ async fn notify_of_delegation( #[cfg(test)] mod tests { + use crate::database::TreeState; use crate::tests::mocks::{ assert_pr_approved_by, assert_pr_unapproved, create_world_with_approve_config, }; @@ -392,6 +466,54 @@ mod tests { .await; } + #[sqlx::test] + async fn tree_closed_with_priority(pool: sqlx::PgPool) { + let world = create_world_with_approve_config(); + BorsBuilder::new(pool) + .world(world) + .run_test(|mut tester| async { + tester.post_comment("@bors treeclosed=5").await?; + assert_eq!( + tester.get_comment().await?, + "Tree closed for PRs with priority less than 5" + ); + + let repo = tester.db().get_repository(&default_repo_name()).await?; + assert_eq!( + repo.unwrap().tree_state, + TreeState::Closed { + priority: 5, + source: format!( + "https://github.com/{}/pull/{}", + default_repo_name(), + default_pr_number() + ), + } + ); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn insufficient_permission_tree_closed(pool: sqlx::PgPool) { + let world = World::default(); + world.default_repo().lock().permissions = Permissions::default(); + + BorsBuilder::new(pool) + .world(world) + .run_test(|mut tester| async { + tester.post_comment("@bors treeclosed=5").await?; + assert_eq!( + tester.get_comment().await?, + "@default-user: :key: Insufficient privileges: not in review users" + ); + Ok(tester) + }) + .await; + } + fn reviewer() -> User { User::new(10, "reviewer") } diff --git a/src/database/client.rs b/src/database/client.rs index 04d5a09e..369bac44 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -2,17 +2,18 @@ use sqlx::PgPool; use crate::bors::RollupMode; use crate::database::{ - BuildModel, BuildStatus, PullRequestModel, WorkflowModel, WorkflowStatus, WorkflowType, + BuildModel, BuildStatus, PullRequestModel, RepoModel, TreeState, WorkflowModel, WorkflowStatus, + WorkflowType, }; use crate::github::PullRequestNumber; 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_running_builds, - get_workflow_urls_for_build, get_workflows_for_build, set_pr_priority, set_pr_rollup, - unapprove_pull_request, undelegate_pull_request, update_build_status, update_pr_build_id, - update_workflow_status, + 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, + set_pr_rollup, unapprove_pull_request, undelegate_pull_request, update_build_status, + update_pr_build_id, update_workflow_status, upsert_repository, }; use super::RunId; @@ -206,4 +207,16 @@ impl PgDbClient { .collect::>(); Ok(workflows) } + + pub async fn get_repository(&self, repo: &GithubRepoName) -> anyhow::Result> { + get_repository(&self.pool, repo).await + } + + pub async fn upsert_repository( + &self, + repo: &GithubRepoName, + tree_state: TreeState, + ) -> anyhow::Result<()> { + upsert_repository(&self.pool, repo, tree_state).await + } } diff --git a/src/database/mod.rs b/src/database/mod.rs index 723df514..bcd6a08d 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -196,3 +196,25 @@ pub struct WorkflowModel { pub status: WorkflowStatus, pub created_at: DateTime, } + +/// Represents the state of a repository's tree. +#[derive(Debug, PartialEq, Clone)] +pub enum TreeState { + /// The repository tree is open for changes + Open, + /// The repository tree is closed to changes with a priority threshold + Closed { + /// PRs with priority lower than this value cannot be merged. + priority: u32, + /// URL to a PR comment that closed the tree. + source: String, + }, +} + +/// Represents a repository configuration. +pub struct RepoModel { + pub id: PrimaryKey, + pub name: GithubRepoName, + pub tree_state: TreeState, + pub created_at: DateTime, +} diff --git a/src/database/operations.rs b/src/database/operations.rs index 35e66db0..a58ce208 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -4,6 +4,7 @@ use sqlx::postgres::PgExecutor; use crate::bors::RollupMode; use crate::database::BuildStatus; +use crate::database::RepoModel; use crate::database::WorkflowModel; use crate::github::CommitSha; use crate::github::GithubRepoName; @@ -12,6 +13,7 @@ use crate::github::PullRequestNumber; use super::BuildModel; use super::PullRequestModel; use super::RunId; +use super::TreeState; use super::WorkflowStatus; use super::WorkflowType; @@ -448,3 +450,65 @@ FROM workflow .await?; Ok(workflows) } + +/// Retrieves a repository from the database or creates it if it does not exist. +pub(crate) async fn get_repository( + executor: impl PgExecutor<'_>, + repo: &GithubRepoName, +) -> anyhow::Result> { + let repo = sqlx::query!( + r#" + SELECT id, name as "name: GithubRepoName", tree_state, treeclosed_src, created_at + FROM repository + WHERE name = $1 + "#, + repo.to_string() + ) + .fetch_optional(executor) + .await?; + + Ok(repo.map(|repo| { + let tree_state = match repo.tree_state { + None => TreeState::Open, + Some(priority) => TreeState::Closed { + priority: priority as u32, + source: repo + .treeclosed_src + .expect("treeclosed_src is NULL even though tree_state is non-NULL"), + }, + }; + RepoModel { + id: repo.id, + name: repo.name, + tree_state, + created_at: repo.created_at, + } + })) +} + +/// Updates the tree state of a repository. +pub(crate) async fn upsert_repository( + executor: impl PgExecutor<'_>, + repo: &GithubRepoName, + tree_state: TreeState, +) -> anyhow::Result<()> { + let (priority, src) = match tree_state { + TreeState::Open => (None, None), + TreeState::Closed { priority, source } => (Some(priority as i32), Some(source)), + }; + sqlx::query!( + r#" + INSERT INTO repository (name, tree_state, treeclosed_src) + VALUES ($1, $2, $3) + ON CONFLICT (name) + DO UPDATE SET tree_state = EXCLUDED.tree_state, treeclosed_src = EXCLUDED.treeclosed_src + "#, + repo.to_string(), + priority, + src + ) + .execute(executor) + .await?; + + Ok(()) +} diff --git a/src/permissions.rs b/src/permissions.rs index 0636fcfb..7ba1ca38 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -7,7 +7,7 @@ use crate::github::GithubRepoName; #[derive(Eq, PartialEq, Debug, Clone)] pub enum PermissionType { - /// Can perform commands like r+. + /// Can perform commands like r+ Review, /// Can start a try build. Try, @@ -34,6 +34,7 @@ impl UserPermissions { try_users, } } + pub fn has_permission(&self, user_id: UserId, permission: PermissionType) -> bool { match permission { PermissionType::Review => self.review_users.contains(&user_id), @@ -73,6 +74,7 @@ impl TeamApiClient { .load_users(repo.name(), PermissionType::Try) .await .map_err(|error| anyhow::anyhow!("Cannot load try users: {error:?}"))?; + Ok(UserPermissions { review_users, try_users, From 30ca72a7ac018b1cf73d6101693fe1e88c8bebab Mon Sep 17 00:00:00 2001 From: Rhythm1710 Date: Tue, 11 Mar 2025 23:57:25 +0530 Subject: [PATCH 2/3] tree_opening_closing mock server tests --- src/bors/event.rs | 1 + src/bors/handlers/mod.rs | 13 ++++++++++--- src/bors/handlers/review.rs | 12 ++++-------- src/github/webhook.rs | 18 ++++++++++++------ src/tests/mocks/comment.rs | 29 +++++++++++++++++++++++------ src/tests/mocks/pull_request.rs | 15 ++++++++++----- src/tests/mocks/repository.rs | 6 ++++++ 7 files changed, 66 insertions(+), 28 deletions(-) diff --git a/src/bors/event.rs b/src/bors/event.rs index f98bca1c..f83e00df 100644 --- a/src/bors/event.rs +++ b/src/bors/event.rs @@ -56,6 +56,7 @@ pub struct PullRequestComment { pub author: GithubUser, pub pr_number: PullRequestNumber, pub text: String, + pub html_url: String, } #[derive(Debug)] diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 481ab455..0a1768fb 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -236,9 +236,16 @@ async fn handle_comment( } BorsCommand::TreeClosed(priority) => { let span = tracing::info_span!("TreeClosed"); - command_close_tree(repo, database, &pull_request, &comment.author, priority) - .instrument(span) - .await + command_close_tree( + repo, + database, + &pull_request, + &comment.author, + priority, + &comment.html_url, + ) + .instrument(span) + .await } BorsCommand::Unapprove => { let span = tracing::info_span!("Unapprove"); diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 2549f1c5..90d58249 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -136,6 +136,7 @@ pub(super) async fn command_close_tree( pr: &PullRequest, author: &GithubUser, priority: u32, + comment_url: &str, ) -> anyhow::Result<()> { if !sufficient_approve_permission(repo_state.clone(), author) { deny_request(&repo_state, pr, author, PermissionType::Review).await?; @@ -145,11 +146,7 @@ pub(super) async fn command_close_tree( repo_state.repository(), TreeState::Closed { priority, - source: format!( - "https://github.com/{}/pull/{}", - repo_state.repository(), - pr.number - ), + source: comment_url.to_string(), }, ) .await?; @@ -484,9 +481,8 @@ mod tests { TreeState::Closed { priority: 5, source: format!( - "https://github.com/{}/pull/{}", - default_repo_name(), - default_pr_number() + "https://github.com/{}/pull/1#issuecomment-1", + default_repo_name() ), } ); diff --git a/src/github/webhook.rs b/src/github/webhook.rs index e100003a..0809b3c6 100644 --- a/src/github/webhook.rs +++ b/src/github/webhook.rs @@ -352,6 +352,7 @@ fn parse_pr_review_comment( author: user, pr_number: PullRequestNumber(payload.pull_request.number), text: payload.comment.body.unwrap_or_default(), + html_url: payload.comment.html_url.to_string(), } } @@ -366,6 +367,7 @@ fn parse_comment_from_pr_review( author: user, pr_number: PullRequestNumber(payload.pull_request.number), text: payload.review.body.unwrap_or_default(), + html_url: payload.review.html_url.to_string(), }) } @@ -384,6 +386,7 @@ fn parse_pr_comment( author: payload.comment.user.into(), text: payload.comment.body.unwrap_or_default(), pr_number: PullRequestNumber(payload.issue.number), + html_url: payload.comment.html_url.to_string(), }) } @@ -456,7 +459,7 @@ mod tests { async fn issue_comment() { insta::assert_debug_snapshot!( check_webhook("webhook/issue-comment.json", "issue_comment").await, - @r###" + @r#" Ok( GitHubWebhook( Repository( @@ -491,12 +494,13 @@ mod tests { 5, ), text: "hello bors", + html_url: "https://github.com/Kobzol/bors-kindergarten/pull/5#issuecomment-1420770715", }, ), ), ), ) - "### + "# ); } @@ -637,7 +641,7 @@ mod tests { async fn pull_request_review() { insta::assert_debug_snapshot!( check_webhook("webhook/pull-request-review.json", "pull_request_review").await, - @r###" + @r#" Ok( GitHubWebhook( Repository( @@ -672,12 +676,13 @@ mod tests { 6, ), text: "review comment", + html_url: "https://github.com/Kobzol/bors-kindergarten/pull/6#pullrequestreview-1476702458", }, ), ), ), ) - "### + "# ); } @@ -685,7 +690,7 @@ mod tests { async fn pull_request_review_comment() { insta::assert_debug_snapshot!( check_webhook("webhook/pull-request-review-comment.json", "pull_request_review_comment").await, - @r###" + @r#" Ok( GitHubWebhook( Repository( @@ -720,12 +725,13 @@ mod tests { 6, ), text: "Foo", + html_url: "https://github.com/Kobzol/bors-kindergarten/pull/6#discussion_r1227824551", }, ), ), ), ) - "### + "# ); } diff --git a/src/tests/mocks/comment.rs b/src/tests/mocks/comment.rs index 7e635db8..fe2ccdc2 100644 --- a/src/tests/mocks/comment.rs +++ b/src/tests/mocks/comment.rs @@ -16,6 +16,7 @@ pub struct Comment { pub pr: u64, pub author: User, pub content: String, + pub comment_id: u64, } impl Comment { @@ -25,12 +26,20 @@ impl Comment { pr, author: User::default(), content: content.to_string(), + comment_id: 1, // comment_id will be updated by the mock server } } pub fn with_author(self, author: User) -> Self { Self { author, ..self } } + + pub fn with_id(self, id: u64) -> Self { + Self { + comment_id: id, + ..self + } + } } impl<'a> From<&'a str> for Comment { @@ -52,7 +61,11 @@ pub struct GitHubIssueCommentEventPayload { impl From for GitHubIssueCommentEventPayload { fn from(value: Comment) -> Self { let time = Utc::now(); - let url = Url::parse("https://foo.bar").unwrap(); + let html_url = format!( + "https://github.com/{}/pull/{}#issuecomment-{}", + value.repo, value.pr, value.comment_id + ); + let url = Url::parse(&html_url).unwrap(); Self { repository: value.repo.into(), action: IssueCommentEventAction::Created, @@ -88,8 +101,8 @@ impl From for GitHubIssueCommentEventPayload { updated_at: time, }, comment: GitHubComment { - id: CommentId(1), - node_id: "1".to_string(), + id: CommentId(value.comment_id), + node_id: value.comment_id.to_string(), url: url.clone(), html_url: url, body: Some(value.content.clone()), @@ -151,10 +164,14 @@ pub(super) struct GitHubComment { impl From for GitHubComment { fn from(value: Comment) -> Self { let time = Utc::now(); - let url = Url::parse("https://foo.bar").unwrap(); + let html_url = format!( + "https://github.com/{}/pull/{}#issuecomment-{}", + value.repo, value.pr, value.comment_id + ); + let url = Url::parse(&html_url).unwrap(); Self { - id: CommentId(1), - node_id: "1".to_string(), + id: CommentId(value.comment_id), + node_id: value.comment_id.to_string(), url: url.clone(), html_url: url, body: Some(value.content.clone()), diff --git a/src/tests/mocks/pull_request.rs b/src/tests/mocks/pull_request.rs index c8ef24b4..d6b9369d 100644 --- a/src/tests/mocks/pull_request.rs +++ b/src/tests/mocks/pull_request.rs @@ -45,7 +45,7 @@ pub async fn mock_pull_requests( .await; mock_pr_comments( - repo_name.clone(), + repo.clone(), pr_number, comments_tx.clone(), mock_server, @@ -56,11 +56,12 @@ pub async fn mock_pull_requests( } async fn mock_pr_comments( - repo_name: GithubRepoName, + repo: Arc>, pr_number: u64, comments_tx: Sender, mock_server: &MockServer, ) { + let repo_name = repo.lock().name.clone(); Mock::given(method("POST")) .and(path(format!( "/repos/{repo_name}/issues/{pr_number}/comments", @@ -72,9 +73,13 @@ async fn mock_pr_comments( } let comment_payload: CommentCreatePayload = req.body_json().unwrap(); - let comment: Comment = - Comment::new(repo_name.clone(), pr_number, &comment_payload.body) - .with_author(User::bors_bot()); + let mut repo = repo.lock(); + let pr = repo.pull_requests.get_mut(&pr_number).unwrap(); + let comment_id = pr.next_comment_id(); + + let comment = Comment::new(repo_name.clone(), pr_number, &comment_payload.body) + .with_author(User::bors_bot()) + .with_id(comment_id); // We cannot use `tx.blocking_send()`, because this function is actually called // from within an async task, but it is not async, so we also cannot use diff --git a/src/tests/mocks/repository.rs b/src/tests/mocks/repository.rs index a54664f6..e510839c 100644 --- a/src/tests/mocks/repository.rs +++ b/src/tests/mocks/repository.rs @@ -27,6 +27,7 @@ use super::user::{GitHubUser, User}; pub struct PullRequest { pub added_labels: Vec, pub removed_labels: Vec, + pub comment_counter: u64, } impl PullRequest { @@ -47,6 +48,11 @@ impl PullRequest { .collect::>(); assert_eq!(&removed_labels, labels); } + + pub fn next_comment_id(&mut self) -> u64 { + self.comment_counter += 1; + self.comment_counter + } } #[derive(Clone)] From 7973f14e75d336e731e26bb73b949459a6fcbd4a Mon Sep 17 00:00:00 2001 From: Rhythm1710 Date: Wed, 12 Mar 2025 00:03:01 +0530 Subject: [PATCH 3/3] fmt changes --- src/tests/mocks/pull_request.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/tests/mocks/pull_request.rs b/src/tests/mocks/pull_request.rs index d6b9369d..1e678222 100644 --- a/src/tests/mocks/pull_request.rs +++ b/src/tests/mocks/pull_request.rs @@ -44,13 +44,7 @@ pub async fn mock_pull_requests( .mount(mock_server) .await; - mock_pr_comments( - repo.clone(), - pr_number, - comments_tx.clone(), - mock_server, - ) - .await; + mock_pr_comments(repo.clone(), pr_number, comments_tx.clone(), mock_server).await; mock_pr_labels(repo.clone(), repo_name.clone(), pr_number, mock_server).await; } }