diff --git a/Cargo.lock b/Cargo.lock index 5f681c8f..31993622 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -188,6 +188,18 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "24508e28c677875c380c20f4d28124fab6f8ed4ef929a1397d7b1a31e92f1005" +[[package]] +name = "cargo-lock" +version = "7.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7fb04b88bd5b2036e30704f95c6ee16f3b5ca3b4ca307da2889d9006648e5c88" +dependencies = [ + "semver 1.0.4", + "serde", + "toml", + "url", +] + [[package]] name = "cc" version = "1.0.54" @@ -1099,6 +1111,7 @@ dependencies = [ "base64 0.11.0", "bincode", "byteorder", + "cargo-lock", "chrono", "curl", "curl-sys", @@ -1407,7 +1420,7 @@ version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "138e3e0acb6c9fb258b19b67cb8abd63c00679d2851805ea151465464fe9030a" dependencies = [ - "semver", + "semver 0.9.0", ] [[package]] @@ -1476,6 +1489,15 @@ dependencies = [ "semver-parser", ] +[[package]] +name = "semver" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "568a8e6258aa33c13358f81fd834adb854c6f7c9468520910a9b1e8fac068012" +dependencies = [ + "serde", +] + [[package]] name = "semver-parser" version = "0.7.0" @@ -1693,18 +1715,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.20" +version = "1.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7dfdd070ccd8ccb78f4ad66bf1982dc37f620ef696c6b5028fe2ed83dd3d0d08" +checksum = "318234ffa22e0920fe9a40d7b8369b5f649d490980cf7aadcf1eb91594869b42" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.20" +version = "1.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bd80fc12f73063ac132ac92aceea36734f04a1d93c1240c6944e23a3b8841793" +checksum = "cae2447b6282786c3493999f40a9be2a6ad20cb8bd268b0a0dbf5a065535c0ab" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index 52f85acd..f1e58a56 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,3 +41,4 @@ serde_yaml = "0.8" thiserror = "1" url = "2.1.1" html-escape = "0.2.9" +cargo-lock = "^7.0.1" diff --git a/src/companion.rs b/src/companion.rs index c2f2e1ed..f1e652a0 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -1,13 +1,22 @@ use regex::RegexBuilder; use rocksdb::DB; use snafu::ResultExt; -use std::path::Path; +use std::{ + collections::HashMap, collections::HashSet, iter::FromIterator, path::Path, + time::Duration, +}; +use tokio::time::delay_for; use crate::{ - cmd::*, constants::MAIN_REPO_FOR_STAGING, error::*, github::*, - github_bot::GithubBot, webhook::wait_to_merge, Result, - COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, COMPANION_SHORT_REGEX, - PR_HTML_URL_REGEX, + cmd::*, + config::BotConfig, + constants::MAIN_REPO_FOR_STAGING, + error::*, + github::*, + github_bot::GithubBot, + webhook::{merge, ready_to_merge, wait_to_merge}, + Result, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, + COMPANION_SHORT_REGEX, PR_HTML_URL_REGEX, }; async fn update_companion_repository( @@ -165,29 +174,55 @@ async fn update_companion_repository( return Err(e); } - // `cargo update` should normally make changes to the lockfile with the latest SHAs from Github - run_cmd( - "cargo", - &[ - "update", - "-vp", - if merge_done_in == MAIN_REPO_FOR_STAGING { - MAIN_REPO_FOR_STAGING + // Update the packages from 'merge_done_in' in the companion + let url_merge_was_done_in = + format!("https://github.com/{}/{}", owner, merge_done_in); + let cargo_lock_path = Path::new(&repo_dir).join("Cargo.lock"); + let lockfile = + cargo_lock::Lockfile::load(cargo_lock_path).map_err(|err| { + Error::Message { + msg: format!( + "Failed to parse lockfile of {}: {:?}", + contributor_repo, err + ), + } + })?; + let pkgs_in_companion: HashSet<&str> = { + HashSet::from_iter(lockfile.packages.iter().filter_map(|pkg| { + if let Some(src) = pkg.source.as_ref() { + if src.url().as_str() == url_merge_was_done_in { + Some(pkg.name.as_str()) + } else { + None + } } else { - "sp-io" - }, - ], - &repo_dir, - CommandMessage::Configured(CommandMessageConfiguration { - secrets_to_hide, - are_errors_silenced: false, - }), - ) - .await?; + None + } + })) + }; + if !pkgs_in_companion.is_empty() { + let args = { + let mut args = vec!["update", "-v"]; + args.extend( + pkgs_in_companion.iter().map(|pkg| ["-p", pkg]).flatten(), + ); + args + }; + run_cmd( + "cargo", + &args, + &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( + let output = run_cmd_with_output( "git", &["status", "--short"], &repo_dir, @@ -197,7 +232,7 @@ async fn update_companion_repository( }), ) .await?; - if !String::from_utf8_lossy(&(&changes_after_update_output).stdout[..]) + if !String::from_utf8_lossy(&(&output).stdout[..]) .trim() .is_empty() { @@ -291,16 +326,70 @@ fn companion_parse_short(body: &str) -> Option { Some((html_url, owner, repo, number)) } -async fn perform_companion_update( +fn parse_all_companions(body: &str) -> Vec { + body.lines().filter_map(companion_parse).collect() +} + +pub async fn check_all_companions_are_mergeable( github_bot: &GithubBot, + pr: &PullRequest, + merge_done_in: &str, +) -> Result<()> { + if merge_done_in == "substrate" || merge_done_in == MAIN_REPO_FOR_STAGING { + if let Some(body) = pr.body.as_ref() { + for (html_url, owner, repo, number) in parse_all_companions(body) { + let companion = + github_bot.pull_request(&owner, &repo, number).await?; + + let is_owner_a_user = companion + .user + .as_ref() + .map(|user| { + user.type_field + .as_ref() + .map(|user_type| user_type == &UserType::User) + .unwrap_or(false) + }) + .unwrap_or(false); + if !is_owner_a_user { + return Err(Error::Message { + msg: format!( + "Companion {} is not owned by a user, therefore processbot would not be able to push the lockfile update to their branch due to a Github limitation (https://github.com/isaacs/github/issues/1681)", + html_url + ), + }); + } + + let is_mergeable = companion + .mergeable + .map(|mergeable| mergeable) + .unwrap_or(false); + if !is_mergeable { + return Err(Error::Message { + msg: format!( + "Github API says companion {} is not mergeable", + html_url + ), + }); + } + } + } + } + + Ok(()) +} + +async fn update_then_merge_companion( + github_bot: &GithubBot, + bot_config: &BotConfig, db: &DB, html_url: &str, owner: &str, repo: &str, - number: i64, + number: &i64, merge_done_in: &str, ) -> Result<()> { - let comp_pr = github_bot.pull_request(&owner, &repo, number).await?; + let pr = github_bot.pull_request(&owner, &repo, *number).await?; if let PullRequest { head: @@ -318,13 +407,13 @@ async fn perform_companion_update( .. }), .. - } = comp_pr.clone() + } = pr.clone() { log::info!("Updating companion {}", html_url); let updated_sha = update_companion_repository( github_bot, - &owner, - &repo, + owner, + repo, &contributor, &contributor_repo, &contributor_branch, @@ -332,18 +421,35 @@ async fn perform_companion_update( ) .await?; - log::info!("Companion updated; waiting for checks on {}", html_url); - wait_to_merge( - github_bot, - &owner, - &repo, - comp_pr.number, - &comp_pr.html_url, - &format!("parity-processbot[bot]"), - &updated_sha, - db, - ) - .await?; + // Wait a bit for all the statuses to settle after we've updated the companion. + delay_for(Duration::from_millis(4096)).await; + + let pr = github_bot.pull_request(&owner, &repo, *number).await?; + if ready_to_merge(github_bot, owner, repo, &pr).await? { + merge( + github_bot, + owner, + repo, + &pr, + bot_config, + "parity-processbot[bot]", + None, + ) + .await??; + } else { + log::info!("Companion updated; waiting for checks on {}", html_url); + wait_to_merge( + github_bot, + &owner, + &repo, + pr.number, + &pr.html_url, + &format!("parity-processbot[bot]"), + &updated_sha, + db, + ) + .await?; + } } else { return Err(Error::Message { msg: "Companion PR is missing some API data.".to_string(), @@ -353,61 +459,112 @@ async fn perform_companion_update( Ok(()) } -async fn detect_then_update_companion( +pub async fn merge_companions( github_bot: &GithubBot, + bot_config: &BotConfig, merge_done_in: &str, pr: &PullRequest, db: &DB, ) -> Result<()> { + let mut errors: Vec = vec![]; + if merge_done_in == "substrate" || merge_done_in == MAIN_REPO_FOR_STAGING { log::info!("Checking for companion."); - if let Some((html_url, owner, repo, number)) = - pr.body.as_ref().map(|body| companion_parse(body)).flatten() - { - log::info!("Found companion {}", html_url); - perform_companion_update( - github_bot, - db, - &html_url, - &owner, - &repo, - number, - merge_done_in, - ) - .await - .map_err(|e| e.map_issue((owner, repo, number)))?; - } else { - log::info!("No companion found."); - } - } - - Ok(()) -} -/// Check for a Polkadot companion and update it if found. -pub async fn update_companion( - github_bot: &GithubBot, - merge_done_in: &str, - pr: &PullRequest, - db: &DB, -) -> Result<()> { - detect_then_update_companion(github_bot, merge_done_in, pr, db) - .await - .map_err(|e| match e { - Error::WithIssue { source, issue } => { - Error::CompanionUpdate { source }.map_issue(issue) - } - _ => { - let e = Error::CompanionUpdate { - source: Box::new(e), + if let Some(body) = pr.body.as_ref() { + let companions = parse_all_companions(body); + if companions.is_empty() { + log::info!("No companion found."); + } else { + let companions_groups = { + let mut companions_groups: HashMap< + String, + Vec, + > = HashMap::new(); + for comp in companions { + let (_, ref owner, ref repo, _) = comp; + let key = format!("{}/{}", owner, repo); + if let Some(group) = companions_groups.get_mut(&key) { + group.push(comp); + } else { + companions_groups.insert(key, vec![comp]); + } + } + companions_groups }; - if let Some(details) = pr.get_issue_details() { - e.map_issue(details) - } else { - e + + let mut remaining_futures = companions_groups + .into_values() + .map(|group| { + Box::pin(async move { + let mut errors: Vec< + CompanionDetailsWithErrorMessage, + > = vec![]; + + for (html_url, owner, repo, ref number) in group { + if let Err(err) = update_then_merge_companion( + github_bot, + bot_config, + db, + &html_url, + &owner, + &repo, + number, + merge_done_in, + ) + .await + { + errors.push( + CompanionDetailsWithErrorMessage { + owner: owner.to_owned(), + repo: repo.to_owned(), + number: *number, + html_url: html_url.to_owned(), + msg: format!( + "Companion update failed: {:?}", + err + ), + }, + ); + } + } + + errors + }) + }) + .collect::>(); + while !remaining_futures.is_empty() { + let (result, _, next_remaining_futures) = + futures::future::select_all(remaining_futures).await; + for CompanionDetailsWithErrorMessage { + ref owner, + ref repo, + ref number, + ref html_url, + ref msg, + } in result + { + let _ = github_bot + .create_issue_comment(owner, repo, *number, msg) + .await + .map_err(|e| { + log::error!("Error posting comment: {}", e); + }); + errors.push(format!("{} {}", html_url, msg)); + } + remaining_futures = next_remaining_futures; } } + } + } + + if errors.is_empty() { + Ok(()) + } else { + Err(Error::Message { + msg: format!("Companion update failed: {}", errors.join("\n")), }) + } } #[cfg(test)] @@ -496,4 +653,29 @@ mod tests { None ); } + + #[test] + fn test_parse_all_companions() { + let owner = "paritytech"; + let repo = "polkadot"; + let pr_number = 1234; + let companion_url = + format!("https://github.com/{}/{}/pull/{}", owner, repo, pr_number); + let expected_companion = ( + companion_url.to_owned(), + owner.to_owned(), + repo.to_owned(), + pr_number, + ); + assert_eq!( + parse_all_companions(&format!( + " + first companion: {} + second companion: {} + ", + &companion_url, &companion_url + )), + vec![expected_companion.clone(), expected_companion.clone()] + ); + } } diff --git a/src/error.rs b/src/error.rs index ac11da69..2c99569f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -6,6 +6,14 @@ pub type IssueDetails = (String, String, i64); // TODO this really should be struct { repository_url, repository, owner, number } pub type IssueDetailsWithRepositoryURL = (String, String, String, i64); +pub struct CompanionDetailsWithErrorMessage { + pub owner: String, + pub repo: String, + pub number: i64, + pub html_url: String, + pub msg: String, +} + #[derive(Debug, Snafu)] #[snafu(visibility = "pub")] pub enum Error { @@ -31,11 +39,6 @@ pub enum Error { created_approval_id: Option, }, - #[snafu(display("Companion update failed: {}", source))] - CompanionUpdate { - source: Box, - }, - #[snafu(display("Rebase failed: {}", source))] Rebase { source: Box, @@ -196,7 +199,7 @@ pub enum Error { }, #[snafu(display( - "Merge failure was skipped (will be solved later): {}", + "Encountered merge failure (would be solved later): {}", msg ))] MergeFailureWillBeSolvedLater { diff --git a/src/github.rs b/src/github.rs index 5532cef3..da0c07e3 100644 --- a/src/github.rs +++ b/src/github.rs @@ -208,6 +208,7 @@ pub struct RequestedReviewers { #[derive(PartialEq, Debug, Clone, Serialize, Deserialize)] pub enum UserType { + User, Bot, #[serde(other)] Unknown, diff --git a/src/webhook.rs b/src/webhook.rs index 073ae4b5..895a33a0 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -435,9 +435,9 @@ async fn checks_and_status( .await??; db.delete(&commit_sha) .context(Db)?; - update_companion( - github_bot, &repo_name, - &pr, db, + merge_companions( + github_bot, bot_config, + &repo_name, &pr, db, ) .await } @@ -559,7 +559,8 @@ async fn handle_comment( Err(e) => return Err(e), _ => (), } - update_companion(github_bot, &repo_name, pr, db).await?; + merge_companions(github_bot, bot_config, &repo_name, pr, db) + .await?; } else { let pr_head_sha = pr.head_sha()?; wait_to_merge( @@ -637,7 +638,7 @@ async fn handle_comment( Err(e) => return Err(e), _ => (), } - update_companion(github_bot, &repo_name, &pr, db).await?; + merge_companions(github_bot, bot_config, &repo_name, &pr, db).await?; } else if body.to_lowercase().trim() == AUTO_MERGE_CANCEL.to_lowercase().trim() { @@ -868,17 +869,17 @@ async fn merge_allowed( requested_by: &str, min_approvals_required: Option, ) -> Result>> { - // We've noticed the bot failing for no human-discernable reason when, for instance, it - // complained that the pull request was not mergeable when, in fact, it seemed to be, - // if one were to guess what the state of the Github API was at the time the response was - // received with "second" precision. For the lack of insight onto the Github Servers, it's - // assumed that those failures happened because the Github API did not update fast enough and - // therefore the state was invalid when the request happened, but it got cleared shortly after - // (possiblymicroseconds after, hence why it is not discernable at "second" resolution). + // We've noticed the bot failing for no human-discernable reason when, for instance, it complained + // that the pull request was not mergeable when, in fact, it seemed to be, if one were to guess + // what the state of the Github API was at the time the response was received with "second" + // precision. For the lack of insight onto the Github Servers, it's assumed that those failures + // happened because the Github API did not update fast enough and therefore the state was invalid + // when the request happened, but it got cleared shortly after (possibly microseconds after, hence + // why it is not discernable at "second" resolution). // As a workaround we'll wait for long enough so that Github hopefully has time to update the API // and make our merges succeed. A proper workaround would also entail retrying every X seconds for // recoverable errors such as "required statuses are missing or pending". - delay_for(Duration::from_millis(8192)).await; + delay_for(Duration::from_millis(4096)).await; let is_mergeable = pr.mergeable.unwrap_or(false); @@ -893,6 +894,8 @@ async fn merge_allowed( log::info!("{} is not mergeable", pr.html_url); } + check_all_companions_are_mergeable(github_bot, pr, repo_name).await?; + if is_mergeable || min_approvals_required.is_some() { match github_bot.reviews(&pr.url).await { Ok(reviews) => { @@ -1147,7 +1150,7 @@ async fn merge_allowed( /// /// This function is used when a merge request is first received, to decide whether to store the /// request and wait for checks -- if so they will later be handled by `checks_and_status`. -async fn ready_to_merge( +pub async fn ready_to_merge( github_bot: &GithubBot, owner: &str, repo_name: &str, @@ -1292,7 +1295,7 @@ async fn prepare_to_merge( /// It might recursively call itself when attempting to solve a merge error after something /// meaningful happens. #[async_recursion] -async fn merge( +pub async fn merge( github_bot: &GithubBot, owner: &str, repo_name: &str, @@ -1639,7 +1642,6 @@ Approval by \"Project Owners\" is only attempted if other means defined in the [ ref status } => Some(format!("Response error (status {}): 
{}
", status, html_escape::encode_safe(&body.to_string()))), Error::OrganizationMembership { .. } - | Error::CompanionUpdate { .. } | Error::Message { .. } | Error::Rebase { .. } => { Some(format!("Error: {}", err))