diff --git a/src/cmd.rs b/src/cmd.rs new file mode 100644 index 00000000..3a36ed0a --- /dev/null +++ b/src/cmd.rs @@ -0,0 +1,166 @@ +use crate::{error::*, Result}; +use snafu::ResultExt; +use std::ffi::OsStr; +use std::fmt::Display; +use std::path::Path; +use std::process::{Output, Stdio}; +use tokio::process::Command; + +#[derive(PartialEq)] +pub struct CommandMessageConfiguration<'a> { + pub secrets_to_hide: Option<&'a [&'a str]>, + pub are_errors_silenced: bool, +} + +#[derive(PartialEq)] +pub enum CommandMessage<'a> { + Configured(CommandMessageConfiguration<'a>), +} + +pub async fn run_cmd<'a, Cmd, Dir>( + cmd: Cmd, + args: &[&str], + dir: Dir, + logging: CommandMessage<'a>, +) -> Result +where + Cmd: AsRef + Display, + Dir: AsRef + Display, +{ + before_cmd(&cmd, args, Some(&dir), &logging); + + #[allow(unused_mut)] + let mut init_cmd = Command::new(cmd); + let cmd = init_cmd.args(args).current_dir(dir).stderr(Stdio::piped()); + let result = cmd.output().await.context(Tokio)?; + + handle_cmd_result(cmd, result, &logging) +} + +pub async fn run_cmd_in_cwd<'a, Cmd>( + cmd: Cmd, + args: &[&str], + logging: CommandMessage<'a>, +) -> Result +where + Cmd: AsRef + Display, +{ + before_cmd::<&Cmd, String>(&cmd, args, None, &logging); + + #[allow(unused_mut)] + let mut init_cmd = Command::new(cmd); + let cmd = init_cmd.args(args).stderr(Stdio::piped()); + let result = cmd.output().await.context(Tokio)?; + + handle_cmd_result(cmd, result, &logging) +} + +pub async fn run_cmd_with_output<'a, Cmd, Dir>( + cmd: Cmd, + args: &[&str], + dir: Dir, + logging: CommandMessage<'a>, +) -> Result +where + Cmd: AsRef + Display, + Dir: AsRef + Display, +{ + before_cmd(&cmd, args, Some(&dir), &logging); + + #[allow(unused_mut)] + let mut init_cmd = Command::new(cmd); + let cmd = init_cmd + .args(args) + .current_dir(dir) + .stdin(Stdio::piped()) + .stderr(Stdio::piped()); + let result = cmd.output().await.context(Tokio)?; + + handle_cmd_result(cmd, result, &logging) +} + +fn before_cmd<'a, Cmd, Dir>( + cmd: Cmd, + args: &[&str], + dir: Option, + logging: &CommandMessage<'a>, +) where + Cmd: AsRef + Display, + Dir: AsRef + Display, +{ + match logging { + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + .. + }) => { + let mut cmd_display = format!("{}", cmd); + let mut args_display = format!("{:?}", args); + if let Some(secrets) = secrets_to_hide.as_ref() { + for secret in secrets.iter() { + cmd_display = cmd_display.replace(secret, "${SECRET}"); + args_display = args_display.replace(secret, "${SECRET}"); + } + } + + if let Some(dir) = dir { + log::info!("Run {} {} in {}", cmd_display, args_display, dir); + } else { + log::info!( + "Run {} {} in the current directory", + cmd_display, + args_display, + ); + } + } + }; +} + +fn handle_cmd_result<'a>( + cmd: &mut Command, + result: Output, + logging: &CommandMessage<'a>, +) -> Result { + if result.status.success() { + Ok(result) + } else { + let (cmd_display, err_msg) = match logging { + CommandMessage::Configured(CommandMessageConfiguration { + are_errors_silenced, + secrets_to_hide, + }) => { + let mut cmd_display = format!("{:?}", cmd); + if let Some(secrets) = secrets_to_hide.as_ref() { + for secret in secrets.iter() { + cmd_display = cmd_display.replace(secret, "${SECRET}"); + } + } + let err_msg = if *are_errors_silenced { + None + } else { + let err_output = String::from_utf8_lossy(&result.stderr); + if err_output.is_empty() { + None + } else { + let mut err_output = err_output.to_string(); + if let Some(secrets) = secrets_to_hide.as_ref() { + for secret in secrets.iter() { + err_output = + err_output.replace(secret, "${SECRET}"); + } + } + log::error!("{}", err_output); + Some(err_output) + } + }; + + (cmd_display, err_msg) + } + }; + + Err(Error::CommandFailed { + cmd: cmd_display, + status_code: result.status.code(), + err: err_msg.unwrap_or_else(|| "no output".to_string()), + }) + } +} diff --git a/src/companion.rs b/src/companion.rs index 1b0e036a..16748b70 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -1,195 +1,233 @@ use regex::Regex; use snafu::ResultExt; -use tokio::process::Command; +use std::path::Path; -use crate::{error::*, github_bot::GithubBot, Result}; +use crate::{cmd::*, error::*, github_bot::GithubBot, Result}; pub async fn companion_update( github_bot: &GithubBot, - base_owner: &str, - base_repo: &str, - head_owner: &str, - head_repo: &str, - branch: &str, -) -> Result> { - let res = companion_update_inner( - github_bot, base_owner, base_repo, head_owner, head_repo, branch, + owner: &str, + owner_repo: &str, + contributor: &str, + contributor_repo: &str, + contributor_branch: &str, +) -> Result { + let token = github_bot.client.auth_key().await?; + let secrets_to_hide = [token.as_str()]; + let secrets_to_hide = Some(&secrets_to_hide[..]); + + let owner_repository_domain = + format!("github.com/{}/{}.git", owner, owner_repo); + let owner_remote_address = format!( + "https://x-access-token:{}@{}", + token, owner_repository_domain + ); + let repo_dir = format!("./{}", owner_repo); + + if Path::new(&repo_dir).exists() { + log::info!("{} is already cloned; skipping", owner_repository_domain); + } else { + run_cmd_in_cwd( + "git", + &["clone", "-v", &owner_remote_address], + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + } + + let contributor_repository_domain = + format!("github.com/{}/{}.git", contributor, contributor_repo); + let contributor_remote_branch = + format!("{}/{}", contributor, contributor_branch); + let contributor_remote_address = format!( + "https://x-access-token:{}@{}", + token, contributor_repository_domain + ); + + // The contributor's remote entry might exist from a previous run (not expected for a fresh + // clone). If so, delete it so that it can be recreated. + if run_cmd( + "git", + &["remote", "get-url", contributor], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: true, + }), + ) + .await + .is_ok() + { + run_cmd( + "git", + &["remote", "remove", contributor], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + } + run_cmd( + "git", + &["remote", "add", contributor, &contributor_remote_address], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + + run_cmd( + "git", + &["fetch", contributor, contributor_branch], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + + // The contributor's branch might exist from a previous run (not expected for a fresh clone). + // If so, delete it so that it can be recreated. + let _ = run_cmd( + "git", + &["branch", "-D", contributor_branch], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: true, + }), ) .await; - // checkout origin master - log::info!("Checking out master."); - Command::new("git") - .arg("checkout") - .arg("master") - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - // delete temp branch - log::info!("Deleting head branch."); - Command::new("git") - .arg("branch") - .arg("-D") - .arg(format!("{}", branch)) - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - // remove temp remote - log::info!("Removing temp remote."); - Command::new("git") - .arg("remote") - .arg("remove") - .arg("temp") - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - res -} + run_cmd( + "git", + &["checkout", "--track", &contributor_remote_branch], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; -async fn companion_update_inner( - github_bot: &GithubBot, - base_owner: &str, - base_repo: &str, - head_owner: &str, - head_repo: &str, - branch: &str, -) -> Result> { - let token = github_bot.client.auth_key().await?; - let mut updated_sha = None; - // clone in case the local clone doesn't exist - log::info!("Cloning repo."); - Command::new("git") - .arg("clone") - .arg("-v") - .arg(format!( - "https://x-access-token:{token}@github.com/{owner}/{repo}.git", - token = token, - owner = base_owner, - repo = base_repo, - )) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - // add temp remote - log::info!("Adding temp remote."); - Command::new("git") - .arg("remote") - .arg("add") - .arg("temp") - .arg(format!( - "https://x-access-token:{token}@github.com/{owner}/{repo}.git", - token = token, - owner = head_owner, - repo = head_repo, - )) - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - // fetch temp - log::info!("Fetching temp."); - Command::new("git") - .arg("fetch") - .arg("temp") - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - // checkout temp branch - log::info!("Checking out head branch."); - let checkout = Command::new("git") - .arg("checkout") - .arg("-b") - .arg(format!("{}", branch)) - .arg(format!("temp/{}", branch)) - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - if checkout.success() { - // merge origin master - log::info!("Merging master."); - let merge_master = Command::new("git") - .arg("merge") - .arg("origin/master") - .arg("--no-ff") - .arg("--no-edit") - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - if merge_master.success() { - // update - log::info!("Updating substrate."); - Command::new("cargo") - .arg("update") - .arg("-vp") - .arg("sp-io") - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - // commit - log::info!("Committing changes."); - Command::new("git") - .arg("commit") - .arg("-am") - .arg("\"Update Substrate\"") - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - // push - log::info!("Pushing changes."); - Command::new("git") - .arg("push") - .arg("temp") - .arg(format!("{}", branch)) - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - // rev-parse - log::info!("Parsing SHA."); - let output = Command::new("git") - .arg("rev-parse") - .arg("HEAD") - .current_dir(format!("./{}", base_repo)) - .output() - .await - .context(Tokio)?; - updated_sha = Some( - String::from_utf8(output.stdout) - .context(Utf8)? - .trim() - .to_string(), - ); - } else { - // abort merge - log::info!("Aborting merge."); - Command::new("git") - .arg("merge") - .arg("--abort") - .current_dir(format!("./{}", base_repo)) - .spawn() - .context(Tokio)? - .await - .context(Tokio)?; - } + let owner_remote = "origin"; + let owner_branch = "master"; + let owner_remote_branch = format!("{}/{}", owner_remote, owner_branch); + + run_cmd( + "git", + &["fetch", owner_remote, &owner_branch], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + + // Create master merge commit before updating packages + let master_merge_result = run_cmd( + "git", + &["merge", &owner_remote_branch, "--no-ff", "--no-edit"], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await; + if let Err(e) = master_merge_result { + log::info!("Aborting companion update due to master merge failure"); + run_cmd( + "git", + &["merge", "--abort"], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + return Err(e); } + + // `cargo update` should normally make changes to the lockfile with the latest SHAs from Github + run_cmd( + "cargo", + &["update", "-vp", "sp-io"], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + + // Check if `cargo update` resulted in any changes. If the master merge commit already had the + // latest lockfile then no changes might have been made. + let changes_after_update_output = run_cmd_with_output( + "git", + &["status", "--short"], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + if !String::from_utf8_lossy(&(&changes_after_update_output).stdout[..]) + .trim() + .is_empty() + { + run_cmd( + "git", + &["commit", "-am", "update Substrate"], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + } + + run_cmd( + "git", + &["push", contributor, contributor_branch], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + + log::info!( + "Getting the head SHA after a companion update in {}", + &contributor_remote_branch + ); + let updated_sha_output = run_cmd_with_output( + "git", + &["rev-parse", "HEAD"], + &repo_dir, + CommandMessage::Configured(CommandMessageConfiguration { + secrets_to_hide, + are_errors_silenced: false, + }), + ) + .await?; + let updated_sha = String::from_utf8(updated_sha_output.stdout) + .context(Utf8)? + .trim() + .to_string(); + Ok(updated_sha) } @@ -199,7 +237,7 @@ pub fn companion_parse(body: &str) -> Option<(String, String, String, i64)> { fn companion_parse_long(body: &str) -> Option<(String, String, String, i64)> { let re = Regex::new( - r"companion.*(?Phttps://github.com/(?P[[:alpha:]]+)/(?P[[:alpha:]]+)/pull/(?P[[:digit:]]+))" + r"companion[^[[:alpha:]]\n]*(?Phttps://github.com/(?P[^/\n]+)/(?P[^/\n]+)/pull/(?P[[:digit:]]+))" ) .unwrap(); let caps = re.captures(&body)?; @@ -217,7 +255,7 @@ fn companion_parse_long(body: &str) -> Option<(String, String, String, i64)> { fn companion_parse_short(body: &str) -> Option<(String, String, String, i64)> { let re = Regex::new( - r"companion.*: (?P[[:alpha:]]+)/(?P[[:alpha:]]+)#(?P[[:digit:]]+)" + r"companion[^[[:alpha:]]\n]*(?P[^/\n]+)/(?P[^/\n]+)#(?P[[:digit:]]+)", ) .unwrap(); let caps = re.captures(&body)?; @@ -244,9 +282,10 @@ mod tests { #[test] fn test_companion_parse() { + // Extra params should not be included in the parsed URL assert_eq!( companion_parse( - "companion: https://github.com/paritytech/polkadot/pull/1234" + "companion: https://github.com/paritytech/polkadot/pull/1234?extra_params=true" ), Some(( "https://github.com/paritytech/polkadot/pull/1234".to_owned(), @@ -255,9 +294,14 @@ mod tests { 1234 )) ); + + // Long version should work even if the body has some other content around the companion + // text assert_eq!( companion_parse( - "\nthis is a companion pr https://github.com/paritytech/polkadot/pull/1234" + "Companion line is in the middle + companion: https://github.com/paritytech/polkadot/pull/1234 + Final line" ), Some(( "https://github.com/paritytech/polkadot/pull/1234".to_owned(), @@ -266,15 +310,14 @@ mod tests { 1234 )) ); + + // Short version should work even if the body has some other content around the companion + // text assert_eq!( companion_parse( - "\nthis is some other pr https://github.com/paritytech/polkadot/pull/1234" - ), - None, - ); - assert_eq!( - companion_parse( - "\nthis is a companion pr https://github.com/paritytech/polkadot/pull/1234/plus+some&other_stuff" + "Companion line is in the middle + companion: paritytech/polkadot#1234 + Final line" ), Some(( "https://github.com/paritytech/polkadot/pull/1234".to_owned(), @@ -283,31 +326,25 @@ mod tests { 1234 )) ); + + // Long version should not be detected if "companion: " and the expression are not both in + // the same line assert_eq!( - companion_parse("companion\nparitytech/polkadot#1234"), + companion_parse( + "I want to talk about companion: but NOT reference it + I submitted it in https://github.com/paritytech/polkadot/pull/1234" + ), None ); + + // Short version should not be detected if "companion: " and the expression are not both in + // the same line assert_eq!( - companion_parse("companion: paritytech/polkadot#1234"), - Some(( - "https://github.com/paritytech/polkadot/pull/1234".to_owned(), - "paritytech".to_owned(), - "polkadot".to_owned(), - 1234 - )) - ); - assert_eq!( - companion_parse("companion: paritytech/polkadot/1234"), + companion_parse( + "I want to talk about companion: but NOT reference it + I submitted it in paritytech/polkadot#1234" + ), None ); - assert_eq!( - companion_parse("stuff\ncompanion pr: paritytech/polkadot#1234"), - Some(( - "https://github.com/paritytech/polkadot/pull/1234".to_owned(), - "paritytech".to_owned(), - "polkadot".to_owned(), - 1234 - )) - ); } } diff --git a/src/error.rs b/src/error.rs index 897a9d74..7911327b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -164,6 +164,18 @@ pub enum Error { UrlCannotBeBase { url: String, }, + + #[snafu(display( + "Cmd '{}' failed with status {:?}; output: {}", + cmd, + status_code, + err + ))] + CommandFailed { + cmd: String, + status_code: Option, + err: String, + }, } impl Error { diff --git a/src/lib.rs b/src/lib.rs index 89e53af5..c15a4b22 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,6 @@ mod auth; pub mod bamboo; +pub mod cmd; pub mod companion; pub mod config; pub mod constants; diff --git a/src/webhook.rs b/src/webhook.rs index 0699074b..d73fddff 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -131,7 +131,10 @@ pub async fn webhook_inner( let payload = serde_json::from_slice::(&msg_bytes).ok().context( Message { - msg: format!("Error parsing request body"), + msg: format!( + "Error parsing request body {}", + String::from_utf8_lossy(&msg_bytes) + ), }, )?; @@ -1241,7 +1244,7 @@ async fn update_companion( } = comp_pr.clone() { log::info!("Updating companion {}", comp_html_url); - if let Some(updated_sha) = companion_update( + match companion_update( github_bot, &comp_owner, &comp_repo, @@ -1250,47 +1253,53 @@ async fn update_companion( &comp_head_branch, ) .await - .map_err(|e| { - Error::Companion { - source: Box::new(e), - } - .map_issue(Some(( - comp_owner.to_string(), - comp_repo.to_string(), - comp_number, - ))) - })? { - log::info!( - "Companion updated; waiting for checks on {}", - comp_html_url - ); - - // wait for checks on the update commit - wait_to_merge( - github_bot, - &comp_owner, - &comp_repo, - comp_pr.number, - &comp_pr.html_url, - &format!("parity-processbot[bot]"), - &updated_sha, - db, - ) - .await?; - } else { - log::info!( - "Failed updating companion {}", - comp_html_url - ); + { + Ok(updated_sha) => { + log::info!( + "Companion updated; waiting for checks on {}", + comp_html_url + ); - Err(Error::Message { - msg: format!("Failed updating substrate."), + // wait for checks on the update commit + wait_to_merge( + github_bot, + &comp_owner, + &comp_repo, + comp_pr.number, + &comp_pr.html_url, + &format!("parity-processbot[bot]"), + &updated_sha, + db, + ) + .await?; + } + Err(e) => { + let err_str = format!("{}", e); + let err_str = err_str.trim(); + log::info!( + "Failed companion update in {} with error: {}", + comp_html_url, + err_str + ); + github_bot + .create_issue_comment( + &comp_owner, + &comp_repo, + comp_number, + format!( + " +Failed companion update: + +``` +{} +``` +", + &err_str + ) + .as_str(), + ) + .await?; } - .map_issue(Some(( - comp_owner.to_string(), - comp_repo.to_string(), - comp_number, - ))))?; } } else { Err(Error::Companion {