Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add option to wait for ci on approval #202

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions migrations/20250227095730_add_priority_to_pr.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- Add down migration script here
ALTER TABLE pull_request DROP COLUMN priority;
2 changes: 2 additions & 0 deletions migrations/20250227095730_add_priority_to_pr.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- Add up migration script here
ALTER TABLE pull_request ADD COLUMN priority INT;
9 changes: 7 additions & 2 deletions src/bors/command/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,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<u32>,
},
/// Unapprove a commit.
Unapprove,
/// Print help
/// Print help.
Help,
/// Ping the bot.
Ping,
Expand Down
128 changes: 109 additions & 19 deletions src/bors/command/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,26 +118,36 @@ fn parse_parts(input: &str) -> Result<Vec<CommandPart>, 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+ <p=priority>"
fn parse_self_approve<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseResult<'a> {
if command != "r+" {
return None;
}

match parse_priority(parts) {
Ok(priority) => Some(Ok(BorsCommand::Approve {
approver: Approver::Myself,
priority,
})),
Err(e) => Some(Err(e)),
}
}

/// Parses "@bors r=<username>".
/// Parses "@bors r=<username> <p=priority>".
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(),
))))
match parse_priority(parts) {
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" }))
Expand Down Expand Up @@ -241,6 +251,29 @@ fn parser_try_cancel<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseRe
}
}

/// Parses "p=<priority>"
fn parse_priority<'a>(parts: &[CommandPart<'a>]) -> Result<Option<u32>, CommandParseError<'a>> {
for part in parts {
match part {
CommandPart::Bare(key) => {
return Err(CommandParseError::UnknownArg(key));
}
CommandPart::KeyValue { key, value } => {
if *key == "p" || *key == "priority" {
return match value.parse::<u32>() {
Ok(p) => Ok(Some(p)),
Err(_) => Err(CommandParseError::ValidationError(
"Priority must be a valid number".to_string(),
)),
};
}
}
}
}

Ok(None)
}

#[cfg(test)]
mod tests {
use crate::bors::command::parser::{CommandParseError, CommandParser};
Expand Down Expand Up @@ -293,40 +326,97 @@ mod tests {
assert_eq!(cmds.len(), 1);
assert!(matches!(
cmds[0],
Ok(BorsCommand::Approve(Approver::Myself))
Ok(BorsCommand::Approve {
approver: Approver::Myself,
priority: None,
})
));
}

#[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###"
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_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 valid number",
),
)
"###);
}

#[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_unapprove() {
let cmds = parse_commands("@bors r-");
Expand Down
22 changes: 14 additions & 8 deletions src/bors/handlers/help.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,14 @@ 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::Try {
parent: None,
Expand All @@ -34,11 +40,11 @@ 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=<priority>]`: Approve this PR. Optionally, you can specify a `<priority>`."
}
BorsCommand::Approve(Approver::Specified(_)) => {
"`r=<user>`: Approve this PR on behalf of `<user>`"
BorsCommand::Approve {approver: Approver::Specified(_), ..} => {
"`r=<user> [p=<priority>]`: Approve this PR on behalf of `<user>`. Optionally, you can specify a `<priority>`."
}
BorsCommand::Unapprove => {
"`r-`: Unapprove this PR"
Expand Down Expand Up @@ -68,8 +74,8 @@ 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=<user>`: Approve this PR on behalf of `<user>`
- `r+ [p=<priority>]`: Approve this PR. Optionally, you can specify a `<priority>`.
- `r=<user> [p=<priority>]`: Approve this PR on behalf of `<user>`. Optionally, you can specify a `<priority>`.
- `r-`: Unapprove this PR
- `try [parent=<parent>] [jobs=<jobs>]`: Start a try build. Optionally, you can specify a `<parent>` SHA or a list of `<jobs>` to run
- `try cancel`: Cancel a running try build
Expand Down
15 changes: 11 additions & 4 deletions src/bors/handlers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,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");
Expand Down
Loading