From cd516a404818605d202dd8cbc98704c948902f9c Mon Sep 17 00:00:00 2001 From: Sakibul Islam Date: Sun, 2 Mar 2025 23:48:28 +0000 Subject: [PATCH] feat: add priority --- ...6112a153713a71af8d62917e2ff9873ecec1.json} | 12 +- ...b19d263ad3a242cbf60841d1284da702b78ff.json | 15 + ...edd2173ab301e8a06c740b587738b140795a.json} | 12 +- ...8871f29d60b8617caf94dfa439f9ef8453caa.json | 15 - ...7950e437d538a747b18ecbf1206ef6b148dc9.json | 16 + docs/commands.md | 1 + ...20250227095730_add_priority_to_pr.down.sql | 2 + .../20250227095730_add_priority_to_pr.up.sql | 2 + src/bors/command/mod.rs | 14 +- src/bors/command/parser.rs | 361 ++++++++++++++++-- src/bors/handlers/help.rs | 27 +- src/bors/handlers/mod.rs | 28 +- src/bors/handlers/review.rs | 170 +++++++-- src/bors/handlers/trybuild.rs | 10 +- src/database/client.rs | 16 +- src/database/mod.rs | 1 + src/database/operations.rs | 25 +- src/permissions.rs | 10 + src/tests/mocks/bors.rs | 11 + 19 files changed, 643 insertions(+), 105 deletions(-) rename .sqlx/{query-6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128.json => query-491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1.json} (56%) create mode 100644 .sqlx/query-667daf4042d6129c923d1f8f6b1b19d263ad3a242cbf60841d1284da702b78ff.json rename .sqlx/{query-9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d.json => query-67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a.json} (56%) delete mode 100644 .sqlx/query-7a9718cb973d09f68a6854a4fb08871f29d60b8617caf94dfa439f9ef8453caa.json create mode 100644 .sqlx/query-ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9.json create mode 100644 migrations/20250227095730_add_priority_to_pr.down.sql create mode 100644 migrations/20250227095730_add_priority_to_pr.up.sql diff --git a/.sqlx/query-6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128.json b/.sqlx/query-491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1.json similarity index 56% rename from .sqlx/query-6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128.json rename to .sqlx/query-491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1.json index e1b5b93a..86c22dfb 100644 --- a/.sqlx/query-6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128.json +++ b/.sqlx/query-491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "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", + "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n pr.priority,\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": [ { @@ -25,11 +25,16 @@ }, { "ordinal": 4, + "name": "priority", + "type_info": "Int4" + }, + { + "ordinal": 5, "name": "try_build: BuildModel", "type_info": "Record" }, { - "ordinal": 5, + "ordinal": 6, "name": "created_at: DateTime", "type_info": "Timestamptz" } @@ -45,9 +50,10 @@ false, false, true, + true, null, false ] }, - "hash": "6410d5ac20c9b301fc7b9e639eabc7b82a2a1d01cde48ba01fdc5cd8ce2ce128" + "hash": "491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1" } diff --git a/.sqlx/query-667daf4042d6129c923d1f8f6b1b19d263ad3a242cbf60841d1284da702b78ff.json b/.sqlx/query-667daf4042d6129c923d1f8f6b1b19d263ad3a242cbf60841d1284da702b78ff.json new file mode 100644 index 00000000..d0196bcf --- /dev/null +++ b/.sqlx/query-667daf4042d6129c923d1f8f6b1b19d263ad3a242cbf60841d1284da702b78ff.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET priority = $1 WHERE id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4", + "Int4" + ] + }, + "nullable": [] + }, + "hash": "667daf4042d6129c923d1f8f6b1b19d263ad3a242cbf60841d1284da702b78ff" +} diff --git a/.sqlx/query-9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d.json b/.sqlx/query-67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a.json similarity index 56% rename from .sqlx/query-9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d.json rename to .sqlx/query-67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a.json index 11289097..5cbf4626 100644 --- a/.sqlx/query-9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d.json +++ b/.sqlx/query-67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "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", + "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n pr.priority,\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": [ { @@ -25,11 +25,16 @@ }, { "ordinal": 4, + "name": "priority", + "type_info": "Int4" + }, + { + "ordinal": 5, "name": "try_build: BuildModel", "type_info": "Record" }, { - "ordinal": 5, + "ordinal": 6, "name": "created_at: DateTime", "type_info": "Timestamptz" } @@ -44,9 +49,10 @@ false, false, true, + true, null, false ] }, - "hash": "9531efa35aabadfb04c06f23375eafe1c8f10444e706940fa0f20a50b824044d" + "hash": "67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a" } diff --git a/.sqlx/query-7a9718cb973d09f68a6854a4fb08871f29d60b8617caf94dfa439f9ef8453caa.json b/.sqlx/query-7a9718cb973d09f68a6854a4fb08871f29d60b8617caf94dfa439f9ef8453caa.json deleted file mode 100644 index 3a40775e..00000000 --- a/.sqlx/query-7a9718cb973d09f68a6854a4fb08871f29d60b8617caf94dfa439f9ef8453caa.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "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-ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9.json b/.sqlx/query-ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9.json new file mode 100644 index 00000000..f36a01cc --- /dev/null +++ b/.sqlx/query-ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET approved_by = $1, priority = COALESCE($2, priority) WHERE id = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Int4", + "Int4" + ] + }, + "nullable": [] + }, + "hash": "ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9" +} diff --git a/docs/commands.md b/docs/commands.md index 318ca2e0..a9987ed7 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -8,3 +8,4 @@ which is by default set to `@bors`. | `try` | `try` | Start a try build based on the most recent commit from the main branch. | | `try parent=` | `try` | Start a try build based on the specified parent commit `sha`. | | `try cancel` | `try` | Cancel a running try build. | +| `p=` | `try` | Set the priority of a PR. | diff --git a/migrations/20250227095730_add_priority_to_pr.down.sql b/migrations/20250227095730_add_priority_to_pr.down.sql new file mode 100644 index 00000000..4e336787 --- /dev/null +++ b/migrations/20250227095730_add_priority_to_pr.down.sql @@ -0,0 +1,2 @@ +-- Add down migration script here +ALTER TABLE pull_request DROP COLUMN priority; diff --git a/migrations/20250227095730_add_priority_to_pr.up.sql b/migrations/20250227095730_add_priority_to_pr.up.sql new file mode 100644 index 00000000..b385fc4f --- /dev/null +++ b/migrations/20250227095730_add_priority_to_pr.up.sql @@ -0,0 +1,2 @@ +-- Add up migration script here +ALTER TABLE pull_request ADD COLUMN priority INT; diff --git a/src/bors/command/mod.rs b/src/bors/command/mod.rs index 37f30fc3..1155125e 100644 --- a/src/bors/command/mod.rs +++ b/src/bors/command/mod.rs @@ -2,6 +2,9 @@ mod parser; use crate::github::CommitSha; pub use parser::{CommandParseError, CommandParser}; +/// Priority of a commit. +pub type Priority = u32; + /// Type of parent allowed in a try build #[derive(Clone, Debug, PartialEq)] pub enum Parent { @@ -23,10 +26,15 @@ pub enum Approver { #[derive(Debug, PartialEq)] pub enum BorsCommand { /// Approve a commit. - Approve(Approver), + Approve { + /// Who is approving the commit. + approver: Approver, + /// Priority of the commit. + priority: Option, + }, /// Unapprove a commit. Unapprove, - /// Print help + /// Print help. Help, /// Ping the bot. Ping, @@ -39,4 +47,6 @@ pub enum BorsCommand { }, /// Cancel a try build. TryCancel, + /// Set the priority of a commit. + SetPriority(Priority), } diff --git a/src/bors/command/parser.rs b/src/bors/command/parser.rs index e922f46d..235ffe51 100644 --- a/src/bors/command/parser.rs +++ b/src/bors/command/parser.rs @@ -5,6 +5,10 @@ use std::collections::HashSet; use crate::bors::command::{Approver, BorsCommand, Parent}; use crate::github::CommitSha; +use super::Priority; + +const PRIORITY_NAMES: [&str; 2] = ["p", "priority"]; + #[derive(Debug, PartialEq)] pub enum CommandParseError<'a> { MissingCommand, @@ -68,11 +72,20 @@ 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); + // For `@bors key=`. + CommandPart::KeyValue { key, .. } => { + if key == "r" { + if let Some(result) = parse_approve_on_behalf(&parts) { + return Some(result); + } } + + if PRIORITY_NAMES.contains(&key) { + if let Some(result) = parse_priority(&parts) { + return Some(result); + } + } + Some(Err(CommandParseError::MissingCommand)) } } @@ -118,26 +131,33 @@ 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_self_approve<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseResult<'a> { + if command != "r+" { + return None; + } + + match parse_priority_arg(parts) { + Ok(priority) => Some(Ok(BorsCommand::Approve { + approver: Approver::Myself, + priority, + })), + Err(e) => Some(Err(e)), } } -/// Parses "@bors r=". +/// 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() { + if let Some(CommandPart::KeyValue { value, .. }) = parts.first() { + if value.is_empty() { return Some(Err(CommandParseError::MissingArgValue { arg: "r" })); - } else { - Some(Ok(BorsCommand::Approve(Approver::Specified( - value.to_string(), - )))) + } + match parse_priority_arg(&parts[1..]) { + Ok(priority) => Some(Ok(BorsCommand::Approve { + approver: Approver::Specified(value.to_string()), + priority, + })), + Err(e) => Some(Err(e)), } } else { Some(Err(CommandParseError::MissingArgValue { arg: "r" })) @@ -178,6 +198,15 @@ fn parse_sha(input: &str) -> Result { Ok(CommitSha(input.to_string())) } +fn validate_priority(value: &str) -> Result { + match value.parse::() { + Ok(p) => Ok(p), + Err(_) => Err(CommandParseError::ValidationError( + "Priority must be a non-negative integer".to_string(), + )), + } +} + /// Parses "@bors try ". fn parser_try<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseResult<'a> { if command != "try" { @@ -241,6 +270,57 @@ fn parser_try_cancel<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseRe } } +/// Parses "@bors p=" +fn parse_priority<'a>(parts: &[CommandPart<'a>]) -> ParseResult<'a> { + // If we have more than one part, check for duplicate priority arguments + if parts.len() > 1 { + for part in &parts[1..] { + if let CommandPart::KeyValue { key, .. } = part { + if PRIORITY_NAMES.contains(key) { + return Some(Err(CommandParseError::DuplicateArg(key))); + } + } + } + } + + if let Some(CommandPart::KeyValue { value, .. }) = parts.first() { + if value.is_empty() { + return Some(Err(CommandParseError::MissingArgValue { arg: "p" })); + } + + match validate_priority(value) { + Ok(p) => Some(Ok(BorsCommand::SetPriority(p))), + Err(e) => Some(Err(e)), + } + } else { + Some(Err(CommandParseError::MissingArgValue { arg: "p" })) + } +} + +/// Parses "p=" +fn parse_priority_arg<'a>(parts: &[CommandPart<'a>]) -> Result, CommandParseError<'a>> { + let mut priority = None; + + for part in parts { + match part { + CommandPart::Bare(key) => { + return Err(CommandParseError::UnknownArg(key)); + } + CommandPart::KeyValue { key, value } => { + if PRIORITY_NAMES.contains(key) { + if priority.is_some() { + return Err(CommandParseError::DuplicateArg(key)); + } + + priority = Some(validate_priority(value)?); + } + } + } + } + + Ok(priority) +} + #[cfg(test)] mod tests { use crate::bors::command::parser::{CommandParseError, CommandParser}; @@ -293,7 +373,10 @@ mod tests { assert_eq!(cmds.len(), 1); assert!(matches!( cmds[0], - Ok(BorsCommand::Approve(Approver::Myself)) + Ok(BorsCommand::Approve { + approver: Approver::Myself, + priority: None, + }) )); } @@ -301,32 +384,254 @@ mod tests { fn parse_approve_on_behalf() { let cmds = parse_commands("@bors r=user1"); assert_eq!(cmds.len(), 1); - insta::assert_debug_snapshot!(cmds[0], @r###" + insta::assert_debug_snapshot!(cmds[0], @r#" Ok( - Approve( - Specified( + Approve { + approver: Specified( "user1", ), - ), + priority: None, + }, ) - "###); + "#); } #[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###" + insta::assert_debug_snapshot!(cmds[0], @r#" Ok( - Approve( - Specified( + Approve { + approver: Specified( "user1,user2", ), + priority: None, + }, + ) + "#); + } + + #[test] + fn parse_approve_with_priority() { + let cmds = parse_commands("@bors r+ p=1"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Myself, + priority: Some(1) + }) + ) + } + + #[test] + fn parse_approve_on_behalf_with_priority() { + let cmds = parse_commands("@bors r=user1 p=2"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Specified("user1".to_string()), + priority: Some(2) + }) + ) + } + + #[test] + fn parse_multiple_priority_commands() { + let cmds = parse_commands( + r#" + @bors r+ p=1 + @bors r=user2 p=2 + "#, + ); + assert_eq!(cmds.len(), 2); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Myself, + priority: Some(1) + }) + ); + assert_eq!( + cmds[1], + Ok(BorsCommand::Approve { + approver: Approver::Specified("user2".to_string()), + priority: Some(2) + }) + ); + } + + #[test] + fn parse_approve_negative_priority_invalid() { + let cmds = parse_commands("@bors r+ p=-1"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + ValidationError( + "Priority must be a non-negative integer", ), ) "###); } + #[test] + fn parse_approve_duplicate_priority_alias() { + let cmds = parse_commands("@bors r+ p=1 priority=2"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("priority")) + )); + } + + #[test] + fn parse_approve_duplicate_priority_args() { + let cmds = parse_commands("@bors r+ p=1 p=2"); + assert_eq!(cmds.len(), 1); + assert!(matches!(cmds[0], Err(CommandParseError::DuplicateArg("p")))); + } + + #[test] + fn parse_approve_duplicate_priority_alias_args() { + let cmds = parse_commands("@bors r+ priority=1 priority=2"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("priority")) + )); + } + + #[test] + fn parse_approve_on_behalf_with_priority_alias() { + let cmds = parse_commands("@bors r=user1 priority=2"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Specified("user1".to_string()), + priority: Some(2) + }) + ) + } + + #[test] + fn parse_approve_priority_invalid() { + let cmds = parse_commands("@bors r+ p=abc"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + ValidationError( + "Priority must be a non-negative integer", + ), + ) + "###); + } + + #[test] + fn parse_approve_priority_empty() { + let cmds = parse_commands("@bors r+ p="); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + MissingArgValue { + arg: "p", + }, + ) + "###); + } + + #[test] + fn parse_priority_exceeds_max_u32() { + let cmds = parse_commands("@bors p=4294967296"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + ValidationError( + "Priority must be a non-negative integer", + ), + ) + "###); + } + + #[test] + fn parse_priority() { + let cmds = parse_commands("@bors p=5"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::SetPriority(5))); + } + + #[test] + fn parse_priority_alias() { + let cmds = parse_commands("@bors priority=5"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::SetPriority(5))); + } + + #[test] + fn parse_priority_empty() { + let cmds = parse_commands("@bors p="); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + MissingArgValue { + arg: "p", + }, + ) + "###); + } + + #[test] + fn parse_priority_invalid() { + let cmds = parse_commands("@bors p=abc"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + ValidationError( + "Priority must be a non-negative integer", + ), + ) + "###); + } + + #[test] + fn parse_priority_negative() { + let cmds = parse_commands("@bors p=-1"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + ValidationError( + "Priority must be a non-negative integer", + ), + ) + "###); + } + + #[test] + fn parse_duplicate_priority() { + let cmds = parse_commands("@bors p=1 p=2"); + assert_eq!(cmds.len(), 1); + assert!(matches!(cmds[0], Err(CommandParseError::DuplicateArg("p")))); + } + + #[test] + fn parse_duplicate_alias_priority() { + let cmds = parse_commands("@bors p=1 priority=2"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("priority")) + )); + } + + #[test] + fn parse_priority_unknown_arg() { + let cmds = parse_commands("@bors p=1 a"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::SetPriority(1))); + } + #[test] fn parse_unapprove() { let cmds = parse_commands("@bors r-"); diff --git a/src/bors/handlers/help.rs b/src/bors/handlers/help.rs index d3300300..302f8d25 100644 --- a/src/bors/handlers/help.rs +++ b/src/bors/handlers/help.rs @@ -9,9 +9,16 @@ pub(super) async fn command_help( pr: &PullRequest, ) -> anyhow::Result<()> { let help = [ - BorsCommand::Approve(Approver::Myself), - BorsCommand::Approve(Approver::Specified("".to_string())), + BorsCommand::Approve { + approver: Approver::Myself, + priority: None, + }, + BorsCommand::Approve { + approver: Approver::Specified("".to_string()), + priority: None, + }, BorsCommand::Unapprove, + BorsCommand::SetPriority(0), BorsCommand::Try { parent: None, jobs: vec![], @@ -34,15 +41,18 @@ pub(super) async fn command_help( fn get_command_help(command: BorsCommand) -> String { // !!! When modifying this match, also update the command list above (in [`command_help`]) !!! let help = match command { - BorsCommand::Approve(Approver::Myself) => { - "`r+`: Approve this PR" + BorsCommand::Approve { approver: Approver::Myself, .. } => { + "`r+ [p=]`: Approve this PR. Optionally, you can specify a ``." } - BorsCommand::Approve(Approver::Specified(_)) => { - "`r=`: Approve this PR on behalf of ``" + BorsCommand::Approve {approver: Approver::Specified(_), ..} => { + "`r= [p=]`: Approve this PR on behalf of ``. Optionally, you can specify a ``." } BorsCommand::Unapprove => { "`r-`: Unapprove this PR" } + BorsCommand::SetPriority(_) => { + "`p=`: Set the priority of this PR" + } BorsCommand::Help => { "`help`: Print this help message" } @@ -68,9 +78,10 @@ mod tests { run_test(pool, |mut tester| async { tester.post_comment("@bors help").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - - `r+`: Approve this PR - - `r=`: Approve this PR on behalf of `` + - `r+ [p=]`: Approve this PR. Optionally, you can specify a ``. + - `r= [p=]`: Approve this PR on behalf of ``. Optionally, you can specify a ``. - `r-`: Unapprove this PR + - `p=`: Set the priority of this PR - `try [parent=] [jobs=]`: Start a try build. Optionally, you can specify a `` SHA or a list of `` to run - `try cancel`: Cancel a running try build - `ping`: Check if the bot is alive diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 7a9f77eb..045ddea8 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use anyhow::Context; use octocrab::Octocrab; +use review::command_set_priority; use tracing::Instrument; use crate::bors::command::{BorsCommand, CommandParseError}; @@ -205,11 +206,18 @@ async fn handle_comment( let repo = Arc::clone(&repo); let database = Arc::clone(&database); let result = match command { - BorsCommand::Approve(approver) => { + BorsCommand::Approve { approver, priority } => { let span = tracing::info_span!("Approve"); - command_approve(repo, database, &pull_request, &comment.author, &approver) - .instrument(span) - .await + command_approve( + repo, + database, + &pull_request, + &comment.author, + &approver, + priority, + ) + .instrument(span) + .await } BorsCommand::Unapprove => { let span = tracing::info_span!("Unapprove"); @@ -217,6 +225,18 @@ async fn handle_comment( .instrument(span) .await } + BorsCommand::SetPriority(priority) => { + let span = tracing::info_span!("Priority"); + command_set_priority( + repo, + database, + &pull_request, + &comment.author, + priority, + ) + .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 index 36dbd6eb..7280a90b 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -22,18 +22,24 @@ pub(super) async fn command_approve( pr: &PullRequest, author: &GithubUser, approver: &Approver, + priority: Option, ) -> anyhow::Result<()> { tracing::info!("Approving PR {}", pr.number); if !sufficient_approve_permission(repo_state.clone(), author) { - deny_approve_request(&repo_state, pr, author).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(), }; - db.approve(repo_state.repository(), pr.number, approver.as_str()) - .await?; + db.approve( + repo_state.repository(), + pr.number, + approver.as_str(), + priority, + ) + .await?; handle_label_trigger(&repo_state, pr.number, LabelTrigger::Approved).await?; notify_of_approval(&repo_state, pr, approver.as_str()).await } @@ -48,7 +54,7 @@ pub(super) async fn command_unapprove( ) -> 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?; + deny_request(&repo_state, pr, author, PermissionType::Review).await?; return Ok(()); }; db.unapprove(repo_state.repository(), pr.number).await?; @@ -56,6 +62,23 @@ pub(super) async fn command_unapprove( notify_of_unapproval(&repo_state, pr).await } +/// Set the priority of a pull request. +/// Priority can only be set by a user of sufficient authority. +pub(super) async fn command_set_priority( + repo_state: Arc, + db: Arc, + pr: &PullRequest, + author: &GithubUser, + priority: u32, +) -> anyhow::Result<()> { + if !sufficient_priority_permission(repo_state.clone(), author) { + deny_request(&repo_state, pr, author, PermissionType::Review).await?; + return Ok(()); + }; + db.set_priority(repo_state.repository(), pr.number, priority) + .await +} + pub(super) async fn handle_pull_request_edited( repo_state: Arc, db: Arc, @@ -105,24 +128,10 @@ fn sufficient_approve_permission(repo: Arc, author: &GithubUser .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 +fn sufficient_priority_permission(repo: Arc, author: &GithubUser) -> bool { + repo.permissions + .load() + .has_permission(author.id, PermissionType::Review) } async fn notify_of_approval( @@ -153,21 +162,22 @@ fn sufficient_unapprove_permission( .has_permission(author.id, PermissionType::Review) } -async fn deny_unapprove_request( +async fn deny_request( repo: &RepositoryState, pr: &PullRequest, author: &GithubUser, + permission_type: PermissionType, ) -> anyhow::Result<()> { tracing::warn!( - "Permission denied for unapprove command by {}", + "Permission denied for request command by {}", author.username ); repo.client .post_comment( pr.number, Comment::new(format!( - "@{}: :key: Insufficient privileges: not in review users", - author.username + "@{}: :key: Insufficient privileges: not in {} users", + author.username, permission_type )), ) .await @@ -191,7 +201,7 @@ async fn notify_of_edited_pr( .post_comment( pr_number, Comment::new(format!( - r#":warning: The base branch changed to `{base_name}`, and the + r#":warning: The base branch changed to `{base_name}`, and the PR will need to be re-approved."#, )), ) @@ -207,7 +217,7 @@ async fn notify_of_pushed_pr( .post_comment( pr_number, Comment::new(format!( - r#":warning: A new commit `{}` was pushed to the branch, the + r#":warning: A new commit `{}` was pushed to the branch, the PR will need to be re-approved."#, head_sha )), @@ -292,6 +302,24 @@ mod tests { .await; } + #[sqlx::test] + async fn insufficient_permission_set_priority(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 p=2").await?; + assert_eq!( + tester.get_comment().await?, + "@default-user: :key: Insufficient privileges: not in review users" + ); + Ok(tester) + }) + .await; + } + #[sqlx::test] async fn unapprove(pool: sqlx::PgPool) { BorsBuilder::new(pool) @@ -348,7 +376,7 @@ mod tests { assert_eq!( tester.get_comment().await?, - r#":warning: The base branch changed to `main`, and the + r#":warning: The base branch changed to `main`, and the PR will need to be re-approved."#, ); check_pr_unapproved(&tester, default_pr_number().into()).await; @@ -428,7 +456,7 @@ PR will need to be re-approved."#, assert_eq!( tester.get_comment().await?, format!( - r#":warning: A new commit `pr-{}-sha` was pushed to the branch, the + r#":warning: A new commit `pr-{}-sha` was pushed to the branch, the PR will need to be re-approved."#, default_pr_number() ) @@ -488,4 +516,86 @@ approve = ["+approved"] let pr = repo.lock().get_pr(default_pr_number()).clone(); pr.check_removed_labels(&["approved"]); } + + #[sqlx::test] + async fn approve_with_priority(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors r+ p=10").await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.priority, Some(10)); + assert!(pr.is_approved()); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_on_behalf_with_priority(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors r=user1 p=10").await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.priority, Some(10)); + assert_eq!(pr.approved_by, Some("user1".to_string())); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn set_priority(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors p=5").await?; + // Wait for db update. + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + + assert_eq!(pr.priority, Some(5)); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn priority_preserved_after_approve(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors p=5").await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert_eq!(pr.priority, Some(5)); + + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert_eq!(pr.priority, Some(5)); + assert!(pr.is_approved()); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn priority_overridden_on_approve_with_priority(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors p=5").await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert_eq!(pr.priority, Some(5)); + + tester.post_comment("@bors r+ p=10").await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert_eq!(pr.priority, Some(10)); + assert!(pr.is_approved()); + + Ok(tester) + }) + .await; + } } diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index a34abdeb..fb15cd3a 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -354,7 +354,7 @@ mod tests { use crate::bors::handlers::trybuild::{TRY_BRANCH_NAME, TRY_MERGE_BRANCH_NAME}; use crate::database::operations::get_all_workflows; use crate::database::BuildStatus; - use crate::github::{CommitSha, PullRequestNumber}; + use crate::github::CommitSha; use crate::tests::mocks::{ default_pr_number, default_repo_name, run_test, BorsBuilder, Permissions, Workflow, WorkflowEvent, World, @@ -674,13 +674,7 @@ mod tests { tester.expect_comments(1).await; tester.post_comment("@bors try cancel").await?; tester.expect_comments(1).await; - let pr = tester - .db() - .get_or_create_pull_request( - &default_repo_name(), - PullRequestNumber(default_pr_number()), - ) - .await?; + let pr = tester.get_default_pr().await?; assert_eq!(pr.try_build.unwrap().status, BuildStatus::Cancelled); Ok(tester) }) diff --git a/src/database/client.rs b/src/database/client.rs index 184e32ca..16691e1d 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -9,7 +9,8 @@ use crate::github::{CommitSha, GithubRepoName}; use super::operations::{ 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, + set_pr_priority, unapprove_pull_request, update_build_status, update_pr_build_id, + update_workflow_status, }; use super::RunId; @@ -29,9 +30,10 @@ impl PgDbClient { repo: &GithubRepoName, pr_number: PullRequestNumber, approver: &str, + priority: Option, ) -> anyhow::Result<()> { let pr = self.get_or_create_pull_request(repo, pr_number).await?; - approve_pull_request(&self.pool, pr.id, approver).await + approve_pull_request(&self.pool, pr.id, approver, priority).await } pub async fn unapprove( @@ -43,6 +45,16 @@ impl PgDbClient { unapprove_pull_request(&self.pool, pr.id).await } + pub async fn set_priority( + &self, + repo: &GithubRepoName, + pr_number: PullRequestNumber, + priority: u32, + ) -> anyhow::Result<()> { + let pr = self.get_or_create_pull_request(repo, pr_number).await?; + set_pr_priority(&self.pool, pr.id, priority).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 a26706f8..a8810a84 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -120,6 +120,7 @@ pub struct PullRequestModel { pub repository: GithubRepoName, pub number: PullRequestNumber, pub approved_by: Option, + pub priority: Option, pub try_build: Option, pub created_at: DateTime, } diff --git a/src/database/operations.rs b/src/database/operations.rs index f6a990fb..23c42599 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -27,6 +27,7 @@ SELECT pr.repository, pr.number, pr.approved_by, + pr.priority, CASE WHEN pr.build_id IS NULL THEN NULL ELSE ( @@ -72,11 +73,15 @@ pub(crate) async fn approve_pull_request( executor: impl PgExecutor<'_>, pr_id: i32, approver: &str, + priority: Option, ) -> anyhow::Result<()> { + let priority_i32 = priority.map(|p| p as i32); + sqlx::query!( - "UPDATE pull_request SET approved_by = $1 WHERE id = $2", + "UPDATE pull_request SET approved_by = $1, priority = COALESCE($2, priority) WHERE id = $3", approver, - pr_id + priority_i32, + pr_id, ) .execute(executor) .await?; @@ -108,6 +113,7 @@ SELECT pr.repository, pr.number, pr.approved_by, + pr.priority, CASE WHEN pr.build_id IS NULL THEN NULL ELSE ( @@ -285,6 +291,21 @@ pub(crate) async fn update_workflow_status( Ok(()) } +pub(crate) async fn set_pr_priority( + executor: impl PgExecutor<'_>, + pr_id: i32, + priority: u32, +) -> anyhow::Result<()> { + sqlx::query!( + "UPDATE pull_request SET priority = $1 WHERE id = $2", + priority as i32, + pr_id, + ) + .execute(executor) + .await?; + Ok(()) +} + pub(crate) async fn get_workflows_for_build( executor: impl PgExecutor<'_>, build_id: i32, diff --git a/src/permissions.rs b/src/permissions.rs index 4c7022fa..0636fcfb 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -1,6 +1,7 @@ use octocrab::models::UserId; use serde::{Deserialize, Serialize}; use std::collections::HashSet; +use std::fmt; use crate::github::GithubRepoName; @@ -12,6 +13,15 @@ pub enum PermissionType { Try, } +impl fmt::Display for PermissionType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + PermissionType::Review => write!(f, "review"), + PermissionType::Try => write!(f, "try"), + } + } +} + pub struct UserPermissions { review_users: HashSet, try_users: HashSet, diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs index a0f8ad5b..7946cd93 100644 --- a/src/tests/mocks/bors.rs +++ b/src/tests/mocks/bors.rs @@ -13,7 +13,9 @@ use tokio::task::JoinHandle; use tower::Service; use crate::bors::WAIT_FOR_REFRESH; +use crate::database::PullRequestModel; use crate::github::api::load_repositories; +use crate::github::PullRequestNumber; use crate::tests::mocks::comment::{Comment, GitHubIssueCommentEventPayload}; use crate::tests::mocks::workflow::{ CheckSuite, GitHubCheckRunEventPayload, GitHubCheckSuiteEventPayload, @@ -141,6 +143,15 @@ impl BorsTester { self.world.get_repo(default_repo_name()) } + pub async fn get_default_pr(&self) -> anyhow::Result { + self.db() + .get_or_create_pull_request( + &default_repo_name(), + PullRequestNumber(default_pr_number()), + ) + .await + } + pub async fn refresh(&self) { self.global_tx.send(BorsGlobalEvent::Refresh).await.unwrap(); // Wait until the refresh is fully handled