diff --git a/.sqlx/query-32c1cd799f8a1e8c451493959419570892d76e7adbc3e5c7b50e97e1bfee27bd.json b/.sqlx/query-32c1cd799f8a1e8c451493959419570892d76e7adbc3e5c7b50e97e1bfee27bd.json new file mode 100644 index 00000000..f02c3a48 --- /dev/null +++ b/.sqlx/query-32c1cd799f8a1e8c451493959419570892d76e7adbc3e5c7b50e97e1bfee27bd.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET approved_by = NULL WHERE id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [] + }, + "hash": "32c1cd799f8a1e8c451493959419570892d76e7adbc3e5c7b50e97e1bfee27bd" +} diff --git a/.sqlx/query-01f9c2c10c4670b6da94c33b5256b76742779bb0820620d79c3fccb5d40078bd.json b/.sqlx/query-6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128.json similarity index 53% rename from .sqlx/query-01f9c2c10c4670b6da94c33b5256b76742779bb0820620d79c3fccb5d40078bd.json rename to .sqlx/query-6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128.json index 222dcdd0..e1b5b93a 100644 --- a/.sqlx/query-01f9c2c10c4670b6da94c33b5256b76742779bb0820620d79c3fccb5d40078bd.json +++ b/.sqlx/query-6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n CASE WHEN pr.build_id IS NULL\n THEN NULL\n ELSE (\n build.id,\n build.repository,\n build.branch,\n build.commit_sha,\n build.status,\n build.parent,\n build.created_at\n )\n END AS \"try_build: BuildModel\",\n pr.created_at as \"created_at: DateTime\"\nFROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\nWHERE pr.repository = $1\n AND pr.number = $2\n", + "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n CASE WHEN pr.build_id IS NULL\n THEN NULL\n ELSE (\n build.id,\n build.repository,\n build.branch,\n build.commit_sha,\n build.status,\n build.parent,\n build.created_at\n )\n END AS \"try_build: BuildModel\",\n pr.created_at as \"created_at: DateTime\"\nFROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\nWHERE pr.repository = $1\n AND pr.number = $2\n", "describe": { "columns": [ { @@ -20,11 +20,16 @@ }, { "ordinal": 3, + "name": "approved_by", + "type_info": "Text" + }, + { + "ordinal": 4, "name": "try_build: BuildModel", "type_info": "Record" }, { - "ordinal": 4, + "ordinal": 5, "name": "created_at: DateTime", "type_info": "Timestamptz" } @@ -39,9 +44,10 @@ false, false, false, + true, null, false ] }, - "hash": "01f9c2c10c4670b6da94c33b5256b76742779bb0820620d79c3fccb5d40078bd" + "hash": "6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128" } diff --git a/.sqlx/query-7a9718cb973d09f68a6854a4fb08871f29d60b8617caf94dfa439f9ef8453caa.json b/.sqlx/query-7a9718cb973d09f68a6854a4fb08871f29d60b8617caf94dfa439f9ef8453caa.json new file mode 100644 index 00000000..3a40775e --- /dev/null +++ b/.sqlx/query-7a9718cb973d09f68a6854a4fb08871f29d60b8617caf94dfa439f9ef8453caa.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET approved_by = $1 WHERE id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Int4" + ] + }, + "nullable": [] + }, + "hash": "7a9718cb973d09f68a6854a4fb08871f29d60b8617caf94dfa439f9ef8453caa" +} diff --git a/.sqlx/query-2ec9efc7c638384a46681a9247d78d4b822eff5f6433f12a0bbf25a51a30a1ae.json b/.sqlx/query-9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d.json similarity index 54% rename from .sqlx/query-2ec9efc7c638384a46681a9247d78d4b822eff5f6433f12a0bbf25a51a30a1ae.json rename to .sqlx/query-9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d.json index e8f28740..11289097 100644 --- a/.sqlx/query-2ec9efc7c638384a46681a9247d78d4b822eff5f6433f12a0bbf25a51a30a1ae.json +++ b/.sqlx/query-9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n CASE WHEN pr.build_id IS NULL\n THEN NULL\n ELSE (\n build.id,\n build.repository,\n build.branch,\n build.commit_sha,\n build.status,\n build.parent,\n build.created_at\n )\n END AS \"try_build: BuildModel\",\n pr.created_at as \"created_at: DateTime\"\nFROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\nWHERE build.id = $1\n", + "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n CASE WHEN pr.build_id IS NULL\n THEN NULL\n ELSE (\n build.id,\n build.repository,\n build.branch,\n build.commit_sha,\n build.status,\n build.parent,\n build.created_at\n )\n END AS \"try_build: BuildModel\",\n pr.created_at as \"created_at: DateTime\"\nFROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\nWHERE build.id = $1\n", "describe": { "columns": [ { @@ -20,11 +20,16 @@ }, { "ordinal": 3, + "name": "approved_by", + "type_info": "Text" + }, + { + "ordinal": 4, "name": "try_build: BuildModel", "type_info": "Record" }, { - "ordinal": 4, + "ordinal": 5, "name": "created_at: DateTime", "type_info": "Timestamptz" } @@ -38,9 +43,10 @@ false, false, false, + true, null, false ] }, - "hash": "2ec9efc7c638384a46681a9247d78d4b822eff5f6433f12a0bbf25a51a30a1ae" + "hash": "9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d" } diff --git a/migrations/20240626072340_add_approved_by_to_pr.down.sql b/migrations/20240626072340_add_approved_by_to_pr.down.sql new file mode 100644 index 00000000..af99433d --- /dev/null +++ b/migrations/20240626072340_add_approved_by_to_pr.down.sql @@ -0,0 +1,2 @@ +-- Add down migration script here +ALTER TABLE pull_request DROP COLUMN approved_by; diff --git a/migrations/20240626072340_add_approved_by_to_pr.up.sql b/migrations/20240626072340_add_approved_by_to_pr.up.sql new file mode 100644 index 00000000..1bca0823 --- /dev/null +++ b/migrations/20240626072340_add_approved_by_to_pr.up.sql @@ -0,0 +1,3 @@ +-- Add up migration script here +ALTER TABLE pull_request ADD COLUMN approved_by TEXT; + diff --git a/rust-bors.example.toml b/rust-bors.example.toml index ab0fa3cd..61a14252 100644 --- a/rust-bors.example.toml +++ b/rust-bors.example.toml @@ -10,6 +10,7 @@ timeout = 3600 # - try_failed: Try build has failed # (Optional) [labels] +approve = ["+approved"] try = ["+foo", "-bar"] try_succeed = ["+foobar", "+foo", "+baz"] try_failed = [] diff --git a/src/bors/command/mod.rs b/src/bors/command/mod.rs index 2195a546..37f30fc3 100644 --- a/src/bors/command/mod.rs +++ b/src/bors/command/mod.rs @@ -11,9 +11,21 @@ pub enum Parent { Last, } +#[derive(Clone, Debug, PartialEq)] +pub enum Approver { + /// The approver is the same as the comment author. + Myself, + /// The approver is specified by the user. + Specified(String), +} + /// Bors command specified by a user. #[derive(Debug, PartialEq)] pub enum BorsCommand { + /// Approve a commit. + Approve(Approver), + /// Unapprove a commit. + Unapprove, /// Print help Help, /// Ping the bot. diff --git a/src/bors/command/parser.rs b/src/bors/command/parser.rs index e917088a..e922f46d 100644 --- a/src/bors/command/parser.rs +++ b/src/bors/command/parser.rs @@ -2,7 +2,7 @@ use std::collections::HashSet; -use crate::bors::command::{BorsCommand, Parent}; +use crate::bors::command::{Approver, BorsCommand, Parent}; use crate::github::CommitSha; #[derive(Debug, PartialEq)] @@ -40,8 +40,14 @@ impl CommandParser { text: &'a str, ) -> Vec>> { // The order of the parsers in the vector is important - let parsers: Vec fn(&'b str, &[CommandPart<'b>]) -> ParseResult<'b>> = - vec![parser_help, parser_ping, parser_try_cancel, parser_try]; + let parsers: Vec fn(&'b str, &[CommandPart<'b>]) -> ParseResult<'b>> = vec![ + parse_self_approve, + parse_unapprove, + parser_help, + parser_ping, + parser_try_cancel, + parser_try, + ]; text.lines() .filter_map(|line| match line.find(&self.prefix) { @@ -62,7 +68,11 @@ impl CommandParser { } Some(Err(CommandParseError::UnknownCommand(command))) } + // for `@bors r=user1` CommandPart::KeyValue { .. } => { + if let Some(result) = parse_approve_on_behalf(&parts) { + return Some(result); + } Some(Err(CommandParseError::MissingCommand)) } } @@ -108,6 +118,41 @@ fn parse_parts(input: &str) -> Result, CommandParseError> { /// Parsers +/// Parses "@bors r+" +fn parse_self_approve<'a>(command: &'a str, _parts: &[CommandPart<'a>]) -> ParseResult<'a> { + if command == "r+" { + Some(Ok(BorsCommand::Approve(Approver::Myself))) + } else { + None + } +} + +/// Parses "@bors r=". +fn parse_approve_on_behalf<'a>(parts: &[CommandPart<'a>]) -> ParseResult<'a> { + if let Some(CommandPart::KeyValue { key, value }) = parts.first() { + if *key != "r" { + Some(Err(CommandParseError::UnknownArg(key))) + } else if value.is_empty() { + return Some(Err(CommandParseError::MissingArgValue { arg: "r" })); + } else { + Some(Ok(BorsCommand::Approve(Approver::Specified( + value.to_string(), + )))) + } + } else { + Some(Err(CommandParseError::MissingArgValue { arg: "r" })) + } +} + +/// Parses "@bors r-" +fn parse_unapprove<'a>(command: &'a str, _parts: &[CommandPart<'a>]) -> ParseResult<'a> { + if command == "r-" { + Some(Ok(BorsCommand::Unapprove)) + } else { + None + } +} + /// Parses "@bors help". fn parser_help<'a>(command: &'a str, _parts: &[CommandPart<'a>]) -> ParseResult<'a> { if command == "help" { @@ -199,7 +244,7 @@ fn parser_try_cancel<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseRe #[cfg(test)] mod tests { use crate::bors::command::parser::{CommandParseError, CommandParser}; - use crate::bors::command::{BorsCommand, Parent}; + use crate::bors::command::{Approver, BorsCommand, Parent}; use crate::github::CommitSha; #[test] @@ -242,6 +287,53 @@ mod tests { assert!(matches!(cmds[0], Err(CommandParseError::DuplicateArg("a")))); } + #[test] + fn parse_default_approve() { + let cmds = parse_commands("@bors r+"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Ok(BorsCommand::Approve(Approver::Myself)) + )); + } + + #[test] + fn parse_approve_on_behalf() { + let cmds = parse_commands("@bors r=user1"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Ok( + Approve( + Specified( + "user1", + ), + ), + ) + "###); + } + + #[test] + fn parse_approve_on_behalf_of_only_one_approver() { + let cmds = parse_commands("@bors r=user1,user2"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Ok( + Approve( + Specified( + "user1,user2", + ), + ), + ) + "###); + } + + #[test] + fn parse_unapprove() { + let cmds = parse_commands("@bors r-"); + assert_eq!(cmds.len(), 1); + assert!(matches!(cmds[0], Ok(BorsCommand::Unapprove))); + } + #[test] fn parse_help() { let cmds = parse_commands("@bors help"); diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index d9540b0e..282517ae 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -8,6 +8,7 @@ use crate::bors::event::{BorsGlobalEvent, BorsRepositoryEvent, PullRequestCommen use crate::bors::handlers::help::command_help; 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::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, @@ -22,6 +23,7 @@ mod help; mod labels; mod ping; mod refresh; +mod review; mod trybuild; mod workflow; @@ -179,6 +181,18 @@ async fn handle_comment( let repo = Arc::clone(&repo); let database = Arc::clone(&database); let result = match command { + BorsCommand::Approve(approver) => { + let span = tracing::info_span!("Approve"); + command_approve(repo, database, &pull_request, &comment.author, &approver) + .instrument(span) + .await + } + BorsCommand::Unapprove => { + let span = tracing::info_span!("Unapprove"); + command_unapprove(repo, database, &pull_request, &comment.author) + .instrument(span) + .await + } BorsCommand::Help => { let span = tracing::info_span!("Help"); command_help(repo, &pull_request).instrument(span).await diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs new file mode 100644 index 00000000..73b648ba --- /dev/null +++ b/src/bors/handlers/review.rs @@ -0,0 +1,294 @@ +use std::sync::Arc; + +use crate::bors::command::Approver; +use crate::bors::handlers::labels::handle_label_trigger; +use crate::bors::Comment; +use crate::bors::RepositoryState; +use crate::github::GithubUser; +use crate::github::LabelTrigger; +use crate::github::PullRequest; +use crate::permissions::PermissionType; +use crate::PgDbClient; + +/// Approve a pull request. +/// A pull request can only be approved by a user of sufficient authority. +pub(super) async fn command_approve( + repo_state: Arc, + db: Arc, + pr: &PullRequest, + author: &GithubUser, + approver: &Approver, +) -> anyhow::Result<()> { + tracing::info!("Approving PR {}", pr.number); + if !sufficient_approve_permission(repo_state.clone(), author) { + deny_approve_request(&repo_state, pr, author).await?; + return Ok(()); + }; + let approver = match approver { + Approver::Myself => author.username.clone(), + Approver::Specified(approver) => approver.clone(), + }; + db.approve(repo_state.repository(), pr.number, approver.as_str()) + .await?; + handle_label_trigger(&repo_state, pr.number, LabelTrigger::Approved).await?; + notify_of_approval(&repo_state, pr, approver.as_str()).await +} + +/// Unapprove a pull request. +/// Pull request's author can also unapprove the pull request. +pub(super) async fn command_unapprove( + repo_state: Arc, + db: Arc, + pr: &PullRequest, + author: &GithubUser, +) -> anyhow::Result<()> { + tracing::info!("Unapproving PR {}", pr.number); + if !sufficient_unapprove_permission(repo_state.clone(), pr, author) { + deny_unapprove_request(&repo_state, pr, author).await?; + return Ok(()); + }; + db.unapprove(repo_state.repository(), pr.number).await?; + handle_label_trigger(&repo_state, pr.number, LabelTrigger::Unapproved).await?; + notify_of_unapproval(&repo_state, pr).await +} + +fn sufficient_approve_permission(repo: Arc, author: &GithubUser) -> bool { + repo.permissions + .load() + .has_permission(author.id, PermissionType::Review) +} + +async fn deny_approve_request( + repo: &RepositoryState, + pr: &PullRequest, + author: &GithubUser, +) -> anyhow::Result<()> { + tracing::warn!( + "Permission denied for approve command by {}", + author.username + ); + repo.client + .post_comment( + pr.number, + Comment::new(format!( + "@{}: :key: Insufficient privileges: not in review users", + author.username + )), + ) + .await +} + +async fn notify_of_approval( + repo: &RepositoryState, + pr: &PullRequest, + approver: &str, +) -> anyhow::Result<()> { + repo.client + .post_comment( + pr.number, + Comment::new(format!( + "Commit {} has been approved by `{}`", + pr.head.sha, approver + )), + ) + .await +} + +fn sufficient_unapprove_permission( + repo: Arc, + pr: &PullRequest, + author: &GithubUser, +) -> bool { + author.id == pr.author.id + || repo + .permissions + .load() + .has_permission(author.id, PermissionType::Review) +} + +async fn deny_unapprove_request( + repo: &RepositoryState, + pr: &PullRequest, + author: &GithubUser, +) -> anyhow::Result<()> { + tracing::warn!( + "Permission denied for unapprove command by {}", + author.username + ); + repo.client + .post_comment( + pr.number, + Comment::new(format!( + "@{}: :key: Insufficient privileges: not in review users", + author.username + )), + ) + .await +} + +async fn notify_of_unapproval(repo: &RepositoryState, pr: &PullRequest) -> anyhow::Result<()> { + repo.client + .post_comment( + pr.number, + Comment::new(format!("Commit {} has been unapproved", pr.head.sha)), + ) + .await +} + +#[cfg(test)] +mod tests { + use crate::{ + github::PullRequestNumber, + tests::mocks::{ + default_pr_number, default_repo_name, BorsBuilder, BorsTester, Permissions, User, World, + }, + }; + + #[sqlx::test] + async fn default_approve(pool: sqlx::PgPool) { + let world = World::default(); + world.default_repo().lock().set_config( + r#" +[labels] +approve = ["+approved"] +"#, + ); + BorsBuilder::new(pool) + .world(world) + .run_test(|mut tester| async { + tester.post_comment("@bors r+").await?; + assert_eq!( + tester.get_comment().await?, + format!( + "Commit pr-{}-sha has been approved by `{}`", + default_pr_number(), + User::default_user().name + ), + ); + + check_pr_approved_by( + &tester, + default_pr_number().into(), + &User::default_user().name, + ) + .await; + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_on_behalf(pool: sqlx::PgPool) { + let world = World::default(); + world.default_repo().lock().set_config( + r#" +[labels] +approve = ["+approved"] +"#, + ); + BorsBuilder::new(pool) + .world(world) + .run_test(|mut tester| async { + let approve_user = "user1"; + tester + .post_comment(format!(r#"@bors r={approve_user}"#).as_str()) + .await?; + assert_eq!( + tester.get_comment().await?, + format!( + "Commit pr-{}-sha has been approved by `{approve_user}`", + default_pr_number(), + ), + ); + + check_pr_approved_by(&tester, default_pr_number().into(), approve_user).await; + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn insufficient_permission_approve(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 try").await?; + assert_eq!( + tester.get_comment().await?, + "@default-user: :key: Insufficient privileges: not in try users" + ); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + #[tracing_test::traced_test] + async fn unapprove(pool: sqlx::PgPool) { + let world = World::default(); + world.default_repo().lock().set_config( + r#" +[labels] +approve = ["+approved"] +"#, + ); + BorsBuilder::new(pool) + .world(world) + .run_test(|mut tester| async { + tester.post_comment("@bors r+").await?; + assert_eq!( + tester.get_comment().await?, + format!( + "Commit pr-{}-sha has been approved by `{}`", + default_pr_number(), + User::default_user().name + ), + ); + check_pr_approved_by( + &tester, + default_pr_number().into(), + &User::default_user().name, + ) + .await; + tester.post_comment("@bors r-").await?; + assert_eq!( + tester.get_comment().await?, + format!("Commit pr-{}-sha has been unapproved", default_pr_number()), + ); + check_pr_unapproved(&tester, default_pr_number().into()).await; + Ok(tester) + }) + .await; + } + + async fn check_pr_approved_by( + tester: &BorsTester, + pr_number: PullRequestNumber, + approved_by: &str, + ) { + let pr_in_db = tester + .db() + .get_or_create_pull_request(&default_repo_name(), pr_number) + .await + .unwrap(); + assert_eq!(pr_in_db.approved_by, Some(approved_by.to_string())); + let repo = tester.default_repo(); + let pr = repo.lock().get_pr(default_pr_number()).clone(); + pr.check_added_labels(&["approved"]); + } + + async fn check_pr_unapproved(tester: &BorsTester, pr_number: PullRequestNumber) { + let pr_in_db = tester + .db() + .get_or_create_pull_request(&default_repo_name(), pr_number) + .await + .unwrap(); + assert_eq!(pr_in_db.approved_by, None); + let repo = tester.default_repo(); + let pr = repo.lock().get_pr(default_pr_number()).clone(); + pr.check_removed_labels(&["approved"]); + } +} diff --git a/src/config.rs b/src/config.rs index e643ec65..63b95cbe 100644 --- a/src/config.rs +++ b/src/config.rs @@ -42,6 +42,8 @@ where #[derive(serde::Deserialize, Eq, PartialEq, Hash)] #[serde(rename_all = "snake_case")] enum Trigger { + Approve, + Unapprove, Try, TrySucceed, TryFailed, @@ -50,6 +52,8 @@ where impl From for LabelTrigger { fn from(value: Trigger) -> Self { match value { + Trigger::Approve => LabelTrigger::Approved, + Trigger::Unapprove => LabelTrigger::Unapproved, Trigger::Try => LabelTrigger::TryBuildStarted, Trigger::TrySucceed => LabelTrigger::TryBuildSucceeded, Trigger::TryFailed => LabelTrigger::TryBuildFailed, @@ -97,7 +101,20 @@ where } } - let triggers = HashMap::>::deserialize(deserializer)?; + let mut triggers = HashMap::>::deserialize(deserializer)?; + // If there are any `approve` triggers, add `unapprove` triggers as well. + if let Some(modifications) = triggers.get(&Trigger::Approve) { + let unapprove_modifications = modifications + .iter() + .map(|m| match m { + Modification::Add(label) => Modification::Remove(label.clone()), + Modification::Remove(label) => Modification::Add(label.clone()), + }) + .collect::>(); + triggers + .entry(Trigger::Unapprove) + .or_insert_with(|| unapprove_modifications); + } let triggers = triggers .into_iter() .map(|(k, v)| (k.into(), v.into_iter().map(|v| v.into()).collect())) @@ -128,6 +145,7 @@ mod tests { #[test] fn deserialize_labels() { let content = r#"[labels] +approve = ["+approved"] try = ["+foo", "-bar"] try_succeed = ["+foobar", "+foo", "+baz"] try_failed = [] @@ -135,6 +153,16 @@ try_failed = [] let config = load_config(content); insta::assert_debug_snapshot!(config.labels.into_iter().collect::>(), @r###" { + Approved: [ + Add( + "approved", + ), + ], + Unapproved: [ + Remove( + "approved", + ), + ], TryBuildStarted: [ Add( "foo", diff --git a/src/database/client.rs b/src/database/client.rs index 78343440..8849fc37 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -7,9 +7,9 @@ use crate::github::PullRequestNumber; use crate::github::{CommitSha, GithubRepoName}; use super::operations::{ - create_build, create_pull_request, create_workflow, find_build, find_pr_by_build, - get_pull_request, get_running_builds, get_workflows_for_build, update_build_status, - update_pr_build_id, update_workflow_status, + approve_pull_request, create_build, create_pull_request, create_workflow, find_build, + find_pr_by_build, get_pull_request, get_running_builds, get_workflows_for_build, + unapprove_pull_request, update_build_status, update_pr_build_id, update_workflow_status, }; use super::RunId; @@ -24,6 +24,25 @@ impl PgDbClient { Self { pool } } + pub async fn approve( + &self, + repo: &GithubRepoName, + pr_number: PullRequestNumber, + approver: &str, + ) -> anyhow::Result<()> { + let pr = self.get_or_create_pull_request(repo, pr_number).await?; + approve_pull_request(&self.pool, pr.id, approver).await + } + + pub async fn unapprove( + &self, + repo: &GithubRepoName, + pr_number: PullRequestNumber, + ) -> anyhow::Result<()> { + let pr = self.get_or_create_pull_request(repo, pr_number).await?; + unapprove_pull_request(&self.pool, pr.id).await + } + pub async fn get_or_create_pull_request( &self, repo: &GithubRepoName, diff --git a/src/database/mod.rs b/src/database/mod.rs index c8e87851..e5f5f923 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -116,6 +116,7 @@ pub struct PullRequestModel { pub id: PrimaryKey, pub repository: GithubRepoName, pub number: PullRequestNumber, + pub approved_by: Option, pub try_build: Option, pub created_at: DateTime, } diff --git a/src/database/operations.rs b/src/database/operations.rs index 105b7cfe..f6a990fb 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -26,6 +26,7 @@ SELECT pr.id, pr.repository, pr.number, + pr.approved_by, CASE WHEN pr.build_id IS NULL THEN NULL ELSE ( @@ -67,6 +68,34 @@ pub(crate) async fn create_pull_request( Ok(()) } +pub(crate) async fn approve_pull_request( + executor: impl PgExecutor<'_>, + pr_id: i32, + approver: &str, +) -> anyhow::Result<()> { + sqlx::query!( + "UPDATE pull_request SET approved_by = $1 WHERE id = $2", + approver, + pr_id + ) + .execute(executor) + .await?; + Ok(()) +} + +pub(crate) async fn unapprove_pull_request( + executor: impl PgExecutor<'_>, + pr_id: i32, +) -> anyhow::Result<()> { + sqlx::query!( + "UPDATE pull_request SET approved_by = NULL WHERE id = $1", + pr_id + ) + .execute(executor) + .await?; + Ok(()) +} + pub(crate) async fn find_pr_by_build( executor: impl PgExecutor<'_>, build_id: i32, @@ -78,6 +107,7 @@ SELECT pr.id, pr.repository, pr.number, + pr.approved_by, CASE WHEN pr.build_id IS NULL THEN NULL ELSE ( diff --git a/src/github/api/client.rs b/src/github/api/client.rs index 1c04f07f..c91bc3f9 100644 --- a/src/github/api/client.rs +++ b/src/github/api/client.rs @@ -9,7 +9,7 @@ use crate::config::{RepositoryConfig, CONFIG_FILE_PATH}; use crate::database::RunId; use crate::github::api::base_github_html_url; use crate::github::api::operations::{merge_branches, set_branch_to_commit, MergeError}; -use crate::github::{Branch, CommitSha, GithubRepoName, PullRequest, PullRequestNumber}; +use crate::github::{CommitSha, GithubRepoName, PullRequest, PullRequestNumber}; /// Provides access to a single app installation (repository) using the GitHub API. pub struct GithubRepositoryClient { @@ -106,7 +106,7 @@ impl GithubRepositoryClient { .map_err(|error| { anyhow::anyhow!("Could not get PR {}/{}: {error:?}", self.repository(), pr.0) })?; - Ok(github_pr_to_pr(pr)) + Ok(pr.into()) } /// Post a comment to the pull request with the given number. @@ -284,23 +284,6 @@ impl GithubRepositoryClient { } } -fn github_pr_to_pr(pr: octocrab::models::pulls::PullRequest) -> PullRequest { - PullRequest { - number: pr.number.into(), - head_label: pr.head.label.unwrap_or_else(|| "".to_string()), - head: Branch { - name: pr.head.ref_field, - sha: pr.head.sha.into(), - }, - base: Branch { - name: pr.base.ref_field, - sha: pr.base.sha.into(), - }, - title: pr.title.unwrap_or_default(), - message: pr.body.unwrap_or_default(), - } -} - #[cfg(test)] mod tests { use crate::github::GithubRepoName; diff --git a/src/github/labels.rs b/src/github/labels.rs index e210de27..337bb33b 100644 --- a/src/github/labels.rs +++ b/src/github/labels.rs @@ -1,6 +1,8 @@ /// An event that may trigger some modifications of labels on a PR. #[derive(Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] pub enum LabelTrigger { + Approved, + Unapproved, TryBuildStarted, TryBuildSucceeded, TryBuildFailed, diff --git a/src/github/mod.rs b/src/github/mod.rs index b13219b8..23e52804 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -53,13 +53,23 @@ impl From for GithubRepoName { } } -#[derive(Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub struct GithubUser { pub id: UserId, pub username: String, pub html_url: Url, } +impl From for GithubUser { + fn from(value: octocrab::models::Author) -> Self { + Self { + id: value.id, + username: value.login, + html_url: value.html_url, + } + } +} + #[derive(Clone, Debug, PartialEq)] pub struct CommitSha(pub String); @@ -94,6 +104,29 @@ pub struct PullRequest { pub base: Branch, pub title: String, pub message: String, + pub author: GithubUser, +} + +impl From for PullRequest { + fn from(pr: octocrab::models::pulls::PullRequest) -> Self { + Self { + number: pr.number.into(), + head_label: pr.head.label.unwrap_or_else(|| "".to_string()), + head: Branch { + name: pr.head.ref_field, + sha: pr.head.sha.into(), + }, + base: Branch { + name: pr.base.ref_field, + sha: pr.base.sha.into(), + }, + // For some reason, author field is optional in Octocrab, but + // they actually are not optional in Github API schema. + author: (*pr.user.unwrap()).into(), + title: pr.title.unwrap_or_default(), + message: pr.body.unwrap_or_default(), + } + } } #[derive(Clone, Copy, Debug)] diff --git a/src/github/webhook.rs b/src/github/webhook.rs index 7a62f6fe..2196ee82 100644 --- a/src/github/webhook.rs +++ b/src/github/webhook.rs @@ -22,7 +22,7 @@ use crate::bors::event::{ }; use crate::database::{WorkflowStatus, WorkflowType}; use crate::github::server::ServerStateRef; -use crate::github::{CommitSha, GithubRepoName, GithubUser, PullRequestNumber}; +use crate::github::{CommitSha, GithubRepoName, PullRequestNumber}; /// Wrapper for a secret which is zeroed on drop and can be exposed only through the /// [`WebhookSecret::expose`] method. @@ -273,7 +273,7 @@ fn parse_pr_review_comment( repo: GithubRepoName, payload: PullRequestReviewCommentEventPayload, ) -> PullRequestComment { - let user = parse_user(payload.comment.user); + let user = payload.comment.user.into(); PullRequestComment { repository: repo, author: user, @@ -286,7 +286,7 @@ fn parse_comment_from_pr_review( payload: WebhookPullRequestReviewEvent<'_>, ) -> anyhow::Result { let repository_name = parse_repository_name(&payload.repository)?; - let user = parse_user(payload.sender); + let user = payload.sender.into(); Ok(PullRequestComment { repository: repository_name, @@ -296,14 +296,6 @@ fn parse_comment_from_pr_review( }) } -fn parse_user(user: Author) -> GithubUser { - GithubUser { - id: user.id, - username: user.login, - html_url: user.html_url, - } -} - fn parse_pr_comment( repo: GithubRepoName, payload: IssueCommentEventPayload, @@ -316,7 +308,7 @@ fn parse_pr_comment( Some(PullRequestComment { repository: repo, - author: parse_user(payload.comment.user), + author: payload.comment.user.into(), text: payload.comment.body.unwrap_or_default(), pr_number: PullRequestNumber(payload.issue.number), }) diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs index 9192ec49..b1420e99 100644 --- a/src/tests/mocks/bors.rs +++ b/src/tests/mocks/bors.rs @@ -135,7 +135,7 @@ impl BorsTester { self.db.clone() } - pub fn default_repo(&mut self) -> Arc> { + pub fn default_repo(&self) -> Arc> { self.world.get_repo(default_repo_name()) } diff --git a/src/tests/mocks/mod.rs b/src/tests/mocks/mod.rs index 393106d1..bf255229 100644 --- a/src/tests/mocks/mod.rs +++ b/src/tests/mocks/mod.rs @@ -14,6 +14,7 @@ use crate::TeamApiClient; pub use bors::run_test; pub use bors::BorsBuilder; +pub use bors::BorsTester; pub use comment::Comment; pub use permissions::Permissions; pub use pull_request::default_pr_number; diff --git a/src/tests/mocks/pull_request.rs b/src/tests/mocks/pull_request.rs index 8a58c3c8..26e5f011 100644 --- a/src/tests/mocks/pull_request.rs +++ b/src/tests/mocks/pull_request.rs @@ -12,7 +12,9 @@ use wiremock::{ use super::{ comment::{Comment, GitHubComment}, - dynamic_mock_req, Repo, User, + dynamic_mock_req, + user::GitHubUser, + Repo, User, }; pub fn default_pr_number() -> u64 { @@ -152,11 +154,14 @@ struct GitHubPullRequest { head: Box, base: Box, + + user: GitHubUser, } impl GitHubPullRequest { fn new(number: u64) -> Self { GitHubPullRequest { + user: User::default().into(), url: "https://test.com".to_string(), id: number + 1000, title: format!("PR #{number}"),