diff --git a/.sqlx/query-ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9.json b/.sqlx/query-7efe43b9bd54cc798a81cfe8c4a1ef396044c382b6f5172fb389175d8ce1346e.json similarity index 63% rename from .sqlx/query-ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9.json rename to .sqlx/query-7efe43b9bd54cc798a81cfe8c4a1ef396044c382b6f5172fb389175d8ce1346e.json index f36a01cc..52a48de2 100644 --- a/.sqlx/query-ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9.json +++ b/.sqlx/query-7efe43b9bd54cc798a81cfe8c4a1ef396044c382b6f5172fb389175d8ce1346e.json @@ -1,16 +1,17 @@ { "db_name": "PostgreSQL", - "query": "UPDATE pull_request SET approved_by = $1, priority = COALESCE($2, priority) WHERE id = $3", + "query": "UPDATE pull_request SET approved_by = $1, priority = COALESCE($2, priority), rollup = $3 WHERE id = $4", "describe": { "columns": [], "parameters": { "Left": [ "Text", "Int4", + "Text", "Int4" ] }, "nullable": [] }, - "hash": "ad1e4676b67caa18bd10c4d9e727950e437d538a747b18ecbf1206ef6b148dc9" + "hash": "7efe43b9bd54cc798a81cfe8c4a1ef396044c382b6f5172fb389175d8ce1346e" } diff --git a/.sqlx/query-a7498731ebc2ae95a48c52046eb9377656acd7bb2907000abcbee1c7ce90ad43.json b/.sqlx/query-a7498731ebc2ae95a48c52046eb9377656acd7bb2907000abcbee1c7ce90ad43.json new file mode 100644 index 00000000..45b6cf0b --- /dev/null +++ b/.sqlx/query-a7498731ebc2ae95a48c52046eb9377656acd7bb2907000abcbee1c7ce90ad43.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET rollup = $1 WHERE id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Int4" + ] + }, + "nullable": [] + }, + "hash": "a7498731ebc2ae95a48c52046eb9377656acd7bb2907000abcbee1c7ce90ad43" +} diff --git a/.sqlx/query-c4b0d4ed216c6b8efceb1b65ae0ad9d830e8870cfdf37542a408b99c961318f7.json b/.sqlx/query-aaceccc739564492d8424e6951f9c272c88dd4d34199c7ee4346462867029823.json similarity index 59% rename from .sqlx/query-c4b0d4ed216c6b8efceb1b65ae0ad9d830e8870cfdf37542a408b99c961318f7.json rename to .sqlx/query-aaceccc739564492d8424e6951f9c272c88dd4d34199c7ee4346462867029823.json index c326d5e0..6d82a53d 100644 --- a/.sqlx/query-c4b0d4ed216c6b8efceb1b65ae0ad9d830e8870cfdf37542a408b99c961318f7.json +++ b/.sqlx/query-aaceccc739564492d8424e6951f9c272c88dd4d34199c7ee4346462867029823.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n pr.priority,\n pr.delegated,\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 pr.rollup as \"rollup: _\",\n pr.delegated,\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": [ { @@ -30,16 +30,21 @@ }, { "ordinal": 5, + "name": "rollup: _", + "type_info": "Text" + }, + { + "ordinal": 6, "name": "delegated", "type_info": "Bool" }, { - "ordinal": 6, + "ordinal": 7, "name": "try_build: BuildModel", "type_info": "Record" }, { - "ordinal": 7, + "ordinal": 8, "name": "created_at: DateTime", "type_info": "Timestamptz" } @@ -56,10 +61,11 @@ false, true, true, + true, false, null, false ] }, - "hash": "c4b0d4ed216c6b8efceb1b65ae0ad9d830e8870cfdf37542a408b99c961318f7" + "hash": "aaceccc739564492d8424e6951f9c272c88dd4d34199c7ee4346462867029823" } diff --git a/.sqlx/query-f223617621c400f216ecfe415f0bf54a0f8cfb8aecace69b5d53cb888e5c9fa4.json b/.sqlx/query-bf90bc08fff5e3cb3c75342b097c60947a857b74ecc21320c94a816090ef04df.json similarity index 60% rename from .sqlx/query-f223617621c400f216ecfe415f0bf54a0f8cfb8aecace69b5d53cb888e5c9fa4.json rename to .sqlx/query-bf90bc08fff5e3cb3c75342b097c60947a857b74ecc21320c94a816090ef04df.json index 2e7de668..6caed5b2 100644 --- a/.sqlx/query-f223617621c400f216ecfe415f0bf54a0f8cfb8aecace69b5d53cb888e5c9fa4.json +++ b/.sqlx/query-bf90bc08fff5e3cb3c75342b097c60947a857b74ecc21320c94a816090ef04df.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n pr.priority,\n pr.delegated,\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 pr.rollup as \"rollup: _\",\n pr.delegated,\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": [ { @@ -30,16 +30,21 @@ }, { "ordinal": 5, + "name": "rollup: _", + "type_info": "Text" + }, + { + "ordinal": 6, "name": "delegated", "type_info": "Bool" }, { - "ordinal": 6, + "ordinal": 7, "name": "try_build: BuildModel", "type_info": "Record" }, { - "ordinal": 7, + "ordinal": 8, "name": "created_at: DateTime", "type_info": "Timestamptz" } @@ -55,10 +60,11 @@ false, true, true, + true, false, null, false ] }, - "hash": "f223617621c400f216ecfe415f0bf54a0f8cfb8aecace69b5d53cb888e5c9fa4" + "hash": "bf90bc08fff5e3cb3c75342b097c60947a857b74ecc21320c94a816090ef04df" } diff --git a/migrations/20250307150834_add_rollup_to_pr.down.sql b/migrations/20250307150834_add_rollup_to_pr.down.sql new file mode 100644 index 00000000..416cde49 --- /dev/null +++ b/migrations/20250307150834_add_rollup_to_pr.down.sql @@ -0,0 +1,2 @@ +-- Add up migration script here +ALTER TABLE pull_request DROP COLUMN rollup; diff --git a/migrations/20250307150834_add_rollup_to_pr.up.sql b/migrations/20250307150834_add_rollup_to_pr.up.sql new file mode 100644 index 00000000..4f9237cf --- /dev/null +++ b/migrations/20250307150834_add_rollup_to_pr.up.sql @@ -0,0 +1,2 @@ +-- Add up migration script here +ALTER TABLE pull_request ADD COLUMN rollup TEXT; diff --git a/src/bors/command/mod.rs b/src/bors/command/mod.rs index f134f520..2ecca549 100644 --- a/src/bors/command/mod.rs +++ b/src/bors/command/mod.rs @@ -1,4 +1,7 @@ mod parser; +use std::fmt; +use std::str::FromStr; + use crate::github::CommitSha; pub use parser::{CommandParseError, CommandParser}; @@ -22,6 +25,42 @@ pub enum Approver { Specified(String), } +const ROLLUP_VALUES: [&str; 4] = ["always", "iffy", "maybe", "never"]; + +#[derive(Copy, Clone, Debug, PartialEq)] +pub enum RollupMode { + Always, + Iffy, + Maybe, + Never, +} + +impl fmt::Display for RollupMode { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let s = match self { + RollupMode::Always => "always", + RollupMode::Iffy => "iffy", + RollupMode::Never => "never", + RollupMode::Maybe => "maybe", + }; + write!(f, "{}", s) + } +} + +impl FromStr for RollupMode { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "always" => Ok(RollupMode::Always), + "iffy" => Ok(RollupMode::Iffy), + "never" => Ok(RollupMode::Never), + "maybe" => Ok(RollupMode::Maybe), + _ => Err(()), + } + } +} + /// Bors command specified by a user. #[derive(Debug, PartialEq)] pub enum BorsCommand { @@ -31,6 +70,8 @@ pub enum BorsCommand { approver: Approver, /// Priority of the commit. priority: Option, + // Rollup status of the commit. + rollup: Option, }, /// Unapprove a commit. Unapprove, @@ -53,4 +94,6 @@ pub enum BorsCommand { Delegate, /// Revoke any previously granted delegation. Undelegate, + /// Rollup status. + Rollup(RollupMode), } diff --git a/src/bors/command/parser.rs b/src/bors/command/parser.rs index 32b48a74..0202ba4f 100644 --- a/src/bors/command/parser.rs +++ b/src/bors/command/parser.rs @@ -1,11 +1,12 @@ //! Defines parsers for bors commands. use std::collections::HashSet; +use std::str::FromStr; -use crate::bors::command::{Approver, BorsCommand, Parent}; +use crate::bors::command::{Approver, BorsCommand, Parent, ROLLUP_VALUES}; use crate::github::CommitSha; -use super::Priority; +use super::{Priority, RollupMode}; const PRIORITY_NAMES: [&str; 2] = ["p", "priority"]; @@ -47,6 +48,7 @@ impl CommandParser { let parsers: Vec fn(&'b str, &[CommandPart<'b>]) -> ParseResult<'b>> = vec![ parse_self_approve, parse_unapprove, + parse_rollup, parser_help, parser_ping, parser_try_cancel, @@ -75,13 +77,31 @@ impl CommandParser { Some(Err(CommandParseError::UnknownCommand(command))) } // For `@bors key=`. - CommandPart::KeyValue { key, .. } => { + CommandPart::KeyValue { key, value } => { if key == "r" { if let Some(result) = parse_approve_on_behalf(&parts) { return Some(result); } } + if key == "rollup" { + return Some({ + let tmp = RollupMode::from_str(value); + match tmp { + Ok(x) => Ok(BorsCommand::Rollup(x)), + Err(_) => { + Err(CommandParseError::ValidationError( + format!( + "rollup mode can be {}", + ROLLUP_VALUES.join("/") + ) + .to_string(), + )) + } + } + }); + } + if PRIORITY_NAMES.contains(&key) { if let Some(result) = parse_priority(&parts) { return Some(result); @@ -131,34 +151,33 @@ fn parse_parts(input: &str) -> Result, CommandParseError> { Ok(parts) } -/// Parsers - -/// Parses "@bors r+ " +/// 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 { + match parse_priority_rollup_arg(parts) { + Ok((priority, rollup)) => Some(Ok(BorsCommand::Approve { approver: Approver::Myself, priority, + rollup, })), 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 { value, .. }) = parts.first() { if value.is_empty() { return Some(Err(CommandParseError::MissingArgValue { arg: "r" })); } - - match parse_priority_arg(&parts[1..]) { - Ok(priority) => Some(Ok(BorsCommand::Approve { + match parse_priority_rollup_arg(&parts[1..]) { + Ok((priority, rollup)) => Some(Ok(BorsCommand::Approve { approver: Approver::Specified(value.to_string()), priority, + rollup, })), Err(e) => Some(Err(e)), } @@ -318,14 +337,21 @@ fn parse_priority<'a>(parts: &[CommandPart<'a>]) -> ParseResult<'a> { } } -/// Parses "p=" -fn parse_priority_arg<'a>(parts: &[CommandPart<'a>]) -> Result, CommandParseError<'a>> { +/// Parses "p= rollup=" +fn parse_priority_rollup_arg<'a>( + parts: &[CommandPart<'a>], +) -> Result<(Option, Option), CommandParseError<'a>> { let mut priority = None; + let mut rollup_status = None; for part in parts { match part { CommandPart::Bare(key) => { - return Err(CommandParseError::UnknownArg(key)); + let status = parse_rollup_bare_helper(key)?; + if rollup_status.is_some() { + return Err(CommandParseError::DuplicateArg("rollup")); + } + rollup_status = Some(status); } CommandPart::KeyValue { key, value } => { if PRIORITY_NAMES.contains(key) { @@ -335,17 +361,61 @@ fn parse_priority_arg<'a>(parts: &[CommandPart<'a>]) -> Result, Comm priority = Some(validate_priority(value)?); } + + if *key == "rollup" { + if rollup_status.is_some() { + return Err(CommandParseError::DuplicateArg("rollup")); + } + + rollup_status = Some(RollupMode::from_str(value).map_err(|_| { + CommandParseError::ValidationError( + format!("rollup mode can be {}", ROLLUP_VALUES.join("/")).to_string(), + ) + })?); + } } } } - Ok(priority) + Ok((priority, rollup_status)) +} + +fn parse_rollup_bare_helper(arg: &str) -> Result> { + if arg == "rollup" { + return Ok(RollupMode::Always); + } else if arg == "rollup-" { + return Ok(RollupMode::Maybe); + } + Err(CommandParseError::UnknownArg(arg)) +} + +/// Parses "rollup=" +fn parse_rollup<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseResult<'a> { + if command != "rollup" && command != "rollup-" { + return None; + } + + // Check for duplicate rollup arguments + if parts.iter().any(|part| match part { + CommandPart::Bare("rollup") | CommandPart::Bare("rollup-") => true, + CommandPart::KeyValue { key, .. } if *key == "rollup" => true, + _ => false, + }) { + return Some(Err(CommandParseError::DuplicateArg("rollup"))); + } + + let rollup_cmd = if command == "rollup" { + BorsCommand::Rollup(RollupMode::Always) + } else { + BorsCommand::Rollup(RollupMode::Maybe) + }; + Some(Ok(rollup_cmd)) } #[cfg(test)] mod tests { use crate::bors::command::parser::{CommandParseError, CommandParser}; - use crate::bors::command::{Approver, BorsCommand, Parent}; + use crate::bors::command::{Approver, BorsCommand, Parent, RollupMode}; use crate::github::CommitSha; #[test] @@ -392,13 +462,14 @@ mod tests { fn parse_default_approve() { let cmds = parse_commands("@bors r+"); assert_eq!(cmds.len(), 1); - assert!(matches!( + assert_eq!( cmds[0], Ok(BorsCommand::Approve { approver: Approver::Myself, priority: None, + rollup: None, }) - )); + ); } #[test] @@ -412,6 +483,7 @@ mod tests { "user1", ), priority: None, + rollup: None, }, ) "#); @@ -428,6 +500,7 @@ mod tests { "user1,user2", ), priority: None, + rollup: None, }, ) "#); @@ -441,7 +514,8 @@ mod tests { cmds[0], Ok(BorsCommand::Approve { approver: Approver::Myself, - priority: Some(1) + priority: Some(1), + rollup: None }) ) } @@ -454,7 +528,8 @@ mod tests { cmds[0], Ok(BorsCommand::Approve { approver: Approver::Specified("user1".to_string()), - priority: Some(2) + priority: Some(2), + rollup: None }) ) } @@ -472,14 +547,16 @@ mod tests { cmds[0], Ok(BorsCommand::Approve { approver: Approver::Myself, - priority: Some(1) + priority: Some(1), + rollup: None }) ); assert_eq!( cmds[1], Ok(BorsCommand::Approve { approver: Approver::Specified("user2".to_string()), - priority: Some(2) + priority: Some(2), + rollup: None }) ); } @@ -532,7 +609,8 @@ mod tests { cmds[0], Ok(BorsCommand::Approve { approver: Approver::Specified("user1".to_string()), - priority: Some(2) + priority: Some(2), + rollup: None }) ) } @@ -653,6 +731,257 @@ mod tests { assert_eq!(cmds[0], Ok(BorsCommand::SetPriority(1))); } + #[test] + fn parse_approve_with_rollup() { + let cmds = parse_commands("@bors r+ rollup"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Myself, + priority: None, + rollup: Some(RollupMode::Always) + }) + ) + } + + #[test] + fn parse_approve_on_behalf_with_rollup_value() { + let cmds = parse_commands("@bors r=user1 rollup=never"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Specified("user1".to_string()), + priority: None, + rollup: Some(RollupMode::Never) + }) + ) + } + + #[test] + fn parse_approve_on_behalf_with_rollup_bare() { + let cmds = parse_commands("@bors r=user1 rollup"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Specified("user1".to_string()), + priority: None, + rollup: Some(RollupMode::Always) + }) + ) + } + + #[test] + fn parse_approve_on_behalf_with_rollup_bare_maybe() { + let cmds = parse_commands("@bors r=user1 rollup-"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Specified("user1".to_string()), + priority: None, + rollup: Some(RollupMode::Maybe) + }) + ) + } + + #[test] + fn parse_multiple_rollup_commands() { + let cmds = parse_commands( + r#" + @bors r+ rollup + @bors r=user2 rollup=iffy + "#, + ); + assert_eq!(cmds.len(), 2); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Myself, + priority: None, + rollup: Some(RollupMode::Always) + }) + ); + assert_eq!( + cmds[1], + Ok(BorsCommand::Approve { + approver: Approver::Specified("user2".to_string()), + priority: None, + rollup: Some(RollupMode::Iffy) + }) + ); + } + + #[test] + fn parse_approve_with_invalid_rollup_status() { + let cmds = parse_commands("@bors r+ rollup=abc"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r#" + Err( + ValidationError( + "rollup mode can be always/iffy/maybe/never", + ), + ) + "#); + } + + #[test] + fn parse_approve_duplicate_rollup_args_bare_value() { + let cmds = parse_commands("@bors r+ rollup rollup=never"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("rollup")) + )); + } + + #[test] + fn parse_approve_duplicate_rollup_args_bare_bare() { + let cmds = parse_commands("@bors r+ rollup rollup-"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("rollup")) + )); + } + + #[test] + fn parse_approve_duplicate_rollup_args_value_value() { + let cmds = parse_commands("@bors r+ rollup=iffy rollup=never"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("rollup")) + )); + } + + #[test] + fn parse_approve_rollup_empty() { + let cmds = parse_commands("@bors r+ rollup="); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + MissingArgValue { + arg: "rollup", + }, + ) + "###); + } + + #[test] + fn parse_rollup_bare() { + let cmds = parse_commands("@bors rollup"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::Rollup(RollupMode::Always))); + } + + #[test] + fn parse_rollup_bare_maybe() { + let cmds = parse_commands("@bors rollup-"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::Rollup(RollupMode::Maybe))); + } + + #[test] + fn parse_priority_rollup() { + let cmds = parse_commands("@bors rollup=always"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::Rollup(RollupMode::Always))); + } + + #[test] + fn parse_rollup_empty() { + let cmds = parse_commands("@bors rollup="); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r###" + Err( + MissingArgValue { + arg: "rollup", + }, + ) + "###); + } + + #[test] + fn parse_rollup_invalid_with_int() { + let cmds = parse_commands("@bors rollup=3"); + assert_eq!(cmds.len(), 1); + insta::assert_debug_snapshot!(cmds[0], @r#" + Err( + ValidationError( + "rollup mode can be always/iffy/maybe/never", + ), + ) + "#); + } + + #[test] + fn parse_duplicate_rollup_bare_bare() { + let cmds = parse_commands("@bors rollup rollup-"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("rollup")) + )); + } + + #[test] + fn parse_duplicate_rollup_bare_value() { + let cmds = parse_commands("@bors rollup rollup=maybe"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("rollup")) + )); + } + + #[test] + fn parse_duplicate_rollup_value_value() { + let cmds = parse_commands("@bors rollup=always rollup=never"); + assert_eq!(cmds.len(), 1); + assert!(matches!( + cmds[0], + Err(CommandParseError::DuplicateArg("rollup")) + )); + } + + #[test] + fn parse_rollup_unknown_arg() { + let cmds = parse_commands("@bors rollup a"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::Rollup(RollupMode::Always))); + } + + #[test] + fn parse_approve_with_rollup_bare_priority() { + let cmds = parse_commands("@bors r+ rollup p=1"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Myself, + priority: Some(1), + rollup: Some(RollupMode::Always) + }) + ); + } + + #[test] + fn parse_approve_with_rollup_value_priority() { + let cmds = parse_commands("@bors r+ rollup=iffy p=1"); + assert_eq!(cmds.len(), 1); + assert_eq!( + cmds[0], + Ok(BorsCommand::Approve { + approver: Approver::Myself, + priority: Some(1), + rollup: Some(RollupMode::Iffy) + }) + ); + } + #[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 20f3496d..dcfee53f 100644 --- a/src/bors/handlers/help.rs +++ b/src/bors/handlers/help.rs @@ -1,4 +1,4 @@ -use crate::bors::command::{Approver, BorsCommand}; +use crate::bors::command::{Approver, BorsCommand, RollupMode}; use crate::bors::Comment; use crate::bors::RepositoryState; use crate::github::PullRequest; @@ -12,10 +12,12 @@ pub(super) async fn command_help( BorsCommand::Approve { approver: Approver::Myself, priority: None, + rollup: None, }, BorsCommand::Approve { approver: Approver::Specified("".to_string()), priority: None, + rollup: None, }, BorsCommand::Unapprove, BorsCommand::SetPriority(0), @@ -28,6 +30,7 @@ pub(super) async fn command_help( BorsCommand::TryCancel, BorsCommand::Ping, BorsCommand::Help, + BorsCommand::Rollup(RollupMode::Always), ] .into_iter() .map(|help| format!("- {}", get_command_help(help))) @@ -44,7 +47,7 @@ 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: Approver::Myself, .. } => { - "`r+ [p=]`: Approve this PR. Optionally, you can specify a ``." + "`r+ [p=] [rollup=]`: Approve this PR. Optionally, you can specify ``, ``." } BorsCommand::Approve {approver: Approver::Specified(_), ..} => { "`r= [p=]`: Approve this PR on behalf of ``. Optionally, you can specify a ``." @@ -73,6 +76,9 @@ fn get_command_help(command: BorsCommand) -> String { BorsCommand::TryCancel => { "`try cancel`: Cancel a running try build" } + BorsCommand::Rollup(_) => { + "`rollup=`: Mark the rollup status of the PR" + } }; help.to_string() } @@ -86,7 +92,7 @@ mod tests { run_test(pool, |mut tester| async { tester.post_comment("@bors help").await?; insta::assert_snapshot!(tester.get_comment().await?, @r" - - `r+ [p=]`: Approve this PR. Optionally, you can specify a ``. + - `r+ [p=] [rollup=]`: Approve this PR. Optionally, you can specify ``, ``. - `r= [p=]`: Approve this PR on behalf of ``. Optionally, you can specify a ``. - `r-`: Unapprove this PR - `p=`: Set the priority of this PR @@ -96,6 +102,7 @@ mod tests { - `try cancel`: Cancel a running try build - `ping`: Check if the bot is alive - `help`: Print this help message + - `rollup=`: Mark the rollup status of the PR "); Ok(tester) }) diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index bdb0a231..7bff46b5 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -2,7 +2,7 @@ use std::sync::Arc; use anyhow::Context; use octocrab::Octocrab; -use review::{command_delegate, command_set_priority, command_undelegate}; +use review::{command_delegate, command_set_priority, command_set_rollup, command_undelegate}; use tracing::Instrument; use crate::bors::command::{BorsCommand, CommandParseError}; @@ -208,7 +208,11 @@ async fn handle_comment( let repo = Arc::clone(&repo); let database = Arc::clone(&database); let result = match command { - BorsCommand::Approve { approver, priority } => { + BorsCommand::Approve { + approver, + priority, + rollup, + } => { let span = tracing::info_span!("Approve"); command_approve( repo, @@ -217,6 +221,7 @@ async fn handle_comment( &comment.author, &approver, priority, + rollup, ) .instrument(span) .await @@ -278,6 +283,12 @@ async fn handle_comment( .instrument(span) .await } + BorsCommand::Rollup(rollup) => { + let span = tracing::info_span!("Rollup"); + command_set_rollup(repo, database, &pull_request, &comment.author, rollup) + .instrument(span) + .await + } }; if result.is_err() { return result.context("Cannot execute Bors command"); diff --git a/src/bors/handlers/refresh.rs b/src/bors/handlers/refresh.rs index 8ba589bb..199e842d 100644 --- a/src/bors/handlers/refresh.rs +++ b/src/bors/handlers/refresh.rs @@ -92,7 +92,7 @@ fn now() -> DateTime { #[cfg(test)] thread_local! { - static MOCK_TIME: std::cell::RefCell>> = std::cell::RefCell::new(None); + static MOCK_TIME: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; } #[cfg(test)] diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 66edc1fc..a45eca86 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use crate::bors::command::Approver; +use crate::bors::command::RollupMode; use crate::bors::event::PullRequestEdited; use crate::bors::event::PullRequestPushed; use crate::bors::handlers::deny_request; @@ -25,6 +26,7 @@ pub(super) async fn command_approve( author: &GithubUser, approver: &Approver, priority: Option, + rollup: Option, ) -> anyhow::Result<()> { tracing::info!("Approving PR {}", pr.number); if !has_permission(&repo_state, author, pr, &db, PermissionType::Review).await? { @@ -40,6 +42,7 @@ pub(super) async fn command_approve( pr.number, approver.as_str(), priority, + rollup, ) .await?; handle_label_trigger(&repo_state, pr.number, LabelTrigger::Approved).await?; @@ -114,6 +117,23 @@ pub(super) async fn command_undelegate( db.undelegate(repo_state.repository(), pr.number).await } +/// Set the rollup of a pull request. +/// rollup can only be set by a user of sufficient authority. +pub(super) async fn command_set_rollup( + repo_state: Arc, + db: Arc, + pr: &PullRequest, + author: &GithubUser, + rollup: RollupMode, +) -> anyhow::Result<()> { + if !has_permission(&repo_state, author, pr, &db, PermissionType::Review).await? { + deny_request(&repo_state, pr, author, PermissionType::Review).await?; + return Ok(()); + } + db.set_rollup(repo_state.repository(), pr.number, rollup) + .await +} + pub(super) async fn handle_pull_request_edited( repo_state: Arc, db: Arc, @@ -237,7 +257,10 @@ async fn notify_of_delegation( #[cfg(test)] mod tests { use crate::{ - bors::handlers::{trybuild::TRY_MERGE_BRANCH_NAME, TRY_BRANCH_NAME}, + bors::{ + handlers::{trybuild::TRY_MERGE_BRANCH_NAME, TRY_BRANCH_NAME}, + RollupMode, + }, github::PullRequestNumber, permissions::PermissionType, tests::mocks::{ @@ -261,6 +284,13 @@ mod tests { ), ); + tester + .wait_for(|| async { + let pr = tester.get_default_pr().await?; + Ok(pr.rollup == Some(RollupMode::Maybe)) + }) + .await?; + check_pr_approved_by( &tester, default_pr_number().into(), @@ -894,4 +924,191 @@ approve = ["+approved"] }) .await; } + + #[sqlx::test] + async fn approve_with_rollup_value(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors r+ rollup=never").await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Never)); + assert!(pr.is_approved()); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_with_rollup_bare(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors r+ rollup").await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Always)); + assert!(pr.is_approved()); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_with_rollup_bare_maybe(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors r+ rollup-").await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Maybe)); + assert!(pr.is_approved()); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_with_priority_rollup(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors r+ p=10 rollup=never").await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.priority, Some(10)); + assert_eq!(pr.rollup, Some(RollupMode::Never)); + assert!(pr.is_approved()); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_on_behalf_with_rollup_bare(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors r=user1 rollup").await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Always)); + assert_eq!(pr.approved_by, Some("user1".to_string())); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_on_behalf_with_rollup_value(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors r=user1 rollup=always").await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Always)); + assert_eq!(pr.approved_by, Some("user1".to_string())); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_on_behalf_with_priority_rollup_value(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester + .post_comment("@bors r=user1 rollup=always priority=10") + .await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Always)); + assert_eq!(pr.priority, Some(10)); + assert_eq!(pr.approved_by, Some("user1".to_string())); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn approve_on_behalf_with_priority_rollup_bare(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester + .post_comment("@bors r=user1 rollup- priority=10") + .await?; + tester.expect_comments(1).await; + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Maybe)); + assert_eq!(pr.priority, Some(10)); + assert_eq!(pr.approved_by, Some("user1".to_string())); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn set_rollup_default(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors rollup").await?; + tester + .wait_for(|| async { + let pr = tester.get_default_pr().await?; + Ok(pr.rollup == Some(RollupMode::Always)) + }) + .await?; + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn set_rollup_with_value(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors rollup=maybe").await?; + tester + .wait_for(|| async { + let pr = tester.get_default_pr().await?; + Ok(pr.rollup == Some(RollupMode::Maybe)) + }) + .await?; + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn rollup_preserved_after_approve(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors rollup").await?; + tester + .wait_for(|| async { + let pr = tester.get_default_pr().await?; + Ok(pr.rollup == Some(RollupMode::Always)) + }) + .await?; + + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Always)); + assert!(pr.is_approved()); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn rollup_overridden_on_approve_with_rollup(pool: sqlx::PgPool) { + run_test(pool, |mut tester| async { + tester.post_comment("@bors rollup=never").await?; + tester + .wait_for(|| async { + let pr = tester.get_default_pr().await?; + Ok(pr.rollup == Some(RollupMode::Never)) + }) + .await?; + + tester.post_comment("@bors r+ rollup").await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert_eq!(pr.rollup, Some(RollupMode::Always)); + assert!(pr.is_approved()); + + Ok(tester) + }) + .await; + } } diff --git a/src/bors/mod.rs b/src/bors/mod.rs index 719a3bd1..9f8c2fb3 100644 --- a/src/bors/mod.rs +++ b/src/bors/mod.rs @@ -1,6 +1,7 @@ use arc_swap::ArcSwap; pub use command::CommandParser; +pub use command::RollupMode; pub use comment::Comment; pub use context::BorsContext; #[cfg(test)] diff --git a/src/database/client.rs b/src/database/client.rs index 27618d97..3d16af24 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -1,5 +1,6 @@ use sqlx::PgPool; +use crate::bors::RollupMode; use crate::database::{ BuildModel, BuildStatus, PullRequestModel, WorkflowModel, WorkflowStatus, WorkflowType, }; @@ -9,8 +10,8 @@ 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_workflows_for_build, set_pr_priority, unapprove_pull_request, undelegate_pull_request, - update_build_status, update_pr_build_id, update_workflow_status, + 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, }; use super::RunId; @@ -31,9 +32,12 @@ impl PgDbClient { pr_number: PullRequestNumber, approver: &str, priority: Option, + rollup: Option, ) -> anyhow::Result<()> { let pr = self.get_or_create_pull_request(repo, pr_number).await?; - approve_pull_request(&self.pool, pr.id, approver, priority).await + let rollup = rollup.or(pr.rollup).unwrap_or(RollupMode::Maybe); + + approve_pull_request(&self.pool, pr.id, approver, priority, rollup).await } pub async fn unapprove( @@ -73,6 +77,16 @@ impl PgDbClient { undelegate_pull_request(&self.pool, pr.id).await } + pub async fn set_rollup( + &self, + repo: &GithubRepoName, + pr_number: PullRequestNumber, + rollup: RollupMode, + ) -> anyhow::Result<()> { + let pr = self.get_or_create_pull_request(repo, pr_number).await?; + set_pr_rollup(&self.pool, pr.id, rollup).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 7cdcff27..f6262e8e 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -1,11 +1,17 @@ //! Provides access to the database. -use std::fmt::{Display, Formatter}; +use std::{ + fmt::{Display, Formatter}, + str::FromStr, +}; use chrono::{DateTime, Utc}; pub use client::PgDbClient; use sqlx::error::BoxDynError; -use crate::github::{GithubRepoName, PullRequestNumber}; +use crate::{ + bors::RollupMode, + github::{GithubRepoName, PullRequestNumber}, +}; mod client; pub(crate) mod operations; @@ -83,6 +89,29 @@ impl sqlx::Type for PullRequestNumber { } } +impl sqlx::Type for RollupMode { + fn type_info() -> sqlx::postgres::PgTypeInfo { + >::type_info() // Store as TEXT in Postgres + } +} + +impl sqlx::Encode<'_, sqlx::Postgres> for RollupMode { + fn encode_by_ref( + &self, + buf: &mut sqlx::postgres::PgArgumentBuffer, + ) -> Result { + >::encode(self.to_string(), buf) + } +} + +impl sqlx::Decode<'_, sqlx::Postgres> for RollupMode { + fn decode(value: sqlx::postgres::PgValueRef<'_>) -> Result { + let value = >::decode(value)?; + + Self::from_str(&value).map_err(|_| panic!("Invalid RollupMode value: {}", value)) + } +} + /// Status of a GitHub build. #[derive(Debug, PartialEq, sqlx::Type)] #[sqlx(type_name = "TEXT")] @@ -122,6 +151,7 @@ pub struct PullRequestModel { pub approved_by: Option, pub delegated: bool, pub priority: Option, + pub rollup: Option, pub try_build: Option, pub created_at: DateTime, } diff --git a/src/database/operations.rs b/src/database/operations.rs index 54b76776..3f1fbb20 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -2,6 +2,7 @@ use chrono::DateTime; use chrono::Utc; use sqlx::postgres::PgExecutor; +use crate::bors::RollupMode; use crate::database::BuildStatus; use crate::database::WorkflowModel; use crate::github::CommitSha; @@ -28,6 +29,7 @@ SELECT pr.number, pr.approved_by, pr.priority, + pr.rollup as "rollup: _", pr.delegated, CASE WHEN pr.build_id IS NULL THEN NULL @@ -52,7 +54,7 @@ WHERE pr.repository = $1 ) .fetch_optional(executor) .await?; - Ok(pull_request.map(Into::into)) + Ok(pull_request) } pub(crate) async fn create_pull_request( @@ -75,13 +77,15 @@ pub(crate) async fn approve_pull_request( pr_id: i32, approver: &str, priority: Option, + rollup: RollupMode, ) -> anyhow::Result<()> { let priority_i32 = priority.map(|p| p as i32); sqlx::query!( - "UPDATE pull_request SET approved_by = $1, priority = COALESCE($2, priority) WHERE id = $3", + "UPDATE pull_request SET approved_by = $1, priority = COALESCE($2, priority), rollup = $3 WHERE id = $4", approver, priority_i32, + rollup.to_string(), pr_id, ) .execute(executor) @@ -141,6 +145,7 @@ SELECT pr.number, pr.approved_by, pr.priority, + pr.rollup as "rollup: _", pr.delegated, CASE WHEN pr.build_id IS NULL THEN NULL @@ -334,6 +339,21 @@ pub(crate) async fn set_pr_priority( Ok(()) } +pub(crate) async fn set_pr_rollup( + executor: impl PgExecutor<'_>, + pr_id: i32, + rollup: RollupMode, +) -> anyhow::Result<()> { + sqlx::query!( + "UPDATE pull_request SET rollup = $1 WHERE id = $2", + rollup.to_string(), + pr_id, + ) + .execute(executor) + .await?; + Ok(()) +} + pub(crate) async fn get_workflows_for_build( executor: impl PgExecutor<'_>, build_id: i32, diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs index a736b61f..27d70acc 100644 --- a/src/tests/mocks/bors.rs +++ b/src/tests/mocks/bors.rs @@ -210,7 +210,7 @@ impl BorsTester { for i in 0..count { self.get_comment() .await - .expect(&format!("Failed to get comment #{i}")); + .unwrap_or_else(|_| panic!("Failed to get comment #{i}")); } } diff --git a/src/tests/mocks/pull_request.rs b/src/tests/mocks/pull_request.rs index 876710dd..c8ef24b4 100644 --- a/src/tests/mocks/pull_request.rs +++ b/src/tests/mocks/pull_request.rs @@ -34,7 +34,7 @@ pub async fn mock_pull_requests( Mock::given(method("GET")) .and(path(format!("/repos/{repo_name}/pulls/{pr_number}"))) .respond_with(move |_: &Request| { - let pull_request_error = repo_clone.lock().pull_request_error.clone(); + let pull_request_error = repo_clone.lock().pull_request_error; if pull_request_error { ResponseTemplate::new(500) } else {