From 8ddd03e3697337fa81a5aa049600d704b5796ac1 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Fri, 8 Oct 2021 06:08:41 -0300 Subject: [PATCH 01/45] improve error handling so that the bot is able to communicate errors better in comments --- src/cmd.rs | 2 +- src/companion.rs | 95 ++- src/error.rs | 21 +- src/github.rs | 35 +- src/github_bot/repository.rs | 17 +- src/lib.rs | 12 + src/webhook.rs | 1364 ++++++++++++++++++---------------- 7 files changed, 886 insertions(+), 660 deletions(-) diff --git a/src/cmd.rs b/src/cmd.rs index 8289103b..dcb9b1c5 100644 --- a/src/cmd.rs +++ b/src/cmd.rs @@ -148,7 +148,7 @@ fn handle_cmd_result<'a>( err_output.replace(secret, "${SECRET}"); } } - log::error!( + log::info!( "handle_cmd_result: {} failed with error: {}", cmd_display, err_output diff --git a/src/companion.rs b/src/companion.rs index f1e652a0..a7ec0ba3 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -14,8 +14,11 @@ use crate::{ error::*, github::*, github_bot::GithubBot, - webhook::{merge, ready_to_merge, wait_to_merge}, - Result, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, + webhook::{ + get_latest_checks_state, get_latest_statuses_state, merge, + ready_to_merge, wait_to_merge, + }, + Result, Status, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, COMPANION_SHORT_REGEX, PR_HTML_URL_REGEX, }; @@ -340,8 +343,9 @@ pub async fn check_all_companions_are_mergeable( for (html_url, owner, repo, number) in parse_all_companions(body) { let companion = github_bot.pull_request(&owner, &repo, number).await?; + let head_sha = companion.head_sha()?; - let is_owner_a_user = companion + let has_user_owner = companion .user .as_ref() .map(|user| { @@ -351,7 +355,7 @@ pub async fn check_all_companions_are_mergeable( .unwrap_or(false) }) .unwrap_or(false); - if !is_owner_a_user { + if !has_user_owner { 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)", @@ -372,6 +376,86 @@ pub async fn check_all_companions_are_mergeable( ), }); } + + let target_branch = github_bot + .branch( + &companion.base.repo.owner.login, + &companion.base.repo.name, + &companion.base.ref_field, + ) + .await?; + + let statuses = get_latest_statuses_state( + github_bot, + &owner, + &repo, + head_sha, + &pr.html_url, + Some( + &target_branch + .protection + .required_status_checks + .contexts, + ), + ) + .await?; + match statuses { + Status::Success => (), + Status::Pending => { + return Err(Error::InvalidCompanionStatus { + status: Status::Pending, + msg: format!( + "Companion {} has pending required statuses", + html_url + ), + }); + } + Status::Failure => { + return Err(Error::InvalidCompanionStatus { + status: Status::Failure, + msg: format!( + "Companion {} has failed required statuses", + html_url + ), + }); + } + }; + + let checks = get_latest_checks_state( + github_bot, + &owner, + &repo, + &head_sha, + &pr.html_url, + Some( + &target_branch + .protection + .required_status_checks + .contexts, + ), + ) + .await?; + match checks { + Status::Success => (), + Status::Pending => { + return Err(Error::InvalidCompanionStatus { + status: checks, + msg: format!( + "Companion {} has pending required checks", + html_url + ), + }); + } + Status::Failure => { + return Err(Error::InvalidCompanionStatus { + status: checks, + msg: format!( + "Companion {} has failed required checks", + html_url + ), + }); + } + }; } } } @@ -447,12 +531,13 @@ async fn update_then_merge_companion( &format!("parity-processbot[bot]"), &updated_sha, db, + None, ) .await?; } } else { return Err(Error::Message { - msg: "Companion PR is missing some API data.".to_string(), + msg: format!("Companion {} is missing some API data", html_url), }); } diff --git a/src/error.rs b/src/error.rs index b922918d..27241108 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,3 +1,4 @@ +use crate::Status; use snafu::Snafu; // TODO this really should be struct { repository, owner, number } @@ -50,11 +51,6 @@ pub enum Error { actual: String, }, - #[snafu(display("Error getting process info: {}", source))] - ProcessFile { - source: Box, - }, - #[snafu(display("Missing process info."))] ProcessInfo { errors: Option>, @@ -195,6 +191,12 @@ pub enum Error { MergeFailureWillBeSolvedLater { msg: String, }, + + #[snafu(display("{}", msg))] + InvalidCompanionStatus { + status: Status, + msg: String, + }, } impl Error { @@ -207,6 +209,15 @@ impl Error { }, } } + pub fn stops_merge_attempt(&self) -> bool { + match self { + Self::WithIssue { source, .. } | Self::Merge { source, .. } => { + source.stops_merge_attempt() + } + Self::MergeFailureWillBeSolvedLater { .. } => false, + _ => true, + } + } } impl From for Error { diff --git a/src/github.rs b/src/github.rs index 91ccdbe8..2af8e94f 100644 --- a/src/github.rs +++ b/src/github.rs @@ -1,5 +1,6 @@ use crate::{ - error::*, utils::parse_bot_comment_from_text, Result, PR_HTML_URL_REGEX, + error::*, utils::parse_bot_comment_from_text, + PlaceholderDeserializationItem, Result, PR_HTML_URL_REGEX, }; use regex::Regex; use serde::{Deserialize, Serialize}; @@ -9,6 +10,19 @@ pub trait HasIssueDetails { fn get_issue_details(&self) -> Option; } +#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct BranchProtectionRequiredStatusChecks { + pub contexts: Vec, +} +#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct BranchProtection { + pub required_status_checks: BranchProtectionRequiredStatusChecks, +} +#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct Branch { + pub protection: BranchProtection, +} + #[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct PullRequest { pub url: String, @@ -73,7 +87,7 @@ pub struct Issue { // User might be missing when it has been deleted pub user: Option, pub body: Option, - pub pull_request: Option, + pub pull_request: Option, pub repository: Option, pub repository_url: Option, } @@ -172,9 +186,6 @@ pub struct Team { pub id: i64, } -#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct IssuePullRequest {} - #[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Head { pub label: Option, @@ -188,10 +199,8 @@ pub struct Head { #[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Base { #[serde(rename = "ref")] - pub ref_field: Option, - pub sha: Option, - // Repository might be missing when it has been deleted - pub repo: Option, + pub ref_field: String, + pub repo: BaseRepo, } #[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -324,6 +333,12 @@ pub struct HeadRepo { pub owner: Option, } +#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct BaseRepo { + pub name: String, + pub owner: User, +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum CheckRunConclusion { @@ -354,7 +369,7 @@ pub struct WebhookIssueComment { pub number: i64, pub html_url: String, pub repository_url: Option, - pub pull_request: Option, + pub pull_request: Option, } impl HasIssueDetails for WebhookIssueComment { diff --git a/src/github_bot/repository.rs b/src/github_bot/repository.rs index 207b15ae..e879d635 100644 --- a/src/github_bot/repository.rs +++ b/src/github_bot/repository.rs @@ -3,7 +3,6 @@ use crate::{github, Result}; use super::GithubBot; impl GithubBot { - /// Returns a repository with the given name. pub async fn repository( &self, owner: &str, @@ -20,6 +19,22 @@ impl GithubBot { ); self.client.get(url).await } + + pub async fn branch( + &self, + owner: &str, + repo: &str, + branch: &str, + ) -> Result { + let url = format!( + "{}/repos/{}/{}/branches/{}", + Self::BASE_URL, + owner, + repo, + branch + ); + self.client.get(url).await + } } /* diff --git a/src/lib.rs b/src/lib.rs index 4cfab2c3..33d80d6d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,5 @@ +use serde::{Deserialize, Serialize}; + mod auth; pub mod bamboo; pub mod cmd; @@ -24,6 +26,7 @@ pub mod webhook; pub type Result = std::result::Result; +#[derive(Debug)] pub enum Status { Success, Pending, @@ -43,3 +46,12 @@ pub enum CommentCommand { BurninRequest, CompareReleaseRequest, } + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct PlaceholderDeserializationItem {} + +pub enum MergeCancelOutcome { + ShaDidNotExist, + WasCancelled, + WasNotCancelled, +} diff --git a/src/webhook.rs b/src/webhook.rs index 309966a3..d6ef4941 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -16,7 +16,7 @@ use crate::{ constants::*, error::*, github::*, github_bot::GithubBot, gitlab_bot::*, matrix_bot::MatrixBot, performance, process, rebase::*, utils::parse_bot_comment_from_text, vanity_service, CommentCommand, - MergeCommentCommand, Result, Status, + MergeCancelOutcome, MergeCommentCommand, Result, Status, }; /// This data gets passed along with each webhook to the webhook handler. @@ -72,11 +72,20 @@ pub async fn webhook( msg: format!("Error parsing x-hub-signature"), })? .to_string(); + log::info!("Lock acquired for {:?}", sig); - if let Err(e) = webhook_inner(req, state).await { - handle_error(e, state).await; - } + if let Some((merge_cancel_outcome, err)) = + match webhook_inner(req, state).await { + Ok((merge_cancel_outcome, result)) => match result { + Ok(_) => None, + Err(err) => Some((merge_cancel_outcome, err)), + }, + Err(err) => Some((MergeCancelOutcome::WasNotCancelled, err)), + } { + handle_error(merge_cancel_outcome, err, state).await + }; log::info!("Will release lock for {:?}", sig); + Response::builder() .status(StatusCode::OK) .body(Body::from("")) @@ -99,7 +108,7 @@ pub async fn webhook( pub async fn webhook_inner( mut req: Request, state: &AppState, -) -> Result<()> { +) -> Result<(MergeCancelOutcome, Result<()>)> { let mut msg_bytes = vec![]; while let Some(item) = req.body_mut().next().await { msg_bytes.extend_from_slice(&item.ok().context(Message { @@ -135,7 +144,7 @@ pub async fn webhook_inner( log::info!("Parsing payload {}", String::from_utf8_lossy(&msg_bytes)); match serde_json::from_slice::(&msg_bytes) { - Ok(payload) => handle_payload(payload, state).await, + Ok(payload) => Ok(handle_payload(payload, state).await), Err(err) => { // If this comment was originated from a Bot, then acting on it might make the bot // to respond to itself recursively, as happened on @@ -170,15 +179,18 @@ Payload: .map_issue(pr_details)) } else { log::info!("Ignoring payload parsing error",); - Ok(()) + Ok((MergeCancelOutcome::WasNotCancelled, Ok(()))) } } } } /// Match different kinds of payload. -async fn handle_payload(payload: Payload, state: &AppState) -> Result<()> { - match payload { +async fn handle_payload( + payload: Payload, + state: &AppState, +) -> (MergeCancelOutcome, Result<()>) { + let (result, sha) = match payload { Payload::IssueComment { action: IssueCommentAction::Created, comment: @@ -193,47 +205,98 @@ async fn handle_payload(payload: Payload, state: &AppState) -> Result<()> { }, issue, } => match type_field { - Some(UserType::Bot) => Ok(()), + Some(UserType::Bot) => (Ok(()), None), _ => match &issue { WebhookIssueComment { number, html_url, repository_url: Some(repo_url), pull_request: Some(_), - } => handle_comment( - body, login, *number, html_url, repo_url, state, - ) - .await - .map_err(|e| match e { - Error::WithIssue { .. } => e, - e => { - if let Some(details) = issue.get_issue_details() { - e.map_issue(details) - } else { - e - } - } - }), - _ => Ok(()), + } => { + let (sha, result) = handle_comment( + body, login, *number, html_url, repo_url, state, + ) + .await; + ( + result.map_err(|err| match err { + Error::WithIssue { .. } => err, + err => { + if let Some(details) = issue.get_issue_details() + { + err.map_issue(details) + } else { + err + } + } + }), + sha, + ) + } + _ => (Ok(()), None), }, }, Payload::CommitStatus { sha, state: status } => { - handle_status(sha, status, state).await + (handle_status(&sha, status, state).await, Some(sha)) } Payload::CheckRun { check_run: CheckRun { status, head_sha, .. }, .. - } => handle_check(status, head_sha, state).await, - _ => Ok(()), - } + } => (handle_check(&head_sha, status, state).await, Some(head_sha)), + _ => (Ok(()), None), + }; + + let merge_cancel_outcome = match result { + Ok(_) => MergeCancelOutcome::WasNotCancelled, + Err(ref err) => { + if err.stops_merge_attempt() { + if let Some(sha) = sha { + match state.db.get(sha.as_bytes()) { + Ok(Some(_)) => match state.db.delete(sha.as_bytes()) { + Ok(_) => { + log::info!( + "MR for {} was cancelled due to {:?}", + sha, + err + ); + MergeCancelOutcome::WasCancelled + } + Err(db_err) => { + log::error!( + "Failed to delete {} from the database due to {:?}", + &sha, + db_err + ); + MergeCancelOutcome::WasNotCancelled + } + }, + Ok(None) => MergeCancelOutcome::ShaDidNotExist, + Err(db_err) => { + log::info!( + "Failed to fetch {} from the database due to {:?}", + sha, + db_err + ); + MergeCancelOutcome::WasNotCancelled + } + } + } else { + MergeCancelOutcome::ShaDidNotExist + } + } else { + MergeCancelOutcome::WasNotCancelled + } + } + }; + + (merge_cancel_outcome, result) } /// If a check completes, query if all statuses and checks are complete. async fn handle_check( + commit_sha: &String, status: CheckRunStatus, - commit_sha: String, state: &AppState, ) -> Result<()> { if status == CheckRunStatus::Completed { @@ -251,7 +314,7 @@ async fn handle_check( /// If we receive a status other than `Pending`, query if all statuses and checks are complete. async fn handle_status( - commit_sha: String, + commit_sha: &String, status: StatusState, state: &AppState, ) -> Result<()> { @@ -268,12 +331,13 @@ async fn handle_status( } } -async fn get_latest_statuses_state( +pub async fn get_latest_statuses_state( github_bot: &GithubBot, owner: &str, owner_repo: &str, commit_sha: &str, html_url: &str, + filter_context: Option<&Vec>, ) -> Result { let status = github_bot.status(&owner, &owner_repo, &commit_sha).await?; log::info!("{:?}", status); @@ -297,6 +361,17 @@ async fn get_latest_statuses_state( { continue; } + + if let Some(filter_context) = filter_context { + if !filter_context + .iter() + .find(|ctx| **ctx == s.context) + .is_some() + { + continue; + } + } + if latest_statuses .get(&s.context) .map(|(prev_id, _)| prev_id < &(&s).id) @@ -327,12 +402,13 @@ async fn get_latest_statuses_state( ) } -async fn get_latest_checks_state( +pub async fn get_latest_checks_state( github_bot: &GithubBot, owner: &str, repo_name: &str, commit_sha: &str, html_url: &str, + filter_name: Option<&Vec>, ) -> Result { let checks = github_bot .check_runs(&owner, &repo_name, commit_sha) @@ -346,6 +422,11 @@ async fn get_latest_checks_state( (i64, CheckRunStatus, Option), > = HashMap::new(); for c in checks.check_runs { + if let Some(filter_name) = filter_name { + if !filter_name.iter().find(|name| **name == c.name).is_some() { + continue; + } + } if latest_checks .get(&c.name) .map(|(prev_id, _, _)| prev_id < &(&c).id) @@ -380,181 +461,144 @@ async fn checks_and_status( bot_config: &BotConfig, github_bot: &GithubBot, db: &DB, - commit_sha: &str, + sha: &str, ) -> Result<()> { - if let Some(pr_bytes) = db.get(commit_sha.as_bytes()).context(Db)? { - let m = bincode::deserialize(&pr_bytes).context(Bincode)?; - log::info!("Deserialized merge request: {:?}", m); + let pr_bytes = match db.get(sha.as_bytes()).context(Db)? { + Some(pr_bytes) => pr_bytes, + None => return Ok(()), + }; + + let mr = bincode::deserialize(&pr_bytes).context(Bincode)?; + log::info!("Deserialized merge request: {:?}", mr); + + async { let MergeRequest { owner, repo_name, number, html_url, requested_by, - } = m; - - // Wait a bit for all the statuses to settle; some missing status might be - // delivered with a small delay right after this is triggered, thus it's - // worthwhile to wait for it instead of having to recover from a premature - // merge attempt due to some slightly-delayed missing status. - delay_for(Duration::from_millis(4096)).await; - - match github_bot.pull_request(&owner, &repo_name, number).await { - Ok(pr) => match pr.head_sha() { - Ok(pr_head_sha) => { - if commit_sha != pr_head_sha { - Err(Error::HeadChanged { - expected: commit_sha.to_string(), - actual: pr_head_sha.to_owned(), - }) - } else { - match get_latest_statuses_state( - github_bot, &owner, &repo_name, commit_sha, - &html_url, + .. + } = &mr; + + let pr = github_bot.pull_request(owner, repo_name, *number).await?; + let pr_head_sha = pr.head_sha()?; + + if sha != pr_head_sha { + return Err(Error::HeadChanged { + expected: sha.to_string(), + actual: pr_head_sha.to_owned(), + }); + } + + let status = get_latest_statuses_state( + github_bot, &owner, &repo_name, sha, &html_url, None, + ) + .await?; + match status { + Status::Success => { + let checks = get_latest_checks_state( + github_bot, &owner, &repo_name, sha, &html_url, None, + ) + .await?; + match checks { + Status::Success => { + merge_allowed( + github_bot, + &owner, + &repo_name, + &pr, + bot_config, + &requested_by, + None, + true, ) - .await + .await?; + + match merge( + github_bot, + &owner, + &repo_name, + &pr, + bot_config, + &requested_by, + None, + ) + .await? { - Ok(status) => match status { - Status::Success => { - match get_latest_checks_state( - github_bot, &owner, &repo_name, - commit_sha, &html_url, - ) - .await - { - Ok(status) => match status { - Status::Success => { - merge( - github_bot, - &owner, - &repo_name, - &pr, - bot_config, - &requested_by, - None, - ) - .await??; - db.delete(&commit_sha) - .context(Db)?; - merge_companions( - github_bot, bot_config, - &repo_name, &pr, db, - ) - .await - } - Status::Failure => { - Err(Error::ChecksFailed { - commit_sha: commit_sha - .to_string(), - }) - } - _ => Ok(()), - }, - Err(e) => Err(e), - } - } - Status::Failure => Err(Error::ChecksFailed { - commit_sha: commit_sha.to_string(), - }), - _ => Ok(()), - }, - Err(e) => Err(e), + Ok(_) => { + db.delete(&sha).context(Db)?; + merge_companions( + github_bot, bot_config, &repo_name, &pr, db, + ) + .await + } + Err(Error::MergeFailureWillBeSolvedLater { + .. + }) => Ok(()), + Err(err) => Err(err), } } + Status::Failure => Err(Error::ChecksFailed { + commit_sha: sha.to_string(), + }), + Status::Pending => Ok(()), } - Err(e) => Err(e), - }, - Err(e) => Err(e), + } + Status::Failure => Err(Error::ChecksFailed { + commit_sha: sha.to_string(), + }), + Status::Pending => Ok(()), } - .map_err(|e| e.map_issue((owner, repo_name, number)))?; } - - Ok(()) + .await + .map_err(|err| err.map_issue((mr.owner, mr.repo_name, mr.number))) } -/// Parse bot commands in pull request comments. Commands are listed in README.md. -async fn handle_comment( - body: &str, - requested_by: &str, +async fn handle_command( + state: &AppState, + cmd: &CommentCommand, + owner: &str, + repo: &str, number: i64, html_url: &str, - repo_url: &str, - state: &AppState, + requested_by: &str, + pr: &PullRequest, ) -> Result<()> { - let db = &state.db; - let github_bot = &state.github_bot; - let bot_config = &state.bot_config; - - let owner = GithubBot::owner_from_html_url(html_url).context(Message { - msg: format!("Failed parsing owner in url: {}", html_url), - })?; - - let repo_name = - repo_url.rsplit('/').next().map(|s| s.to_string()).context( - Message { - msg: format!("Failed parsing repo name in url: {}", repo_url), - }, - )?; - - let cmd = match parse_bot_comment_from_text(body) { - Some(cmd) => cmd, - None => return Ok(()), - }; - log::info!("{:?} requested by {}", cmd, requested_by); - - let auth = - GithubUserAuthenticator::new(requested_by, owner, &repo_name, number); - auth.check_org_membership(&github_bot).await?; - - match cmd { - CommentCommand::Merge(_) => { - // 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(4096)).await; - } - _ => (), - } - - let pr = &github_bot.pull_request(owner, &repo_name, number).await?; + let AppState { + db, + github_bot, + bot_config, + .. + } = state; match cmd { CommentCommand::Merge(cmd) => { + let pr_head_sha = pr.head_sha()?; + merge_allowed( github_bot, owner, - &repo_name, - pr, + &repo, + &pr, bot_config, requested_by, None, + match cmd { + MergeCommentCommand::Normal => true, + MergeCommentCommand::Force => false, + }, ) - .await??; + .await?; match cmd { MergeCommentCommand::Normal => { - if ready_to_merge(github_bot, owner, &repo_name, pr).await? - { - prepare_to_merge( - github_bot, - owner, - &repo_name, - pr.number, - &pr.html_url, - ) - .await?; + if ready_to_merge(github_bot, owner, &repo, &pr).await? { match merge( github_bot, owner, - &repo_name, - pr, + &repo, + &pr, bot_config, requested_by, None, @@ -566,15 +610,22 @@ async fn handle_comment( Err(Error::MergeFailureWillBeSolvedLater { msg, }) => { - let _ = create_merge_request( + let msg = format!( + "This PR cannot be merged **at the moment** due to: {}\n\nprocessbot expects that the problem will be solved automatically later and so the auto-merge process will be started. You can simply wait for now.\n\n", + msg + ); + wait_to_merge( + github_bot, owner, - &repo_name, + &repo, pr.number, &pr.html_url, requested_by, - &pr.head_sha()?, + pr_head_sha, db, - ); + Some(&msg), + ) + .await?; return Err( Error::MergeFailureWillBeSolvedLater { msg, @@ -585,34 +636,26 @@ async fn handle_comment( _ => (), } } else { - let pr_head_sha = pr.head_sha()?; wait_to_merge( github_bot, owner, - &repo_name, + &repo, pr.number, &pr.html_url, requested_by, pr_head_sha, db, + None, ) .await?; return Ok(()); } } MergeCommentCommand::Force => { - prepare_to_merge( - github_bot, - owner, - &repo_name, - pr.number, - &pr.html_url, - ) - .await?; match merge( github_bot, owner, - &repo_name, + &repo, &pr, bot_config, requested_by, @@ -625,16 +668,16 @@ async fn handle_comment( Err(Error::MergeFailureWillBeSolvedLater { msg }) => { return Err(Error::Merge { source: Box::new(Error::Message { msg }), - commit_sha: pr.head_sha()?.to_owned(), + commit_sha: pr_head_sha.to_owned(), pr_url: pr.html_url.to_owned(), - owner: owner.to_string(), - repo_name: repo_name.to_string(), + owner: owner.to_owned(), + repo_name: repo.to_owned(), pr_number: pr.number, created_approval_id: None, } .map_issue(( - owner.to_string(), - repo_name.to_string(), + owner.to_owned(), + repo.to_owned(), pr.number, ))) } @@ -644,8 +687,8 @@ async fn handle_comment( } } - merge_companions(github_bot, bot_config, &repo_name, &pr, db) - .await?; + db.delete(pr_head_sha.as_bytes()).context(Db)?; + merge_companions(github_bot, bot_config, &repo, &pr, db).await } CommentCommand::CancelMerge => { log::info!("Deleting merge request for {}", html_url); @@ -653,17 +696,23 @@ async fn handle_comment( let pr_head_sha = pr.head_sha()?; db.delete(pr_head_sha.as_bytes()).context(Db)?; - let _ = github_bot + if let Err(err) = github_bot .create_issue_comment( owner, - &repo_name, + &repo, pr.number, "Merge cancelled.", ) .await - .map_err(|e| { - log::error!("Error posting comment: {}", e); - }); + { + log::error!( + "Failed to post comment on {} due to {}", + html_url, + err + ); + } + + Ok(()) } CommentCommand::Rebase => { if let PullRequest { @@ -684,32 +733,30 @@ async fn handle_comment( .. } = pr.clone() { - let _ = - github_bot - .create_issue_comment( - owner, - &repo_name, - pr.number, - "Rebasing.", - ) - .await - .map_err(|e| { - log::error!("Error posting comment for notifying rebase: {}", e); - }); + if let Err(err) = github_bot + .create_issue_comment(owner, &repo, pr.number, "Rebasing") + .await + { + log::error!( + "Failed to post comment on {} due to {}", + html_url, + err + ); + } rebase( github_bot, owner, - &repo_name, + &repo, &head_owner, &head_repo, &head_branch, ) - .await?; + .await } else { - return Err(Error::Message { - msg: format!("PR is missing some API data"), - }); + Err(Error::Message { + msg: "This PR is missing some API data".to_owned(), + }) } } CommentCommand::BurninRequest => { @@ -719,17 +766,17 @@ async fn handle_comment( &state.matrix_bot, owner, requested_by, - &repo_name, + &repo, &pr, ) - .await?; + .await } - CommentCommand::CompareReleaseRequest => match repo_name.as_str() { + CommentCommand::CompareReleaseRequest => match repo { "polkadot" => { let pr_head_sha = pr.head_sha()?; - let rel = github_bot.latest_release(owner, &repo_name).await?; + let rel = github_bot.latest_release(owner, &repo).await?; let release_tag = - github_bot.tag(owner, &repo_name, &rel.tag_name).await?; + github_bot.tag(owner, &repo, &rel.tag_name).await?; let release_substrate_commit = github_bot .substrate_commit_from_polkadot_commit( &release_tag.object.sha, @@ -745,23 +792,103 @@ async fn handle_comment( &branch_substrate_commit, ); log::info!("Posting link to substrate diff: {}", &link); - let _ = github_bot - .create_issue_comment(owner, &repo_name, number, &link) + if let Err(err) = github_bot + .create_issue_comment(owner, &repo, number, &link) .await - .map_err(|e| { - log::error!("Error posting comment: {}", e); - }); - } - _ => { - return Err(Error::Message { - msg: "This command can't be requested from this repository" - .to_string(), - }) + { + log::error!( + "Failed to post comment on {} due to {}", + html_url, + err + ); + } + Ok(()) } + _ => Err(Error::Message { + msg: "This command can't be requested from this repository" + .to_string(), + }), }, + } +} + +/// Parse bot commands in pull request comments. Commands are listed in README.md. +/// The first member of the returned tuple is the relevant commit SHA to invalidate from the +/// database in case of errors. +/// The second member of the returned tuple is the result of handling the parsed command. +async fn handle_comment( + body: &str, + requested_by: &str, + number: i64, + html_url: &str, + repo_url: &str, + state: &AppState, +) -> (Option, Result<()>) { + let cmd = match parse_bot_comment_from_text(body) { + Some(cmd) => cmd, + None => return (None, Ok(())), }; + log::info!("{:?} requested by {}", cmd, requested_by); - Ok(()) + let AppState { github_bot, .. } = state; + + let (owner, repo, pr) = match async { + let owner = + GithubBot::owner_from_html_url(html_url).context(Message { + msg: format!("Failed parsing owner in url: {}", html_url), + })?; + + let repo = repo_url.rsplit('/').next().context(Message { + msg: format!("Failed parsing repo name in url: {}", repo_url), + })?; + + let auth = + GithubUserAuthenticator::new(requested_by, owner, &repo, number); + auth.check_org_membership(&github_bot).await?; + + if let CommentCommand::Merge(_) = cmd { + // 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(4096)).await; + }; + + let pr = github_bot.pull_request(owner, &repo, number).await?; + + Ok((owner, repo, pr)) + } + .await + { + Ok(value) => value, + Err(err) => return (None, Err(err)), + }; + + let result = handle_command( + state, + &cmd, + owner, + repo, + number, + html_url, + requested_by, + &pr, + ) + .await; + let sha = match cmd { + CommentCommand::Merge(_) => { + pr.head_sha().ok().map(|head_sha| head_sha.to_owned()) + } + _ => None, + }; + + (sha, result) } async fn handle_burnin_request( @@ -867,7 +994,8 @@ async fn merge_allowed( bot_config: &BotConfig, requested_by: &str, min_approvals_required: Option, -) -> Result>> { + allow_pending_required_status_for_companions: bool, +) -> Result> { let is_mergeable = pr.mergeable.unwrap_or(false); if let Some(min_approvals_required) = &min_approvals_required { @@ -881,255 +1009,234 @@ 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) => { - let mut errors: Vec = Vec::new(); - - // Consider only the latest relevant review submitted per user - let mut latest_reviews: HashMap = - HashMap::new(); - for review in reviews { - // Do not consider states such as "Commented" as having invalidated a previous - // approval. Note: this assumes approvals are not invalidated on comments or - // pushes. - if review - .state - .as_ref() - .map(|state| { - *state == ReviewState::Approved - || *state == ReviewState::ChangesRequested - }) - .unwrap_or(false) + let result = async { + if let Err(err) = + check_all_companions_are_mergeable(github_bot, &pr, &repo_name) + .await + { + match err { + Error::InvalidCompanionStatus { ref status, .. } => { + match (status, allow_pending_required_status_for_companions) { - if let Some(user) = review.user.as_ref() { - if latest_reviews - .get(&user.login) - .map(|(prev_id, _)| *prev_id < review.id) - .unwrap_or(true) - { - let user_login = (&user.login).to_owned(); - latest_reviews - .insert(user_login, (review.id, review)); - } - } + (Status::Pending, true) => (), + _ => return Err(err), } } - let approved_reviews = latest_reviews - .values() - .filter_map(|(_, review)| { - if review.state == Some(ReviewState::Approved) { - Some(review) - } else { - None - } - }) - .collect::>(); + err => return Err(err), + } + } - let team_leads = github_bot - .substrate_team_leads(owner) - .await - .unwrap_or_else(|e| { - let msg = format!( - "Error getting {}: `{}`", - SUBSTRATE_TEAM_LEADS_GROUP, e - ); - log::error!("{}", msg); - errors.push(msg); - vec![] - }); - let lead_approvals = approved_reviews - .iter() - .filter(|review| { - team_leads.iter().any(|team_lead| { - review - .user - .as_ref() - .map(|user| user.login == team_lead.login) - .unwrap_or(false) - }) - }) - .count(); + if !is_mergeable && min_approvals_required.is_none() { + return Err(Error::Message { + msg: format!( + "Github API says {} is not mergeable", + pr.html_url + ), + }); + } - let core_devs = - github_bot.core_devs(owner).await.unwrap_or_else(|e| { - let msg = format!( - "Error getting {}: `{}`", - CORE_DEVS_GROUP, e - ); - log::error!("{}", msg); - errors.push(msg); - vec![] - }); - let core_approvals = approved_reviews - .iter() - .filter(|review| { - core_devs.iter().any(|core_dev| { - review - .user - .as_ref() - .map(|user| user.login == core_dev.login) - .unwrap_or(false) - }) + // Consider only the latest relevant review submitted per user + let latest_reviews = { + let reviews = github_bot.reviews(&pr.url).await?; + let mut latest_reviews = HashMap::new(); + for review in reviews { + // Do not consider states such as "Commented" as having invalidated a previous + // approval. Note: this assumes approvals are not invalidated on comments or + // pushes. + if review + .state + .as_ref() + .map(|state| { + *state == ReviewState::Approved + || *state == ReviewState::ChangesRequested }) - .count(); - - let relevant_approvals_count = - if core_approvals > lead_approvals { - core_approvals - } else { - lead_approvals - }; - - let relevant_approvals_count = if team_leads - .iter() - .any(|lead| lead.login == requested_by) + .unwrap_or(false) { - log::info!( - "{} merge requested by a team lead.", - pr.html_url - ); - Ok(relevant_approvals_count) + if let Some(user) = review.user.as_ref() { + if latest_reviews + .get(&user.login) + .map(|(prev_id, _)| *prev_id < review.id) + .unwrap_or(true) + { + let user_login = (&user.login).to_owned(); + latest_reviews + .insert(user_login, (review.id, review)); + } + } + } + } + latest_reviews + }; + + let approved_reviews = latest_reviews + .values() + .filter_map(|(_, review)| { + if review.state == Some(ReviewState::Approved) { + Some(review) } else { - let min_reviewers = if pr - .labels - .iter() - .find(|label| label.name.contains("insubstantial")) - .is_some() - { - 1 - } else { - bot_config.min_reviewers - }; + None + } + }) + .collect::>(); - let core_approved = core_approvals >= min_reviewers; - let lead_approved = lead_approvals >= 1; + let mut errors: Vec = Vec::new(); - if core_approved || lead_approved { - log::info!( - "{} has core or team lead approval.", - pr.html_url - ); - Ok(relevant_approvals_count) + let team_leads = github_bot + .substrate_team_leads(owner) + .await + .unwrap_or_else(|e| { + let msg = format!( + "Error getting {}: `{}`", + SUBSTRATE_TEAM_LEADS_GROUP, e + ); + log::error!("{}", msg); + errors.push(msg); + vec![] + }); + let lead_approvals = approved_reviews + .iter() + .filter(|review| { + team_leads.iter().any(|team_lead| { + review + .user + .as_ref() + .map(|user| user.login == team_lead.login) + .unwrap_or(false) + }) + }) + .count(); + + let core_devs = github_bot.core_devs(owner).await.unwrap_or_else(|e| { + let msg = format!("Error getting {}: `{}`", CORE_DEVS_GROUP, e); + log::error!("{}", msg); + errors.push(msg); + vec![] + }); + let core_approvals = approved_reviews + .iter() + .filter(|review| { + core_devs.iter().any(|core_dev| { + review + .user + .as_ref() + .map(|user| user.login == core_dev.login) + .unwrap_or(false) + }) + }) + .count(); + + let relevant_approvals_count = if core_approvals > lead_approvals { + core_approvals + } else { + lead_approvals + }; + + let relevant_approvals_count = if team_leads + .iter() + .any(|lead| lead.login == requested_by) + { + log::info!("{} merge requested by a team lead.", pr.html_url); + Ok(relevant_approvals_count) + } else { + let min_reviewers = if pr + .labels + .iter() + .find(|label| label.name.contains("insubstantial")) + .is_some() + { + 1 + } else { + bot_config.min_reviewers + }; + + let core_approved = core_approvals >= min_reviewers; + let lead_approved = lead_approvals >= 1; + + if core_approved || lead_approved { + log::info!("{} has core or team lead approval.", pr.html_url); + Ok(relevant_approvals_count) + } else { + let (process, process_warnings) = process::get_process( + github_bot, owner, repo_name, pr.number, + ) + .await?; + + let project_owner_approved = + approved_reviews.iter().rev().any(|review| { + review + .user + .as_ref() + .map(|user| process.is_owner(&user.login)) + .unwrap_or(false) + }); + let project_owner_requested = process.is_owner(requested_by); + + if project_owner_approved || project_owner_requested { + log::info!("{} has project owner approval.", pr.html_url); + Ok(relevant_approvals_count) + } else { + errors.extend(process_warnings); + if process.is_empty() { + Err(Error::ProcessInfo { + errors: Some(errors), + }) } else { - match process::get_process( - github_bot, owner, repo_name, pr.number, - ) - .await - { - Ok((process, process_warnings)) => { - let project_owner_approved = approved_reviews - .iter() - .rev() - .any(|review| { - review - .user - .as_ref() - .map(|user| { - process.is_owner(&user.login) - }) - .unwrap_or(false) - }); - let project_owner_requested = - process.is_owner(requested_by); - - if project_owner_approved - || project_owner_requested - { - log::info!( - "{} has project owner approval.", - pr.html_url - ); - Ok(relevant_approvals_count) - } else { - errors.extend(process_warnings); - if process.is_empty() { - Err(Error::ProcessInfo { - errors: Some(errors), - }) - } else { - Err(Error::Approval { - errors: Some(errors), - }) - } - } - } - Err(e) => Err(Error::ProcessFile { - source: Box::new(e), - }), - } - } - }; - - match relevant_approvals_count { - Ok(relevant_approvals_count) => { - Ok(match min_approvals_required { - Some(min_approvals_required) => { - let has_bot_approved = - approved_reviews.iter().any(|review| { - review - .user - .as_ref() - .map(|user| { - user.type_field - .as_ref() - .map(|type_field| { - *type_field - == UserType::Bot - }) - .unwrap_or(false) - }) - .unwrap_or(false) - }); - - // If the bot has already approved, then approving again will not make a - // difference. - if !has_bot_approved - && relevant_approvals_count + 1 - == min_approvals_required - { - if team_leads.iter().any(|team_lead| { - team_lead.login == requested_by - }) { - Ok(Some("a team lead".to_string())) - } else { - process::get_process( - github_bot, owner, repo_name, - pr.number, - ) - .await - .map(|(process, _)| { - if process.is_owner(requested_by) { - Some( - "a project owner" - .to_string(), - ) - } else { - None - } - }) - } - } else { - Ok(None) - } - } - _ => Ok(None), + Err(Error::Approval { + errors: Some(errors), }) } - Err(e) => Err(e), } } - Err(e) => Err(e), + }?; + + let min_approvals_required = match min_approvals_required { + Some(min_approvals_required) => min_approvals_required, + None => return Ok(None), + }; + + let has_bot_approved = approved_reviews.iter().any(|review| { + review + .user + .as_ref() + .map(|user| { + user.type_field + .as_ref() + .map(|type_field| *type_field == UserType::Bot) + .unwrap_or(false) + }) + .unwrap_or(false) + }); + + // If the bot has already approved, then approving again will not make a + // difference. + if has_bot_approved + // If the bot's approval is not enough to reach the minimum, then don't bother with approving + || relevant_approvals_count + 1 != min_approvals_required + { + return Ok(None); } - } else { - Err(Error::Message { - msg: format!("Github API says {} is not mergeable", pr.html_url), - }) - } - .map_err(|e| { - e.map_issue((owner.to_string(), repo_name.to_string(), pr.number)) + + let role = if team_leads + .iter() + .any(|team_lead| team_lead.login == requested_by) + { + Some("a team lead".to_string()) + } else { + let (process, _) = + process::get_process(github_bot, owner, repo_name, pr.number) + .await?; + if process.is_owner(requested_by) { + Some("a project owner".to_string()) + } else { + None + } + }; + + Ok(role) + }; + + result.await.map_err(|err| { + err.map_issue((owner.to_string(), repo_name.to_string(), pr.number)) }) } @@ -1151,6 +1258,7 @@ pub async fn ready_to_merge( repo_name, pr_head_sha, &pr.html_url, + None, ) .await { @@ -1162,6 +1270,7 @@ pub async fn ready_to_merge( repo_name, pr_head_sha, &pr.html_url, + None, ) .await { @@ -1193,33 +1302,30 @@ pub async fn ready_to_merge( /// Create a merge request object. /// /// If this has been called, error handling must remove the db entry. -async fn create_merge_request( +async fn register_merge_request( owner: &str, - repo_name: &str, + repo: &str, number: i64, html_url: &str, requested_by: &str, commit_sha: &str, db: &DB, ) -> Result<()> { - let m = MergeRequest { + let mr = MergeRequest { owner: owner.to_string(), - repo_name: repo_name.to_string(), + repo_name: repo.to_string(), number: number, html_url: html_url.to_string(), requested_by: requested_by.to_string(), }; - log::info!("Serializing merge request: {:?}", m); - let bytes = bincode::serialize(&m).context(Bincode).map_err(|e| { - e.map_issue((owner.to_string(), repo_name.to_string(), number)) - })?; - log::info!("Writing merge request to db (head sha: {})", commit_sha); - db.put(commit_sha.trim().as_bytes(), bytes) - .context(Db) - .map_err(|e| { - e.map_issue((owner.to_string(), repo_name.to_string(), number)) - })?; - Ok(()) + async { + log::info!("Serializing merge request: {:?}", mr); + let bytes = bincode::serialize(&mr).context(Bincode)?; + log::info!("Writing merge request to db (head sha: {})", commit_sha); + db.put(commit_sha.trim().as_bytes(), bytes).context(Db) + } + .await + .map_err(|err| err.map_issue((owner.to_string(), repo.to_string(), number))) } /// Create a merge request, add it to the database, and post a comment stating the merge is @@ -1227,17 +1333,17 @@ async fn create_merge_request( pub async fn wait_to_merge( github_bot: &GithubBot, owner: &str, - repo_name: &str, + repo: &str, number: i64, html_url: &str, requested_by: &str, commit_sha: &str, db: &DB, + msg: Option<&str>, ) -> Result<()> { - log::info!("{} checks incomplete.", html_url); - create_merge_request( + register_merge_request( owner, - repo_name, + repo, number, html_url, requested_by, @@ -1245,36 +1351,21 @@ pub async fn wait_to_merge( db, ) .await?; - log::info!("Waiting for commit status."); - let _ = github_bot - .create_issue_comment( - owner, - &repo_name, - number, - "Waiting for commit status.", + + let comment = { + let msg = msg.unwrap_or("Waiting for commit status."); + format!( + "{} If your PR has companions, processbot will also wait until the companion's required statuses are passing.", + msg ) - .await - .map_err(|e| { - log::error!("Error posting comment: {}", e); - }); - Ok(()) -} + }; + let post_comment_result = github_bot + .create_issue_comment(owner, &repo, number, &comment) + .await; + if let Err(err) = post_comment_result { + log::error!("Error posting comment: {}", err); + } -/// Post a comment stating the merge will be attempted. -async fn prepare_to_merge( - github_bot: &GithubBot, - owner: &str, - repo_name: &str, - number: i64, - html_url: &str, -) -> Result<()> { - log::info!("Trying merge of {}", html_url); - let _ = github_bot - .create_issue_comment(owner, &repo_name, number, "Trying merge.") - .await - .map_err(|e| { - log::error!("Error posting comment: {}", e); - }); Ok(()) } @@ -1359,51 +1450,48 @@ pub async fn merge( bot_config, requested_by, Some(min_approvals_required), + false ) .await { - Ok(result) => match result { - Ok(requester_role) => match requester_role { - Some(requester_role) => { - let _ = github_bot - .create_issue_comment( - owner, - &repo_name, - pr.number, - &format!( - "Bot will approve on the behalf of @{}, since they are {}, in an attempt to reach the minimum approval count", - requested_by, - requester_role, - ), - ) - .await - .map_err(|e| { - log::error!("Error posting comment: {}", e); - }); - match github_bot.approve_merge_request( + Ok(requester_role) => match requester_role { + Some(requester_role) => { + if let Err(err) = github_bot + .create_issue_comment( owner, - repo_name, - pr.number - ).await { - Ok(review) => merge( - github_bot, - owner, - repo_name, - pr, - bot_config, + &repo_name, + pr.number, + &format!( + "Bot will approve on the behalf of @{}, since they are {}, in an attempt to reach the minimum approval count", requested_by, - Some(review.id) - ).await, - Err(e) => Err(e) - } - }, - None => Err(Error::Message { - msg: "Requester's approval is not enough to make the PR mergeable".to_string() - }), + requester_role, + ), + ) + .await { + log::error!("Failed to post comment on {} due to {}", pr.html_url, err); + } + match github_bot.approve_merge_request( + owner, + repo_name, + pr.number + ).await { + Ok(review) => merge( + github_bot, + owner, + repo_name, + pr, + bot_config, + requested_by, + Some(review.id) + ).await, + Err(e) => Err(e) + } }, - Err(e) => Err(e) + None => Err(Error::Message { + msg: "Requester's approval is not enough to make the PR mergeable".to_string() + }), }, - Err(e) => Err(e), + Err(e) => Err(e) }.map_err(|e| Error::Message { msg: format!( "Could not recover from: `{}` due to: `{}`", @@ -1530,7 +1618,13 @@ async fn performance_regression( Ok(()) } -const TROUBLESHOOT_MSG: &str = "Merge failed. Check out the [criteria for merge](https://github.com/paritytech/parity-processbot#criteria-for-merge)."; +fn get_troubleshoot_msg() -> String { + return format!( + "Merge failed. Check out the [criteria for merge](https://github.com/paritytech/parity-processbot#criteria-for-merge). If you're not meeting the approval count, check if the approvers are members of {} or {}", + SUBSTRATE_TEAM_LEADS_GROUP, + CORE_DEVS_GROUP + ); +} fn display_errors_along_the_way(errors: Option>) -> String { errors @@ -1539,7 +1633,7 @@ fn display_errors_along_the_way(errors: Option>) -> String { "".to_string() } else { format!( - "The following errors *might* have affected the outcome of this attempt:\n{}", + "The following errors **might** have affected the outcome of this attempt:\n{}", errors.iter().map(|e| format!("- {}", e)).join("\n") ) } @@ -1547,125 +1641,119 @@ fn display_errors_along_the_way(errors: Option>) -> String { .unwrap_or_else(|| "".to_string()) } -async fn handle_error_inner(err: Error, state: &AppState) -> Option { +#[async_recursion] +async fn handle_error_inner(err: Error, state: &AppState) -> String { match err { - Error::Merge { source, commit_sha, pr_url, owner, repo_name, pr_number, created_approval_id } => { - let _ = state.db.delete(commit_sha.as_bytes()).map_err(|e| { - log::error!("Error deleting merge request from db: {}", e); - }); - let github_bot = &state.github_bot; - if let Some(created_approval_id) = created_approval_id { - let _ = github_bot.clear_merge_request_approval( - &owner, - &repo_name, - pr_number, - created_approval_id - ).await.map_err( - |e| log::error!("Failed to cleanup a bot review in {} due to: {}", pr_url, e) + Error::Merge { + source, + commit_sha, + pr_url, + owner, + repo_name, + pr_number, + created_approval_id, + } => { + if let Err(db_err) = state.db.delete(commit_sha.as_bytes()) { + log::error!( + "Failed to delete {} from database due to {:?}", + commit_sha, + db_err ); } - match *source { - Error::Response { - ref body, - ref status - } => Some(format!("Merge failed with response status: {} and body: 
{}
", status, html_escape::encode_safe(&body.to_string()))), - Error::Http { source, .. } => { - Some(format!("Merge failed due to network error:\n\n{}", source)) - } - Error::Message { .. } => { - Some(format!("Merge failed: {}", *source)) + + let github_bot = &state.github_bot; + if let Some(created_approval_id) = created_approval_id { + if let Err(clear_err) = github_bot + .clear_merge_request_approval( + &owner, + &repo_name, + pr_number, + created_approval_id, + ) + .await + { + log::error!( + "Failed to cleanup a bot review in {} due to {}", + pr_url, + clear_err + ) } - _ => Some("Merge failed due to unexpected error".to_string()), } + + handle_error_inner(*source, state).await } - Error::ProcessFile { source } => match *source { - Error::Response { - ref body, - ref status - } => Some(format!("Error getting {} (status code {}): 
{}
", PROCESS_FILE, status, html_escape::encode_safe(&body.to_string()))), - Error::Http { source, .. } => { - Some(format!("Network error getting {}:\n\n{}", PROCESS_FILE, source)) - } - _ => Some(format!("Unexpected error getting {}:\n\n{}", PROCESS_FILE, source)), - }, Error::ProcessInfo { errors } => { - Some( - format!( + format!( " Error: When trying to meet the \"Project Owners\" approval requirements: this pull request does not belong to a project defined in {}. Approval by \"Project Owners\" is only attempted if other means defined in the [criteria for merge](https://github.com/paritytech/parity-processbot#criteria-for-merge) are not satisfied first. +{} + {} ", PROCESS_FILE, display_errors_along_the_way(errors), + get_troubleshoot_msg() ) - ) - } - Error::Approval { errors } => { - Some( - format!( - "Error: Approval criteria was not satisfied.\n\n{}\n\n{}", - display_errors_along_the_way(errors), - TROUBLESHOOT_MSG - ) - ) - } - Error::HeadChanged { ref expected, .. } => { - let _ = state.db.delete(expected.as_bytes()).map_err(|e| { - log::error!("Error deleting merge request from db: {}", e); - }); - Some(format!("Merge aborted: {}", err)) - } - Error::ChecksFailed { ref commit_sha } => { - let _ = state.db.delete(commit_sha.as_bytes()).map_err(|e| { - log::error!("Error deleting merge request from db: {}", e); - }); - Some(format!("Merge aborted: {}", err)) } + Error::Approval { errors } => format!( + "Approval criteria was not satisfied.\n\n{}\n\n{}", + display_errors_along_the_way(errors), + get_troubleshoot_msg() + ), Error::Response { ref body, - ref status - } => Some(format!("Response error (status {}): 
{}
", status, html_escape::encode_safe(&body.to_string()))), - Error::Message { .. } => { - Some(format!("Error: {}", err)) - } - _ => None + ref status, + } => format!( + "Response error (status {}): 
{}
", + status, + html_escape::encode_safe(&body.to_string()) + ), + _ => format!("{}", err), } } -async fn handle_error(e: Error, state: &AppState) { - log::info!("handle_error: {}", e); - match e { +async fn handle_error( + merge_cancel_outcome: MergeCancelOutcome, + err: Error, + state: &AppState, +) { + log::info!("handle_error: {}", err); + match err { Error::MergeFailureWillBeSolvedLater { .. } => (), - e => match e { + err => match err { Error::WithIssue { source, issue: (owner, repo, number), .. } => match *source { Error::MergeFailureWillBeSolvedLater { .. } => (), - e => { - let msg = handle_error_inner(e, state) - .await - .unwrap_or_else(|| { - format!( - "Unexpected error (at {} server time).", - chrono::Utc::now().to_string() - ) - }); - let _ = state + err => { + let msg = { + let description = handle_error_inner(err, state).await; + let caption = match merge_cancel_outcome { + MergeCancelOutcome::ShaDidNotExist => "", + MergeCancelOutcome::WasCancelled => "Merge cancelled due to error.", + MergeCancelOutcome::WasNotCancelled => "Some error happened, but the merge was not cancelled.", + }; + format!("{} Error: {}", caption, description) + }; + if let Err(comment_post_err) = state .github_bot .create_issue_comment(&owner, &repo, number, &msg) .await - .map_err(|e| { - log::error!("Error posting comment: {}", e); - }); + { + log::error!( + "Error posting comment: {}", + comment_post_err + ); + } } }, _ => { - handle_error_inner(e, state).await; + handle_error_inner(err, state).await; } }, } From de5db2cc228f5284a3ea3e20dc43f35266e78460 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sat, 9 Oct 2021 13:55:27 -0300 Subject: [PATCH 02/45] wip --- src/companion.rs | 321 ++++++++++++++++++++++------------- src/constants.rs | 2 + src/error.rs | 4 +- src/github.rs | 1 + src/github_bot/repository.rs | 16 -- src/webhook.rs | 203 +++++++++------------- 6 files changed, 287 insertions(+), 260 deletions(-) diff --git a/src/companion.rs b/src/companion.rs index a7ec0ba3..27465a93 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -1,5 +1,4 @@ use regex::RegexBuilder; -use rocksdb::DB; use snafu::ResultExt; use std::{ collections::HashMap, collections::HashSet, iter::FromIterator, path::Path, @@ -9,14 +8,14 @@ use tokio::time::delay_for; use crate::{ cmd::*, - config::BotConfig, - constants::MAIN_REPO_FOR_STAGING, + constants::{BOT_NAME_FOR_COMMITS, MAIN_REPO_FOR_STAGING}, error::*, github::*, github_bot::GithubBot, webhook::{ get_latest_checks_state, get_latest_statuses_state, merge, - ready_to_merge, wait_to_merge, + ready_to_merge, wait_to_merge, AppState, MergeRequest, + MergeRequestReference, }, Result, Status, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, COMPANION_SHORT_REGEX, PR_HTML_URL_REGEX, @@ -329,7 +328,7 @@ fn companion_parse_short(body: &str) -> Option { Some((html_url, owner, repo, number)) } -fn parse_all_companions(body: &str) -> Vec { +pub fn parse_all_companions(body: &str) -> Vec { body.lines().filter_map(companion_parse).collect() } @@ -343,6 +342,11 @@ pub async fn check_all_companions_are_mergeable( for (html_url, owner, repo, number) in parse_all_companions(body) { let companion = github_bot.pull_request(&owner, &repo, number).await?; + + if companion.merged { + continue; + } + let head_sha = companion.head_sha()?; let has_user_owner = companion @@ -377,26 +381,12 @@ pub async fn check_all_companions_are_mergeable( }); } - let target_branch = github_bot - .branch( - &companion.base.repo.owner.login, - &companion.base.repo.name, - &companion.base.ref_field, - ) - .await?; - let statuses = get_latest_statuses_state( github_bot, &owner, &repo, head_sha, &pr.html_url, - Some( - &target_branch - .protection - .required_status_checks - .contexts, - ), ) .await?; match statuses { @@ -427,12 +417,6 @@ pub async fn check_all_companions_are_mergeable( &repo, &head_sha, &pr.html_url, - Some( - &target_branch - .protection - .required_status_checks - .contexts, - ), ) .await?; match checks { @@ -463,17 +447,112 @@ pub async fn check_all_companions_are_mergeable( Ok(()) } +async fn trigger_merge_through_parent( + state: &AppState, + companion: &MergeRequestReference, +) -> bool { + let AppState { db, github_bot, .. } = state; + + let mut triggered_through_parent: Option = None; + let mut required_in_parent = false; + + let mut iter = db.iterator(IteratorMode::Start); + for (sha, value) in iter { + let mr: MergeRequest = + match bincode::deserialize(&value).context(Bincode) { + Ok(mr) => mr, + Err(err) => { + log::error!( + "Failed to deserialize {} during when_companion_ready", + String::from_utf8_lossy(sha) + ); + continue; + } + }; + + log::info!("Deserialized merge request: {:?}", mr); + + let companion_children = match mr.companion_children { + Some(companion_children) => companion_children, + None => continue, + }; + + for child in companion_children { + if child.owner == companion.owner + && child.repo == companion.repo + && child.number == companion.number + { + match async { + let pr = github_bot + .pull_request(mr.owner, mr.repo_name, mr.number) + .await?; + + if pr.merged { + return Ok(None); + } + + let companions = + match pr.body.map(|body| parse_all_companions(body)) { + Some(companions) => companions, + None => return Ok(None), + }; + if companions.iter().find(|parsed| parsed).is_none() { + return Ok(None); + } + + check_all_companions_are_mergeable( + github_bot, + &pr, + mr.repo_name, + ) + .await?; + merge( + github_bot, + owner, + repo, + &pr, + bot_config, + BOT_NAME_FOR_COMMITS, + None, + ) + .await??; + db.delete(&sha).context(Db)?; + merge_companions(state, &repo, &pr).await + + Ok(Some(pr.html_url)) + } + .await { + Ok(Some(result)) => { + triggered_through_parent = Some(result); + break; + }, + _ => None + } + } + } + } + + merge_was_triggered +} + async fn update_then_merge_companion( - github_bot: &GithubBot, - bot_config: &BotConfig, - db: &DB, + state: &AppState, html_url: &str, owner: &str, repo: &str, number: &i64, merge_done_in: &str, ) -> Result<()> { + let AppState { + github_bot, + bot_config, + .. + } = state; + let pr = github_bot.pull_request(&owner, &repo, *number).await?; + if pr.merged { + return Ok(()); + } if let PullRequest { head: @@ -516,21 +595,23 @@ async fn update_then_merge_companion( repo, &pr, bot_config, - "parity-processbot[bot]", + BOT_NAME_FOR_COMMITS, 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]"), + state, &updated_sha, - db, + &MergeRequest { + owner: owner.to_owned(), + repo_name: repo.to_owned(), + number: number.to_owned(), + html_url: html_url.to_owned(), + requested_by: BOT_NAME_FOR_COMMITS.to_owned(), + companion_children: None, + }, None, ) .await?; @@ -545,102 +626,100 @@ async fn update_then_merge_companion( } pub async fn merge_companions( - github_bot: &GithubBot, - bot_config: &BotConfig, + state: &AppState, 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 { + return Ok(()); + } - if merge_done_in == "substrate" || merge_done_in == MAIN_REPO_FOR_STAGING { - log::info!("Checking for companion."); + log::info!("Checking for companion in {}", pr.html_url); - if let Some(body) = pr.body.as_ref() { - let companions = parse_all_companions(body); - if companions.is_empty() { - log::info!("No companion found."); + let companions_groups = { + let body = match pr.body.as_ref() { + Some(body) => body, + None => return Ok(()), + }; + + let companions = parse_all_companions(body); + if companions.is_empty() { + log::info!("No companion found."); + return Ok(()); + } + + 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 { - 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 - }; + companions_groups.insert(key, vec![comp]); + } + } - 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 + companions_groups + }; + + let AppState { github_bot, .. } = state; + + let mut remaining_futures = companions_groups + .into_values() + .map(|group| { + Box::pin(async move { + let mut errors: Vec = vec![]; + + for (html_url, owner, repo, ref number) in group { + if let Err(err) = update_then_merge_companion( + state, + &html_url, + &owner, + &repo, + number, + merge_done_in, + ) + .await { - 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)); + 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), + }); } - remaining_futures = next_remaining_futures; } - } + + errors + }) + }) + .collect::>(); + + let mut errors: Vec = vec![]; + 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() { diff --git a/src/constants.rs b/src/constants.rs index 6f9b51b9..24a2f0ae 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -5,3 +5,5 @@ pub const CORE_DEVS_GROUP: &str = "core-devs"; pub const PROCESS_FILE: &str = "Process.json"; pub const MAIN_REPO_FOR_STAGING: &str = "main-for-processbot-staging"; + +pub const BOT_NAME_FOR_COMMITS: &str = "parity-processbot[bot]"; diff --git a/src/error.rs b/src/error.rs index 27241108..1d888c2d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,10 +1,10 @@ use crate::Status; use snafu::Snafu; -// TODO this really should be struct { repository, owner, number } +// TODO this really should be struct { owner, repo, number } pub type IssueDetails = (String, String, i64); -// TODO this really should be struct { repository_url, repository, owner, number } +// TODO this really should be struct { repository_url, owner, repo, number } pub type IssueDetailsWithRepositoryURL = (String, String, String, i64); pub struct CompanionDetailsWithErrorMessage { diff --git a/src/github.rs b/src/github.rs index 2af8e94f..5875b178 100644 --- a/src/github.rs +++ b/src/github.rs @@ -35,6 +35,7 @@ pub struct PullRequest { pub head: Option, pub base: Base, pub repository: Option, + pub merged: bool, } impl HasIssueDetails for PullRequest { diff --git a/src/github_bot/repository.rs b/src/github_bot/repository.rs index e879d635..989266c6 100644 --- a/src/github_bot/repository.rs +++ b/src/github_bot/repository.rs @@ -19,22 +19,6 @@ impl GithubBot { ); self.client.get(url).await } - - pub async fn branch( - &self, - owner: &str, - repo: &str, - branch: &str, - ) -> Result { - let url = format!( - "{}/repos/{}/{}/branches/{}", - Self::BASE_URL, - owner, - repo, - branch - ); - self.client.get(url).await - } } /* diff --git a/src/webhook.rs b/src/webhook.rs index d6ef4941..3527c1fb 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -12,11 +12,11 @@ use std::{collections::HashMap, sync::Arc, time::Duration}; use tokio::{sync::Mutex, time::delay_for}; use crate::{ - auth::GithubUserAuthenticator, companion::*, config::BotConfig, - constants::*, error::*, github::*, github_bot::GithubBot, gitlab_bot::*, - matrix_bot::MatrixBot, performance, process, rebase::*, - utils::parse_bot_comment_from_text, vanity_service, CommentCommand, - MergeCancelOutcome, MergeCommentCommand, Result, Status, + auth::GithubUserAuthenticator, companion::parse_all_companions, + companion::*, config::BotConfig, constants::*, error::*, github::*, + github_bot::GithubBot, gitlab_bot::*, matrix_bot::MatrixBot, performance, + process, rebase::*, utils::parse_bot_comment_from_text, vanity_service, + CommentCommand, MergeCancelOutcome, MergeCommentCommand, Result, Status, }; /// This data gets passed along with each webhook to the webhook handler. @@ -33,12 +33,20 @@ pub struct AppState { /// This stores information about a pull request while we wait for checks to complete. #[derive(Debug, Serialize, Deserialize)] #[repr(C)] +pub struct MergeRequestReference { + pub owner: String, + pub repo: String, + pub number: i64, +} +#[derive(Debug, Serialize, Deserialize)] +#[repr(C)] pub struct MergeRequest { - owner: String, - repo_name: String, - number: i64, - html_url: String, - requested_by: String, + pub owner: String, + pub repo_name: String, + pub number: i64, + pub html_url: String, + pub requested_by: String, + pub companion_children: Option>, } /// Check the SHA1 signature on a webhook payload. @@ -300,13 +308,7 @@ async fn handle_check( state: &AppState, ) -> Result<()> { if status == CheckRunStatus::Completed { - checks_and_status( - &state.bot_config, - &state.github_bot, - &state.db, - &commit_sha, - ) - .await + checks_and_status(state, &commit_sha).await } else { Ok(()) } @@ -321,13 +323,7 @@ async fn handle_status( if status == StatusState::Pending { Ok(()) } else { - checks_and_status( - &state.bot_config, - &state.github_bot, - &state.db, - &commit_sha, - ) - .await + checks_and_status(state, commit_sha).await } } @@ -337,7 +333,6 @@ pub async fn get_latest_statuses_state( owner_repo: &str, commit_sha: &str, html_url: &str, - filter_context: Option<&Vec>, ) -> Result { let status = github_bot.status(&owner, &owner_repo, &commit_sha).await?; log::info!("{:?}", status); @@ -362,16 +357,6 @@ pub async fn get_latest_statuses_state( continue; } - if let Some(filter_context) = filter_context { - if !filter_context - .iter() - .find(|ctx| **ctx == s.context) - .is_some() - { - continue; - } - } - if latest_statuses .get(&s.context) .map(|(prev_id, _)| prev_id < &(&s).id) @@ -408,7 +393,6 @@ pub async fn get_latest_checks_state( repo_name: &str, commit_sha: &str, html_url: &str, - filter_name: Option<&Vec>, ) -> Result { let checks = github_bot .check_runs(&owner, &repo_name, commit_sha) @@ -422,11 +406,6 @@ pub async fn get_latest_checks_state( (i64, CheckRunStatus, Option), > = HashMap::new(); for c in checks.check_runs { - if let Some(filter_name) = filter_name { - if !filter_name.iter().find(|name| **name == c.name).is_some() { - continue; - } - } if latest_checks .get(&c.name) .map(|(prev_id, _, _)| prev_id < &(&c).id) @@ -457,12 +436,8 @@ pub async fn get_latest_checks_state( /// Check that no commit has been pushed since the merge request was received. Query checks and /// statuses and if they are green, attempt merge. -async fn checks_and_status( - bot_config: &BotConfig, - github_bot: &GithubBot, - db: &DB, - sha: &str, -) -> Result<()> { +async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { + let AppState { db, .. } = state; let pr_bytes = match db.get(sha.as_bytes()).context(Db)? { Some(pr_bytes) => pr_bytes, None => return Ok(()), @@ -480,6 +455,12 @@ async fn checks_and_status( requested_by, .. } = &mr; + let AppState { + github_bot, + db, + bot_config, + .. + } = state; let pr = github_bot.pull_request(owner, repo_name, *number).await?; let pr_head_sha = pr.head_sha()?; @@ -492,13 +473,13 @@ async fn checks_and_status( } let status = get_latest_statuses_state( - github_bot, &owner, &repo_name, sha, &html_url, None, + github_bot, &owner, &repo_name, sha, &html_url, ) .await?; match status { Status::Success => { let checks = get_latest_checks_state( - github_bot, &owner, &repo_name, sha, &html_url, None, + github_bot, &owner, &repo_name, sha, &html_url, ) .await?; match checks { @@ -528,10 +509,7 @@ async fn checks_and_status( { Ok(_) => { db.delete(&sha).context(Db)?; - merge_companions( - github_bot, bot_config, &repo_name, &pr, db, - ) - .await + merge_companions(state, &repo_name, &pr).await } Err(Error::MergeFailureWillBeSolvedLater { .. @@ -591,6 +569,24 @@ async fn handle_command( ) .await?; + let mr = MergeRequest { + owner: owner.to_owned(), + repo_name: repo.to_owned(), + number, + html_url: html_url.to_owned(), + requested_by: requested_by.to_owned(), + companion_children: pr.body.as_ref().map(|body| { + parse_all_companions(body) + .into_iter() + .map(|(_, owner, repo, number)| MergeRequestReference { + owner, + repo, + number, + }) + .collect() + }), + }; + match cmd { MergeCommentCommand::Normal => { if ready_to_merge(github_bot, owner, &repo, &pr).await? { @@ -615,14 +611,9 @@ async fn handle_command( msg ); wait_to_merge( - github_bot, - owner, - &repo, - pr.number, - &pr.html_url, - requested_by, + state, pr_head_sha, - db, + &mr, Some(&msg), ) .await?; @@ -636,18 +627,7 @@ async fn handle_command( _ => (), } } else { - wait_to_merge( - github_bot, - owner, - &repo, - pr.number, - &pr.html_url, - requested_by, - pr_head_sha, - db, - None, - ) - .await?; + wait_to_merge(state, pr_head_sha, &mr, None).await?; return Ok(()); } } @@ -688,7 +668,7 @@ async fn handle_command( } db.delete(pr_head_sha.as_bytes()).context(Db)?; - merge_companions(github_bot, bot_config, &repo, &pr, db).await + merge_companions(state, &repo, &pr).await } CommentCommand::CancelMerge => { log::info!("Deleting merge request for {}", html_url); @@ -1258,7 +1238,6 @@ pub async fn ready_to_merge( repo_name, pr_head_sha, &pr.html_url, - None, ) .await { @@ -1270,7 +1249,6 @@ pub async fn ready_to_merge( repo_name, pr_head_sha, &pr.html_url, - None, ) .await { @@ -1303,64 +1281,43 @@ pub async fn ready_to_merge( /// /// If this has been called, error handling must remove the db entry. async fn register_merge_request( - owner: &str, - repo: &str, - number: i64, - html_url: &str, - requested_by: &str, - commit_sha: &str, - db: &DB, + state: &AppState, + sha: &str, + mr: &MergeRequest, ) -> Result<()> { - let mr = MergeRequest { - owner: owner.to_string(), - repo_name: repo.to_string(), - number: number, - html_url: html_url.to_string(), - requested_by: requested_by.to_string(), - }; - async { - log::info!("Serializing merge request: {:?}", mr); - let bytes = bincode::serialize(&mr).context(Bincode)?; - log::info!("Writing merge request to db (head sha: {})", commit_sha); - db.put(commit_sha.trim().as_bytes(), bytes).context(Db) - } - .await - .map_err(|err| err.map_issue((owner.to_string(), repo.to_string(), number))) + let AppState { db, .. } = state; + log::info!("Serializing merge request: {:?}", mr); + let bytes = bincode::serialize(mr).context(Bincode)?; + log::info!("Writing merge request to db (sha: {})", sha); + db.put(sha.trim().as_bytes(), bytes).context(Db) } /// Create a merge request, add it to the database, and post a comment stating the merge is /// pending. pub async fn wait_to_merge( - github_bot: &GithubBot, - owner: &str, - repo: &str, - number: i64, - html_url: &str, - requested_by: &str, - commit_sha: &str, - db: &DB, + state: &AppState, + sha: &str, + mr: &MergeRequest, msg: Option<&str>, ) -> Result<()> { - register_merge_request( + register_merge_request(state, sha, mr).await?; + + let AppState { github_bot, .. } = state; + + let MergeRequest { owner, - repo, + repo_name, number, - html_url, - requested_by, - commit_sha, - db, - ) - .await?; + .. + } = mr; - let comment = { - let msg = msg.unwrap_or("Waiting for commit status."); - format!( - "{} If your PR has companions, processbot will also wait until the companion's required statuses are passing.", - msg - ) - }; let post_comment_result = github_bot - .create_issue_comment(owner, &repo, number, &comment) + .create_issue_comment( + owner, + repo_name, + *number, + msg.unwrap_or("Waiting for commit status."), + ) .await; if let Err(err) = post_comment_result { log::error!("Error posting comment: {}", err); @@ -1382,6 +1339,10 @@ pub async fn merge( requested_by: &str, created_approval_id: Option, ) -> Result> { + if pr.merged { + return Ok(Ok()); + } + match pr.head_sha() { Ok(pr_head_sha) => match github_bot .merge_pull_request(owner, repo_name, pr.number, pr_head_sha) From d09bb351748fd201e9bab7b0e35a0a29d33837f6 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 00:08:41 -0300 Subject: [PATCH 03/45] wip --- src/companion.rs | 92 +---------------- src/webhook.rs | 249 ++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 226 insertions(+), 115 deletions(-) diff --git a/src/companion.rs b/src/companion.rs index 27465a93..847654ca 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -15,7 +15,7 @@ use crate::{ webhook::{ get_latest_checks_state, get_latest_statuses_state, merge, ready_to_merge, wait_to_merge, AppState, MergeRequest, - MergeRequestReference, + MergeRequestBase, }, Result, Status, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, COMPANION_SHORT_REGEX, PR_HTML_URL_REGEX, @@ -447,94 +447,6 @@ pub async fn check_all_companions_are_mergeable( Ok(()) } -async fn trigger_merge_through_parent( - state: &AppState, - companion: &MergeRequestReference, -) -> bool { - let AppState { db, github_bot, .. } = state; - - let mut triggered_through_parent: Option = None; - let mut required_in_parent = false; - - let mut iter = db.iterator(IteratorMode::Start); - for (sha, value) in iter { - let mr: MergeRequest = - match bincode::deserialize(&value).context(Bincode) { - Ok(mr) => mr, - Err(err) => { - log::error!( - "Failed to deserialize {} during when_companion_ready", - String::from_utf8_lossy(sha) - ); - continue; - } - }; - - log::info!("Deserialized merge request: {:?}", mr); - - let companion_children = match mr.companion_children { - Some(companion_children) => companion_children, - None => continue, - }; - - for child in companion_children { - if child.owner == companion.owner - && child.repo == companion.repo - && child.number == companion.number - { - match async { - let pr = github_bot - .pull_request(mr.owner, mr.repo_name, mr.number) - .await?; - - if pr.merged { - return Ok(None); - } - - let companions = - match pr.body.map(|body| parse_all_companions(body)) { - Some(companions) => companions, - None => return Ok(None), - }; - if companions.iter().find(|parsed| parsed).is_none() { - return Ok(None); - } - - check_all_companions_are_mergeable( - github_bot, - &pr, - mr.repo_name, - ) - .await?; - merge( - github_bot, - owner, - repo, - &pr, - bot_config, - BOT_NAME_FOR_COMMITS, - None, - ) - .await??; - db.delete(&sha).context(Db)?; - merge_companions(state, &repo, &pr).await - - Ok(Some(pr.html_url)) - } - .await { - Ok(Some(result)) => { - triggered_through_parent = Some(result); - break; - }, - _ => None - } - } - } - } - - merge_was_triggered -} - async fn update_then_merge_companion( state: &AppState, html_url: &str, @@ -550,7 +462,7 @@ async fn update_then_merge_companion( } = state; let pr = github_bot.pull_request(&owner, &repo, *number).await?; - if pr.merged { + if check_cleanup_merged_pr(state, pr, None).await? { return Ok(()); } diff --git a/src/webhook.rs b/src/webhook.rs index 3527c1fb..9c9a4e3a 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -33,7 +33,7 @@ pub struct AppState { /// This stores information about a pull request while we wait for checks to complete. #[derive(Debug, Serialize, Deserialize)] #[repr(C)] -pub struct MergeRequestReference { +pub struct MergeRequestBase { pub owner: String, pub repo: String, pub number: i64, @@ -46,7 +46,7 @@ pub struct MergeRequest { pub number: i64, pub html_url: String, pub requested_by: String, - pub companion_children: Option>, + pub companion_children: Option>, } /// Check the SHA1 signature on a webhook payload. @@ -508,8 +508,10 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { .await? { Ok(_) => { - db.delete(&sha).context(Db)?; - merge_companions(state, &repo_name, &pr).await + attempt_merge_as_companion_fallback_merge( + state, pr, + ) + .await } Err(Error::MergeFailureWillBeSolvedLater { .. @@ -569,26 +571,27 @@ async fn handle_command( ) .await?; - let mr = MergeRequest { - owner: owner.to_owned(), - repo_name: repo.to_owned(), - number, - html_url: html_url.to_owned(), - requested_by: requested_by.to_owned(), - companion_children: pr.body.as_ref().map(|body| { - parse_all_companions(body) - .into_iter() - .map(|(_, owner, repo, number)| MergeRequestReference { - owner, - repo, - number, - }) - .collect() - }), - }; - match cmd { MergeCommentCommand::Normal => { + let mr = MergeRequest { + owner: owner.to_owned(), + repo_name: repo.to_owned(), + number, + html_url: html_url.to_owned(), + requested_by: requested_by.to_owned(), + companion_children: pr.body.as_ref().map(|body| { + parse_all_companions(body) + .into_iter() + .map(|(_, owner, repo, number)| { + MergeRequestBase { + owner, + repo, + number, + } + }) + .collect() + }), + }; if ready_to_merge(github_bot, owner, &repo, &pr).await? { match merge( github_bot, @@ -1326,20 +1329,71 @@ pub async fn wait_to_merge( Ok(()) } +pub async fn check_cleanup_merged_pr( + state: &AppState, + pr: &PullRequest, +) -> bool { + if !pr.merged { + return false; + } + + cleanup_merged_pr( + state, + &MergeRequestBase { + owner: pr.base.repo.owner.to_owned(), + repo: pr.base.repo.name.to_owned(), + number: pr.number, + }, + ); + + true +} + +pub fn cleanup_merged_pr(state: &AppState, pr: &MergeRequestBase) { + let AppState { db, .. } = state; + let mut iter = db.iterator(rocksdb::IteratorMode::Start); + for (key, _) in iter { + let db_mr: MergeRequest = + match bincode::deserialize(&value).context(Bincode) { + Ok(mr) => mr, + Err(err) => { + log::error!( + "Failed to deserialize {} during cleanup_merged_pr", + String::from_utf8_lossy(sha) + ); + continue; + } + }; + log::info!("Deserialized merge request: {:?}", db_mr); + + if db_mr.owner == pr.owner + && db_mr.repo_name == pr.repo + && db_mr.number == pr.number + { + if let Err(err) = db.delete(key) { + log::error!( + "Failed to delete {} during cleanup_merged_pr due to {:?}", + String::from_utf8_lossy(key), + err + ); + } + } + } +} + /// Send a merge request. /// It might recursively call itself when attempting to solve a merge error after something /// meaningful happens. #[async_recursion] pub async fn merge( - github_bot: &GithubBot, + state: &AppState, owner: &str, repo_name: &str, pr: &PullRequest, - bot_config: &BotConfig, requested_by: &str, created_approval_id: Option, ) -> Result> { - if pr.merged { + if check_cleanup_merged_pr(state, pr) { return Ok(Ok()); } @@ -1350,6 +1404,7 @@ pub async fn merge( { Ok(_) => { log::info!("{} merged successfully.", pr.html_url); + cleanup_merged_pr(state, pr); Ok(Ok(())) } Err(e) => match e { @@ -1719,3 +1774,147 @@ async fn handle_error( }, } } + +/// Try queueing this merge through the parent if some PR is depending on it +async fn attempt_merge_as_companion_fallback_merge( + state: &AppState, + mr: &MergeRequestBase, +) -> bool { + let AppState { db, github_bot, .. } = state; + + // Iteration is restarted after merge because merges might clean up not only this key, but also + // others which could come before it + 'restart_iteration: loop { + let mut iter = db.iterator(rocksdb::IteratorMode::Start); + + for (sha, value) in iter { + let db_mr: MergeRequest = + match bincode::deserialize(&value).context(Bincode) { + Ok(mr) => mr, + Err(err) => { + log::error!( + "Failed to deserialize {} during attempt_merge", + String::from_utf8_lossy(sha) + ); + continue; + } + }; + log::info!("Deserialized merge request: {:?}", db_mr); + + let companion_children = match db_mr.companion_children { + Some(companion_children) => companion_children, + None => continue, + }; + + for child in companion_children { + if child.owner != mr.owner + || child.repo != mr.repo + || child.number != mr.number + { + continue; + } + + if let Ok(true) = async { + let pr_from_db_mr = github_bot + .pull_request( + db_mr.owner, + db_mr.repo_name, + db_mr.number, + ) + .await?; + + if check_cleanup_merged_pr(state, pr_from_db_mr, None) + .await? + { + return Ok(false); + } + + let companions = match pr_from_db_mr + .body + .map(|body| parse_all_companions(body)) + { + Some(companions) => companions, + None => return Ok(false), + }; + // Check if this MR would be queued as a companion if we were to merge the MR parsed from + // the database (the current iteration's value) + if companions + .iter() + .find(|(_, owner, repo, number)| { + owner == mr.owner + && repo == mr.repo && number == mr.number + }) + .is_none() + { + return Ok(false); + } + + merge_allowed( + github_bot, + &owner, + &repo_name, + &pr_from_db_mr, + bot_config, + &requested_by, + None, + false, + ) + .await?; + + if ready_to_merge(github_bot, owner, &repo, &pr_from_db_mr) + .await? + { + merge( + state, + db_mr.owner, + db_mr.repo, + &pr_from_db_mr, + bot_config, + db_mr.requested_by, + None, + ) + .await??; + merge_companions(state, &repo, &pr_from_db_mr).await?; + } else { + wait_to_merge( + state, + pr_from_db_mr.head_sha()?, + &MergeRequest { + owner: db_mr.owner, + repo_name: db_mr.repo_name, + number: db_mr.number, + html_url: db_mr.html_url, + requested_by: db_mr.requested_by, + companion_children: pr.body.as_ref().map( + |body| { + parse_all_companions(body) + .into_iter() + .map(|(_, owner, repo, number)| { + MergeRequestBase { + owner, + repo, + number, + } + }) + .collect() + }, + ), + }, + None, + ) + .await?; + } + + Ok(true) + } + .await + { + // See the note about restarting iteration at the top of this loop + continue 'restart_iteration; + }; + } + } + + break; + } +} From d9592533a4a762afb169c33aa56560adfbeb0169 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 00:52:41 -0300 Subject: [PATCH 04/45] wip --- src/companion.rs | 228 +++++++++++++++++++-------------------- src/webhook.rs | 276 +++++++++++++++++++++++------------------------ 2 files changed, 246 insertions(+), 258 deletions(-) diff --git a/src/companion.rs b/src/companion.rs index 847654ca..4aea2c28 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -13,9 +13,9 @@ use crate::{ github::*, github_bot::GithubBot, webhook::{ - get_latest_checks_state, get_latest_statuses_state, merge, - ready_to_merge, wait_to_merge, AppState, MergeRequest, - MergeRequestBase, + check_cleanup_merged_pr, get_latest_checks_state, + get_latest_statuses_state, merge, ready_to_merge, wait_to_merge, + AppState, MergeRequest, MergeRequestBase, }, Result, Status, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, COMPANION_SHORT_REGEX, PR_HTML_URL_REGEX, @@ -28,7 +28,6 @@ async fn update_companion_repository( contributor: &str, contributor_repo: &str, contributor_branch: &str, - merge_done_in: &str, ) -> Result { let token = github_bot.client.auth_key().await?; let secrets_to_hide = [token.as_str()]; @@ -178,7 +177,7 @@ async fn update_companion_repository( // Update the packages from 'merge_done_in' in the companion let url_merge_was_done_in = - format!("https://github.com/{}/{}", owner, merge_done_in); + format!("https://github.com/{}/{}", owner, owner_repo); let cargo_lock_path = Path::new(&repo_dir).join("Cargo.lock"); let lockfile = cargo_lock::Lockfile::load(cargo_lock_path).map_err(|err| { @@ -335,113 +334,119 @@ pub fn parse_all_companions(body: &str) -> Vec { 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?; - - if companion.merged { - continue; - } + // TODO: get rid of this limitation by breaking cycles in companion descriptions across repositories + if pr.base.repo.name != "substrate" + && pr.base.repo.name != MAIN_REPO_FOR_STAGING + { + return Ok(()); + } - let head_sha = companion.head_sha()?; + let body = match pr.body.as_ref() { + Some(body) => body, + None => return Ok(()), + }; + + for (html_url, owner, repo, number) in parse_all_companions(body) { + let companion = github_bot.pull_request(&owner, &repo, number).await?; + + if companion.merged { + continue; + } + + let head_sha = companion.head_sha()?; - let has_user_owner = companion - .user + let has_user_owner = companion + .user + .as_ref() + .map(|user| { + user.type_field .as_ref() - .map(|user| { - user.type_field - .as_ref() - .map(|user_type| user_type == &UserType::User) - .unwrap_or(false) - }) - .unwrap_or(false); - if !has_user_owner { - 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 - ), - }); - } + .map(|user_type| user_type == &UserType::User) + .unwrap_or(false) + }) + .unwrap_or(false); + if !has_user_owner { + 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 - ), - }); - } + 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 + ), + }); + } - let statuses = get_latest_statuses_state( - github_bot, - &owner, - &repo, - head_sha, - &pr.html_url, - ) - .await?; - match statuses { - Status::Success => (), - Status::Pending => { - return Err(Error::InvalidCompanionStatus { - status: Status::Pending, - msg: format!( - "Companion {} has pending required statuses", - html_url - ), - }); - } - Status::Failure => { - return Err(Error::InvalidCompanionStatus { - status: Status::Failure, - msg: format!( - "Companion {} has failed required statuses", - html_url - ), - }); - } - }; - - let checks = get_latest_checks_state( - github_bot, - &owner, - &repo, - &head_sha, - &pr.html_url, - ) - .await?; - match checks { - Status::Success => (), - Status::Pending => { - return Err(Error::InvalidCompanionStatus { - status: checks, - msg: format!( - "Companion {} has pending required checks", - html_url - ), - }); - } - Status::Failure => { - return Err(Error::InvalidCompanionStatus { - status: checks, - msg: format!( - "Companion {} has failed required checks", - html_url - ), - }); - } - }; + let statuses = get_latest_statuses_state( + github_bot, + &owner, + &repo, + head_sha, + &pr.html_url, + ) + .await?; + match statuses { + Status::Success => (), + Status::Pending => { + return Err(Error::InvalidCompanionStatus { + status: Status::Pending, + msg: format!( + "Companion {} has pending required statuses", + html_url + ), + }); } - } + Status::Failure => { + return Err(Error::InvalidCompanionStatus { + status: Status::Failure, + msg: format!( + "Companion {} has failed required statuses", + html_url + ), + }); + } + }; + + let checks = get_latest_checks_state( + github_bot, + &owner, + &repo, + &head_sha, + &pr.html_url, + ) + .await?; + match checks { + Status::Success => (), + Status::Pending => { + return Err(Error::InvalidCompanionStatus { + status: checks, + msg: format!( + "Companion {} has pending required checks", + html_url + ), + }); + } + Status::Failure => { + return Err(Error::InvalidCompanionStatus { + status: checks, + msg: format!( + "Companion {} has failed required checks", + html_url + ), + }); + } + }; } Ok(()) @@ -492,7 +497,6 @@ async fn update_then_merge_companion( &contributor, &contributor_repo, &contributor_branch, - merge_done_in, ) .await?; @@ -539,10 +543,11 @@ async fn update_then_merge_companion( pub async fn merge_companions( state: &AppState, - merge_done_in: &str, pr: &PullRequest, ) -> Result<()> { - if merge_done_in != "substrate" && merge_done_in != MAIN_REPO_FOR_STAGING { + if pr.base.repo.owner.login != "substrate" + && pr.base.repo.owner.login != MAIN_REPO_FOR_STAGING + { return Ok(()); } @@ -587,12 +592,7 @@ pub async fn merge_companions( for (html_url, owner, repo, ref number) in group { if let Err(err) = update_then_merge_companion( - state, - &html_url, - &owner, - &repo, - number, - merge_done_in, + state, &html_url, &owner, &repo, number, ) .await { diff --git a/src/webhook.rs b/src/webhook.rs index 9c9a4e3a..b1f59e9c 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -538,12 +538,8 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { async fn handle_command( state: &AppState, cmd: &CommentCommand, - owner: &str, - repo: &str, - number: i64, - html_url: &str, - requested_by: &str, pr: &PullRequest, + requested_by: &str, ) -> Result<()> { let AppState { db, @@ -557,11 +553,8 @@ async fn handle_command( let pr_head_sha = pr.head_sha()?; merge_allowed( - github_bot, - owner, - &repo, + state, &pr, - bot_config, requested_by, None, match cmd { @@ -593,17 +586,7 @@ async fn handle_command( }), }; if ready_to_merge(github_bot, owner, &repo, &pr).await? { - match merge( - github_bot, - owner, - &repo, - &pr, - bot_config, - requested_by, - None, - ) - .await? - { + match merge(state, pr, requested_by, None).await? { // If the merge failure will be solved later, then register the PR in the database so that // it'll eventually resume processing when later statuses arrive Err(Error::MergeFailureWillBeSolvedLater { @@ -671,13 +654,17 @@ async fn handle_command( } db.delete(pr_head_sha.as_bytes()).context(Db)?; - merge_companions(state, &repo, &pr).await + merge_companions(state, pr).await } CommentCommand::CancelMerge => { log::info!("Deleting merge request for {}", html_url); - let pr_head_sha = pr.head_sha()?; - db.delete(pr_head_sha.as_bytes()).context(Db)?; + cleanup_pr( + state, + pr.base.repo.owner.login, + pr.base.repo.name, + pr.number, + ); if let Err(err) = github_bot .create_issue_comment( @@ -853,17 +840,7 @@ async fn handle_comment( Err(err) => return (None, Err(err)), }; - let result = handle_command( - state, - &cmd, - owner, - repo, - number, - html_url, - requested_by, - &pr, - ) - .await; + let result = handle_command(state, &cmd, &pr, requested_by).await; let sha = match cmd { CommentCommand::Merge(_) => { pr.head_sha().ok().map(|head_sha| head_sha.to_owned()) @@ -970,15 +947,18 @@ async fn handle_burnin_request( /// because the merge might succeed regardless of them, thus it does not make /// sense to fail this scenario completely if the request fails for some reason. async fn merge_allowed( - github_bot: &GithubBot, - owner: &str, - repo_name: &str, + state: &AppState, pr: &PullRequest, - bot_config: &BotConfig, requested_by: &str, min_approvals_required: Option, allow_pending_required_status_for_companions: bool, ) -> Result> { + let AppState { + github_bot, + bot_config, + .. + } = state; + let is_mergeable = pr.mergeable.unwrap_or(false); if let Some(min_approvals_required) = &min_approvals_required { @@ -994,8 +974,7 @@ async fn merge_allowed( let result = async { if let Err(err) = - check_all_companions_are_mergeable(github_bot, &pr, &repo_name) - .await + check_all_companions_are_mergeable(github_bot, &pr).await { match err { Error::InvalidCompanionStatus { ref status, .. } => { @@ -1065,7 +1044,7 @@ async fn merge_allowed( let mut errors: Vec = Vec::new(); let team_leads = github_bot - .substrate_team_leads(owner) + .substrate_team_leads(&pr.base.repo.owner.login) .await .unwrap_or_else(|e| { let msg = format!( @@ -1089,12 +1068,15 @@ async fn merge_allowed( }) .count(); - let core_devs = github_bot.core_devs(owner).await.unwrap_or_else(|e| { - let msg = format!("Error getting {}: `{}`", CORE_DEVS_GROUP, e); - log::error!("{}", msg); - errors.push(msg); - vec![] - }); + let core_devs = github_bot + .core_devs(&pr.base.repo.owner.login) + .await + .unwrap_or_else(|e| { + let msg = format!("Error getting {}: `{}`", CORE_DEVS_GROUP, e); + log::error!("{}", msg); + errors.push(msg); + vec![] + }); let core_approvals = approved_reviews .iter() .filter(|review| { @@ -1140,7 +1122,10 @@ async fn merge_allowed( Ok(relevant_approvals_count) } else { let (process, process_warnings) = process::get_process( - github_bot, owner, repo_name, pr.number, + github_bot, + &pr.base.repo.owner.login, + &pr.base.repo.name, + pr.number, ) .await?; @@ -1205,9 +1190,13 @@ async fn merge_allowed( { Some("a team lead".to_string()) } else { - let (process, _) = - process::get_process(github_bot, owner, repo_name, pr.number) - .await?; + let (process, _) = process::get_process( + github_bot, + &pr.base.repo.owner.login, + &pr.base.repo.name, + pr.number, + ) + .await?; if process.is_owner(requested_by) { Some("a project owner".to_string()) } else { @@ -1219,7 +1208,7 @@ async fn merge_allowed( }; result.await.map_err(|err| { - err.map_issue((owner.to_string(), repo_name.to_string(), pr.number)) + err.map_issue((pr.base.repo.owner.login, pr.base.repo.name, pr.number)) }) } @@ -1229,8 +1218,6 @@ async fn merge_allowed( /// request and wait for checks -- if so they will later be handled by `checks_and_status`. pub async fn ready_to_merge( github_bot: &GithubBot, - owner: &str, - repo_name: &str, pr: &PullRequest, ) -> Result { match pr.head_sha() { @@ -1337,46 +1324,47 @@ pub async fn check_cleanup_merged_pr( return false; } - cleanup_merged_pr( - state, - &MergeRequestBase { - owner: pr.base.repo.owner.to_owned(), - repo: pr.base.repo.name.to_owned(), - number: pr.number, - }, - ); + cleanup_pr(state, pr.base.repo.owner, pr.base.repo.name, pr.number); true } -pub fn cleanup_merged_pr(state: &AppState, pr: &MergeRequestBase) { +pub fn cleanup_pr( + state: &AppState, + main_key: &str, + owner: &str, + repo: &str, + number: i64, +) { let AppState { db, .. } = state; + let mut iter = db.iterator(rocksdb::IteratorMode::Start); - for (key, _) in iter { + for (key, value) in iter { let db_mr: MergeRequest = match bincode::deserialize(&value).context(Bincode) { Ok(mr) => mr, Err(err) => { log::error!( - "Failed to deserialize {} during cleanup_merged_pr", - String::from_utf8_lossy(sha) + "Failed to deserialize {} during cleanup_pr", + String::from_utf8_lossy(key) ); continue; } }; - log::info!("Deserialized merge request: {:?}", db_mr); - if db_mr.owner == pr.owner - && db_mr.repo_name == pr.repo - && db_mr.number == pr.number + if db_mr.owner != owner + || db_mr.repo_name != repo + || db_mr.number != number { - if let Err(err) = db.delete(key) { - log::error!( - "Failed to delete {} during cleanup_merged_pr due to {:?}", - String::from_utf8_lossy(key), - err - ); - } + continue; + } + + if let Err(err) = db.delete(key) { + log::error!( + "Failed to delete {} during cleanup_pr due to {:?}", + String::from_utf8_lossy(key), + err + ); } } } @@ -1387,8 +1375,6 @@ pub fn cleanup_merged_pr(state: &AppState, pr: &MergeRequestBase) { #[async_recursion] pub async fn merge( state: &AppState, - owner: &str, - repo_name: &str, pr: &PullRequest, requested_by: &str, created_approval_id: Option, @@ -1397,14 +1383,19 @@ pub async fn merge( return Ok(Ok()); } + let AppState { + github_bot, + bot_config, + .. + } = state; match pr.head_sha() { Ok(pr_head_sha) => match github_bot - .merge_pull_request(owner, repo_name, pr.number, pr_head_sha) + .merge_pull_request(pr.base.repo.owner.login, pr.base.repo.name, pr.number, pr_head_sha) .await { Ok(_) => { log::info!("{} merged successfully.", pr.html_url); - cleanup_merged_pr(state, pr); + cleanup_pr(state, pr); Ok(Ok(())) } Err(e) => match e { @@ -1459,11 +1450,8 @@ pub async fn merge( .parse::() .unwrap(); match merge_allowed( - github_bot, - owner, - repo_name, + state, pr, - bot_config, requested_by, Some(min_approvals_required), false @@ -1474,8 +1462,8 @@ pub async fn merge( Some(requester_role) => { if let Err(err) = github_bot .create_issue_comment( - owner, - &repo_name, + pr.base.repo.owner.login, + pr.base.repo.name, pr.number, &format!( "Bot will approve on the behalf of @{}, since they are {}, in an attempt to reach the minimum approval count", @@ -1487,16 +1475,13 @@ pub async fn merge( log::error!("Failed to post comment on {} due to {}", pr.html_url, err); } match github_bot.approve_merge_request( - owner, - repo_name, + pr.base.repo.owner, + pr.base.repo.name, pr.number ).await { Ok(review) => merge( - github_bot, - owner, - repo_name, + state, pr, - bot_config, requested_by, Some(review.id) ).await, @@ -1539,9 +1524,9 @@ Pull Request Merge Endpoint responded with unexpected body: `{}`", .map_err(|e| Error::Merge { source: Box::new(e), commit_sha: pr_head_sha.to_string(), - pr_url: pr.url.to_string(), - owner: owner.to_string(), - repo_name: repo_name.to_string(), + pr_url: pr.url, + owner: pr.base.repo.owner.login, + repo_name: pr.base.repo.name, pr_number: pr.number, created_approval_id }), @@ -1549,7 +1534,11 @@ Pull Request Merge Endpoint responded with unexpected body: `{}`", Err(e) => Err(e), } .map_err(|e| { - e.map_issue((owner.to_string(), repo_name.to_string(), pr.number)) + e.map_issue(( + pr.base.repo.owner.login.to_owned(), + pr.base.repo.name.to_owned(), + pr.number + )) }) } @@ -1778,9 +1767,14 @@ async fn handle_error( /// Try queueing this merge through the parent if some PR is depending on it async fn attempt_merge_as_companion_fallback_merge( state: &AppState, - mr: &MergeRequestBase, + pr: &PullRequest, ) -> bool { - let AppState { db, github_bot, .. } = state; + let AppState { + db, + github_bot, + bot_config, + .. + } = state; // Iteration is restarted after merge because merges might clean up not only this key, but also // others which could come before it @@ -1793,13 +1787,12 @@ async fn attempt_merge_as_companion_fallback_merge( Ok(mr) => mr, Err(err) => { log::error!( - "Failed to deserialize {} during attempt_merge", + "Failed to deserialize {} during attempt_merge_as_companion_fallback_merge", String::from_utf8_lossy(sha) ); continue; } }; - log::info!("Deserialized merge request: {:?}", db_mr); let companion_children = match db_mr.companion_children { Some(companion_children) => companion_children, @@ -1807,13 +1800,19 @@ async fn attempt_merge_as_companion_fallback_merge( }; for child in companion_children { - if child.owner != mr.owner - || child.repo != mr.repo - || child.number != mr.number + if child.owner != pr.base.repo.owner + || child.repo != pr.base.repo.name + || child.number != pr.number { continue; } + log::info!( + "{} is listed as a companion of {} in the database", + pr.html_url, + db_mr.html_url + ); + if let Ok(true) = async { let pr_from_db_mr = github_bot .pull_request( @@ -1841,64 +1840,53 @@ async fn attempt_merge_as_companion_fallback_merge( if companions .iter() .find(|(_, owner, repo, number)| { - owner == mr.owner - && repo == mr.repo && number == mr.number + owner == pr.owner + && repo == pr.repo && number == pr.number }) .is_none() { return Ok(false); } + log::info!( + "{} is listed as a companion of {} in the database", + pr.html_url, + db_mr.html_url + ); + merge_allowed( - github_bot, - &owner, - &repo_name, + state, &pr_from_db_mr, - bot_config, - &requested_by, + &db_mr.requested_by, None, false, ) .await?; - - if ready_to_merge(github_bot, owner, &repo, &pr_from_db_mr) - .await? - { - merge( - state, - db_mr.owner, - db_mr.repo, - &pr_from_db_mr, - bot_config, - db_mr.requested_by, - None, - ) - .await??; - merge_companions(state, &repo, &pr_from_db_mr).await?; + if ready_to_merge(github_bot, &pr_from_db_mr).await? { + merge(state, &pr_from_db_mr, &db_mr.requested_by, None) + .await??; + merge_companions(state, &pr_from_db_mr).await?; } else { wait_to_merge( state, pr_from_db_mr.head_sha()?, &MergeRequest { - owner: db_mr.owner, - repo_name: db_mr.repo_name, - number: db_mr.number, - html_url: db_mr.html_url, + owner: pr_from_db_mr.base.repo.owner, + repo_name: pr_from_db_mr.base.repo.name, + number: pr_from_db_mr.number, + html_url: pr_from_db_mr.html_url, requested_by: db_mr.requested_by, - companion_children: pr.body.as_ref().map( - |body| { - parse_all_companions(body) - .into_iter() - .map(|(_, owner, repo, number)| { - MergeRequestBase { - owner, - repo, - number, - } - }) - .collect() - }, - ), + // Reparse the companion's bodies so that we'll be able to + companion_children: companions + .into_iter() + .map(|(_, owner, repo, number)| { + MergeRequestBase { + owner, + repo, + number, + } + }) + .collect(), }, None, ) From ee1db52726d68a0ed4a4066ed59a25fe18f4bd67 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 01:35:21 -0300 Subject: [PATCH 05/45] wip --- src/companion.rs | 29 ++--- src/lib.rs | 5 + src/webhook.rs | 310 ++++++++++++++++++++++++++--------------------- 3 files changed, 193 insertions(+), 151 deletions(-) diff --git a/src/companion.rs b/src/companion.rs index 4aea2c28..1d7b0fdc 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -15,7 +15,7 @@ use crate::{ webhook::{ check_cleanup_merged_pr, get_latest_checks_state, get_latest_statuses_state, merge, ready_to_merge, wait_to_merge, - AppState, MergeRequest, MergeRequestBase, + AppState, MergeRequest, }, Result, Status, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, COMPANION_SHORT_REGEX, PR_HTML_URL_REGEX, @@ -454,20 +454,20 @@ pub async fn check_all_companions_are_mergeable( async fn update_then_merge_companion( state: &AppState, - html_url: &str, owner: &str, repo: &str, number: &i64, + html_url: &str, merge_done_in: &str, ) -> Result<()> { let AppState { - github_bot, bot_config, + github_bot, .. } = state; let pr = github_bot.pull_request(&owner, &repo, *number).await?; - if check_cleanup_merged_pr(state, pr, None).await? { + if check_cleanup_merged_pr(state, &pr) { return Ok(()); } @@ -504,17 +504,8 @@ async fn update_then_merge_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, - BOT_NAME_FOR_COMMITS, - None, - ) - .await??; + if ready_to_merge(&state.github_bot, &pr).await? { + merge(state, &pr, BOT_NAME_FOR_COMMITS, None).await??; } else { log::info!("Companion updated; waiting for checks on {}", html_url); wait_to_merge( @@ -545,6 +536,7 @@ pub async fn merge_companions( state: &AppState, pr: &PullRequest, ) -> Result<()> { + // TODO: get rid of this limitation by breaking cycles in companion descriptions across repositories if pr.base.repo.owner.login != "substrate" && pr.base.repo.owner.login != MAIN_REPO_FOR_STAGING { @@ -592,7 +584,12 @@ pub async fn merge_companions( for (html_url, owner, repo, ref number) in group { if let Err(err) = update_then_merge_companion( - state, &html_url, &owner, &repo, number, + state, + &owner, + &repo, + number, + &html_url, + &pr.base.repo.owner.login, ) .await { diff --git a/src/lib.rs b/src/lib.rs index 33d80d6d..c3cd7d1b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -55,3 +55,8 @@ pub enum MergeCancelOutcome { WasCancelled, WasNotCancelled, } + +pub enum PendingCompanionStatusesRestriction { + Allow, + Disallow, +} diff --git a/src/webhook.rs b/src/webhook.rs index b1f59e9c..ee67daa2 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -16,7 +16,8 @@ use crate::{ companion::*, config::BotConfig, constants::*, error::*, github::*, github_bot::GithubBot, gitlab_bot::*, matrix_bot::MatrixBot, performance, process, rebase::*, utils::parse_bot_comment_from_text, vanity_service, - CommentCommand, MergeCancelOutcome, MergeCommentCommand, Result, Status, + CommentCommand, MergeCancelOutcome, MergeCommentCommand, + PendingCompanionStatusesRestriction, Result, Status, }; /// This data gets passed along with each webhook to the webhook handler. @@ -330,11 +331,11 @@ async fn handle_status( pub async fn get_latest_statuses_state( github_bot: &GithubBot, owner: &str, - owner_repo: &str, + repo: &str, commit_sha: &str, html_url: &str, ) -> Result { - let status = github_bot.status(&owner, &owner_repo, &commit_sha).await?; + let status = github_bot.status(&owner, &repo, &commit_sha).await?; log::info!("{:?}", status); // Since Github only considers the latest instance of each status, we should abide by the same @@ -484,17 +485,18 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { .await?; match checks { Status::Success => { - merge_allowed( - github_bot, - &owner, - &repo_name, + if let Err(err) = merge_allowed( + state, &pr, - bot_config, &requested_by, None, - true, + PendingCompanionStatusesRestriction::Disallow, ) - .await?; + .await? { + match err { + Err(Error::InvalidCompanionStatus) + } + } match merge( github_bot, @@ -509,7 +511,9 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { { Ok(_) => { attempt_merge_as_companion_fallback_merge( - state, pr, + state, + pr, + requested_by, ) .await } @@ -541,12 +545,7 @@ async fn handle_command( pr: &PullRequest, requested_by: &str, ) -> Result<()> { - let AppState { - db, - github_bot, - bot_config, - .. - } = state; + let AppState { db, github_bot, .. } = state; match cmd { CommentCommand::Merge(cmd) => { @@ -558,8 +557,12 @@ async fn handle_command( requested_by, None, match cmd { - MergeCommentCommand::Normal => true, - MergeCommentCommand::Force => false, + MergeCommentCommand::Normal => { + PendingCompanionStatusesRestriction::Allow + } + MergeCommentCommand::Force => { + PendingCompanionStatusesRestriction::Disallow + } }, ) .await?; @@ -567,10 +570,10 @@ async fn handle_command( match cmd { MergeCommentCommand::Normal => { let mr = MergeRequest { - owner: owner.to_owned(), - repo_name: repo.to_owned(), - number, - html_url: html_url.to_owned(), + owner: pr.base.repo.owner.login.to_owned(), + repo_name: pr.base.repo.name.to_owned(), + number: pr.number, + html_url: pr.html_url.to_owned(), requested_by: requested_by.to_owned(), companion_children: pr.body.as_ref().map(|body| { parse_all_companions(body) @@ -585,7 +588,7 @@ async fn handle_command( .collect() }), }; - if ready_to_merge(github_bot, owner, &repo, &pr).await? { + if ready_to_merge(github_bot, &pr).await? { match merge(state, pr, requested_by, None).await? { // If the merge failure will be solved later, then register the PR in the database so that // it'll eventually resume processing when later statuses arrive @@ -618,17 +621,7 @@ async fn handle_command( } } MergeCommentCommand::Force => { - match merge( - github_bot, - owner, - &repo, - &pr, - bot_config, - requested_by, - None, - ) - .await? - { + match merge(state, pr, requested_by, None).await? { // Even if the merge failure can be solved later, it does not matter because `merge force` is // supposed to be immediate. We should give up here and yield the error message. Err(Error::MergeFailureWillBeSolvedLater { msg }) => { @@ -636,14 +629,14 @@ async fn handle_command( source: Box::new(Error::Message { msg }), commit_sha: pr_head_sha.to_owned(), pr_url: pr.html_url.to_owned(), - owner: owner.to_owned(), - repo_name: repo.to_owned(), + owner: pr.base.repo.owner.login.to_owned(), + repo_name: pr.base.repo.name.to_owned(), pr_number: pr.number, created_approval_id: None, } .map_issue(( - owner.to_owned(), - repo.to_owned(), + pr.base.repo.owner.login.to_owned(), + pr.base.repo.name.to_owned(), pr.number, ))) } @@ -657,19 +650,19 @@ async fn handle_command( merge_companions(state, pr).await } CommentCommand::CancelMerge => { - log::info!("Deleting merge request for {}", html_url); + log::info!("Deleting merge request for {}", pr.html_url); cleanup_pr( state, - pr.base.repo.owner.login, - pr.base.repo.name, + &pr.base.repo.owner.login, + &pr.base.repo.name, pr.number, ); if let Err(err) = github_bot .create_issue_comment( - owner, - &repo, + &pr.base.repo.owner.login, + &pr.base.repo.name, pr.number, "Merge cancelled.", ) @@ -677,7 +670,7 @@ async fn handle_command( { log::error!( "Failed to post comment on {} due to {}", - html_url, + pr.html_url, err ); } @@ -704,20 +697,25 @@ async fn handle_command( } = pr.clone() { if let Err(err) = github_bot - .create_issue_comment(owner, &repo, pr.number, "Rebasing") + .create_issue_comment( + &pr.base.repo.owner.login, + &pr.base.repo.name, + pr.number, + "Rebasing", + ) .await { log::error!( "Failed to post comment on {} due to {}", - html_url, + pr.html_url, err ); } rebase( github_bot, - owner, - &repo, + &pr.base.repo.owner.login, + &pr.base.repo.name, &head_owner, &head_repo, &head_branch, @@ -730,55 +728,63 @@ async fn handle_command( } } CommentCommand::BurninRequest => { - handle_burnin_request( - github_bot, - &state.gitlab_bot, - &state.matrix_bot, - owner, - requested_by, - &repo, - &pr, - ) - .await + handle_burnin_request(state, pr, requested_by).await } - CommentCommand::CompareReleaseRequest => match repo { - "polkadot" => { - let pr_head_sha = pr.head_sha()?; - let rel = github_bot.latest_release(owner, &repo).await?; - let release_tag = - github_bot.tag(owner, &repo, &rel.tag_name).await?; - let release_substrate_commit = github_bot - .substrate_commit_from_polkadot_commit( - &release_tag.object.sha, - ) - .await?; - let branch_substrate_commit = github_bot - .substrate_commit_from_polkadot_commit(pr_head_sha) - .await?; - let link = github_bot.diff_url( - owner, - "substrate", - &release_substrate_commit, - &branch_substrate_commit, - ); - log::info!("Posting link to substrate diff: {}", &link); - if let Err(err) = github_bot - .create_issue_comment(owner, &repo, number, &link) - .await - { - log::error!( - "Failed to post comment on {} due to {}", - html_url, - err + CommentCommand::CompareReleaseRequest => { + match pr.base.repo.name.as_str() { + "polkadot" => { + let pr_head_sha = pr.head_sha()?; + let rel = github_bot + .latest_release( + &pr.base.repo.owner.login, + &pr.base.repo.name, + ) + .await?; + let release_tag = github_bot + .tag( + &pr.base.repo.owner.login, + &pr.base.repo.name, + &rel.tag_name, + ) + .await?; + let release_substrate_commit = github_bot + .substrate_commit_from_polkadot_commit( + &release_tag.object.sha, + ) + .await?; + let branch_substrate_commit = github_bot + .substrate_commit_from_polkadot_commit(pr_head_sha) + .await?; + let link = github_bot.diff_url( + &pr.base.repo.owner.login, + "substrate", + &release_substrate_commit, + &branch_substrate_commit, ); + log::info!("Posting link to substrate diff: {}", &link); + if let Err(err) = github_bot + .create_issue_comment( + &pr.base.repo.owner.login, + &pr.base.repo.name, + pr.number, + &link, + ) + .await + { + log::error!( + "Failed to post comment on {} due to {}", + pr.html_url, + err + ); + } + Ok(()) } - Ok(()) + _ => Err(Error::Message { + msg: "This command can't be requested from this repository" + .to_string(), + }), } - _ => Err(Error::Message { - msg: "This command can't be requested from this repository" - .to_string(), - }), - }, + } } } @@ -852,14 +858,17 @@ async fn handle_comment( } async fn handle_burnin_request( - github_bot: &GithubBot, - gitlab_bot: &GitlabBot, - matrix_bot: &MatrixBot, - owner: &str, - requested_by: &str, - repo_name: &str, + state: &AppState, pr: &PullRequest, + requested_by: &str, ) -> Result<()> { + let AppState { + gitlab_bot, + github_bot, + matrix_bot, + .. + } = state; + let make_job_link = |url| format!("CI job for burn-in deployment", url); @@ -928,13 +937,18 @@ async fn handle_burnin_request( }; github_bot - .create_issue_comment(owner, &repo_name, pr.number, &msg) + .create_issue_comment( + &pr.base.repo.owner.login, + &pr.base.repo.name, + pr.number, + &msg, + ) .await?; matrix_bot.send_html_to_default( format!( "Received burn-in request for {}#{} from {}
\n{}", - pr.html_url, repo_name, pr.number, requested_by, matrix_msg.unwrap_or(msg), + pr.html_url, pr.base.repo.name, pr.number, requested_by, matrix_msg.unwrap_or(msg), ) .as_str(), )?; @@ -951,7 +965,7 @@ async fn merge_allowed( pr: &PullRequest, requested_by: &str, min_approvals_required: Option, - allow_pending_required_status_for_companions: bool, + pending_companion_statuses_restriction: PendingCompanionStatusesRestriction, ) -> Result> { let AppState { github_bot, @@ -978,9 +992,11 @@ async fn merge_allowed( { match err { Error::InvalidCompanionStatus { ref status, .. } => { - match (status, allow_pending_required_status_for_companions) - { - (Status::Pending, true) => (), + match (status, pending_companion_statuses_restriction) { + ( + Status::Pending, + PendingCompanionStatusesRestriction::Allow, + ) => (), _ => return Err(err), } } @@ -1208,7 +1224,11 @@ async fn merge_allowed( }; result.await.map_err(|err| { - err.map_issue((pr.base.repo.owner.login, pr.base.repo.name, pr.number)) + err.map_issue(( + pr.base.repo.owner.login.to_owned(), + pr.base.repo.name.to_owned(), + pr.number, + )) }) } @@ -1224,8 +1244,8 @@ pub async fn ready_to_merge( Ok(pr_head_sha) => { match get_latest_statuses_state( github_bot, - owner, - repo_name, + &pr.base.repo.owner.login, + &pr.base.repo.name, pr_head_sha, &pr.html_url, ) @@ -1235,8 +1255,8 @@ pub async fn ready_to_merge( Status::Success => { match get_latest_checks_state( github_bot, - owner, - repo_name, + &pr.base.repo.owner.login, + &pr.base.repo.name, pr_head_sha, &pr.html_url, ) @@ -1263,7 +1283,11 @@ pub async fn ready_to_merge( Err(e) => Err(e), } .map_err(|e| { - e.map_issue((owner.to_string(), repo_name.to_string(), pr.number)) + e.map_issue(( + pr.base.repo.owner.login.to_owned(), + pr.base.repo.name.to_owned(), + pr.number, + )) }) } @@ -1316,26 +1340,22 @@ pub async fn wait_to_merge( Ok(()) } -pub async fn check_cleanup_merged_pr( - state: &AppState, - pr: &PullRequest, -) -> bool { +pub fn check_cleanup_merged_pr(state: &AppState, pr: &PullRequest) -> bool { if !pr.merged { return false; } - cleanup_pr(state, pr.base.repo.owner, pr.base.repo.name, pr.number); + cleanup_pr( + state, + &pr.base.repo.owner.login, + &pr.base.repo.name, + pr.number, + ); true } -pub fn cleanup_pr( - state: &AppState, - main_key: &str, - owner: &str, - repo: &str, - number: i64, -) { +pub fn cleanup_pr(state: &AppState, owner: &str, repo: &str, number: i64) { let AppState { db, .. } = state; let mut iter = db.iterator(rocksdb::IteratorMode::Start); @@ -1346,7 +1366,7 @@ pub fn cleanup_pr( Err(err) => { log::error!( "Failed to deserialize {} during cleanup_pr", - String::from_utf8_lossy(key) + String::from_utf8_lossy(&key) ); continue; } @@ -1362,7 +1382,7 @@ pub fn cleanup_pr( if let Err(err) = db.delete(key) { log::error!( "Failed to delete {} during cleanup_pr due to {:?}", - String::from_utf8_lossy(key), + String::from_utf8_lossy(&key), err ); } @@ -1380,7 +1400,7 @@ pub async fn merge( created_approval_id: Option, ) -> Result> { if check_cleanup_merged_pr(state, pr) { - return Ok(Ok()); + return Ok(Ok(())); } let AppState { @@ -1390,12 +1410,12 @@ pub async fn merge( } = state; match pr.head_sha() { Ok(pr_head_sha) => match github_bot - .merge_pull_request(pr.base.repo.owner.login, pr.base.repo.name, pr.number, pr_head_sha) + .merge_pull_request(&pr.base.repo.owner.login, &pr.base.repo.name, pr.number, pr_head_sha) .await { Ok(_) => { log::info!("{} merged successfully.", pr.html_url); - cleanup_pr(state, pr); + cleanup_pr(state, &pr.base.repo.owner.login, &pr.base.repo.name, pr.number); Ok(Ok(())) } Err(e) => match e { @@ -1462,8 +1482,8 @@ pub async fn merge( Some(requester_role) => { if let Err(err) = github_bot .create_issue_comment( - pr.base.repo.owner.login, - pr.base.repo.name, + &pr.base.repo.owner.login, + &pr.base.repo.name, pr.number, &format!( "Bot will approve on the behalf of @{}, since they are {}, in an attempt to reach the minimum approval count", @@ -1475,8 +1495,8 @@ pub async fn merge( log::error!("Failed to post comment on {} due to {}", pr.html_url, err); } match github_bot.approve_merge_request( - pr.base.repo.owner, - pr.base.repo.name, + &pr.base.repo.owner.login, + &pr.base.repo.name, pr.number ).await { Ok(review) => merge( @@ -1768,7 +1788,8 @@ async fn handle_error( async fn attempt_merge_as_companion_fallback_merge( state: &AppState, pr: &PullRequest, -) -> bool { + requested_by: &str, +) -> Result<()> { let AppState { db, github_bot, @@ -1776,6 +1797,8 @@ async fn attempt_merge_as_companion_fallback_merge( .. } = state; + let pr_head_sha = pr.head_sha()?; + // Iteration is restarted after merge because merges might clean up not only this key, but also // others which could come before it 'restart_iteration: loop { @@ -1862,7 +1885,8 @@ async fn attempt_merge_as_companion_fallback_merge( false, ) .await?; - if ready_to_merge(github_bot, &pr_from_db_mr).await? { + + if ready_to_merge(state.github_bot, &pr_from_db_mr).await? { merge(state, &pr_from_db_mr, &db_mr.requested_by, None) .await??; merge_companions(state, &pr_from_db_mr).await?; @@ -1905,4 +1929,20 @@ async fn attempt_merge_as_companion_fallback_merge( break; } + + // If the PR was created as a companion, or is already pending, there's no point in trying to + // merge it here + if db.get(pr_head_sha).context(Db)?.is_some() { + return Ok(()); + } + + merge_allowed(state, pr, requested_by, None, true).await?; + if ready_to_merge(github_bot, &pr).await? { + match merge(state, &pr, requested_by, None).await? { + Ok(_) => (), + Err(Error::MergeFailureWillBeSolvedLater { .. }) => return Ok(()), + Err(err) => return Err(err), + }; + merge_companions(state, &pr).await?; + } } From 391c5b5bd2a06adbf0d78c26fed1955db2b711d4 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 02:46:15 -0300 Subject: [PATCH 06/45] wip --- src/companion.rs | 18 ++-- src/error.rs | 9 +- src/webhook.rs | 234 ++++++++++++++++++++++------------------------- 3 files changed, 122 insertions(+), 139 deletions(-) diff --git a/src/companion.rs b/src/companion.rs index 1d7b0fdc..a62cf657 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -28,6 +28,7 @@ async fn update_companion_repository( contributor: &str, contributor_repo: &str, contributor_branch: &str, + merge_done_in: &str, ) -> Result { let token = github_bot.client.auth_key().await?; let secrets_to_hide = [token.as_str()]; @@ -177,7 +178,7 @@ async fn update_companion_repository( // Update the packages from 'merge_done_in' in the companion let url_merge_was_done_in = - format!("https://github.com/{}/{}", owner, owner_repo); + 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| { @@ -400,7 +401,7 @@ pub async fn check_all_companions_are_mergeable( Status::Success => (), Status::Pending => { return Err(Error::InvalidCompanionStatus { - status: Status::Pending, + value: InvalidCompanionStatusValue::Pending, msg: format!( "Companion {} has pending required statuses", html_url @@ -409,7 +410,7 @@ pub async fn check_all_companions_are_mergeable( } Status::Failure => { return Err(Error::InvalidCompanionStatus { - status: Status::Failure, + value: InvalidCompanionStatusValue::Failure, msg: format!( "Companion {} has failed required statuses", html_url @@ -430,7 +431,7 @@ pub async fn check_all_companions_are_mergeable( Status::Success => (), Status::Pending => { return Err(Error::InvalidCompanionStatus { - status: checks, + value: InvalidCompanionStatusValue::Pending, msg: format!( "Companion {} has pending required checks", html_url @@ -439,7 +440,7 @@ pub async fn check_all_companions_are_mergeable( } Status::Failure => { return Err(Error::InvalidCompanionStatus { - status: checks, + value: InvalidCompanionStatusValue::Failure, msg: format!( "Companion {} has failed required checks", html_url @@ -460,11 +461,7 @@ async fn update_then_merge_companion( html_url: &str, merge_done_in: &str, ) -> Result<()> { - let AppState { - bot_config, - github_bot, - .. - } = state; + let AppState { github_bot, .. } = state; let pr = github_bot.pull_request(&owner, &repo, *number).await?; if check_cleanup_merged_pr(state, &pr) { @@ -497,6 +494,7 @@ async fn update_then_merge_companion( &contributor, &contributor_repo, &contributor_branch, + merge_done_in, ) .await?; diff --git a/src/error.rs b/src/error.rs index 1d888c2d..42b4eed7 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,4 +1,3 @@ -use crate::Status; use snafu::Snafu; // TODO this really should be struct { owner, repo, number } @@ -15,6 +14,12 @@ pub struct CompanionDetailsWithErrorMessage { pub msg: String, } +#[derive(Debug)] +pub enum InvalidCompanionStatusValue { + Pending, + Failure, +} + #[derive(Debug, Snafu)] #[snafu(visibility = "pub")] pub enum Error { @@ -194,7 +199,7 @@ pub enum Error { #[snafu(display("{}", msg))] InvalidCompanionStatus { - status: Status, + value: InvalidCompanionStatusValue, msg: String, }, } diff --git a/src/webhook.rs b/src/webhook.rs index ee67daa2..ea3415c7 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -456,12 +456,7 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { requested_by, .. } = &mr; - let AppState { - github_bot, - db, - bot_config, - .. - } = state; + let AppState { github_bot, .. } = state; let pr = github_bot.pull_request(owner, repo_name, *number).await?; let pr_head_sha = pr.head_sha()?; @@ -485,38 +480,39 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { .await?; match checks { Status::Success => { - if let Err(err) = merge_allowed( + if let Err(err) = check_merge_is_allowed( state, &pr, &requested_by, None, PendingCompanionStatusesRestriction::Disallow, ) - .await? { - match err { - Err(Error::InvalidCompanionStatus) - } + .await + { + return match err { + Error::InvalidCompanionStatus { + ref value, + .. + } => match value { + InvalidCompanionStatusValue::Pending => { + Ok(()) + } + InvalidCompanionStatusValue::Failure => { + Err(err) + } + }, + _ => Err(err), + }; } - match merge( - github_bot, - &owner, - &repo_name, + match attempt_merge_as_companion_fallback_direct( + state, &pr, - bot_config, - &requested_by, - None, + requested_by, ) - .await? + .await { - Ok(_) => { - attempt_merge_as_companion_fallback_merge( - state, - pr, - requested_by, - ) - .await - } + Ok(_) => Ok(()), Err(Error::MergeFailureWillBeSolvedLater { .. }) => Ok(()), @@ -551,7 +547,7 @@ async fn handle_command( CommentCommand::Merge(cmd) => { let pr_head_sha = pr.head_sha()?; - merge_allowed( + check_merge_is_allowed( state, &pr, requested_by, @@ -846,7 +842,11 @@ async fn handle_comment( Err(err) => return (None, Err(err)), }; - let result = handle_command(state, &cmd, &pr, requested_by).await; + let result = handle_command(state, &cmd, &pr, requested_by) + .await + .map_err(|err| { + err.map_issue((owner.to_owned(), repo.to_owned(), number)) + }); let sha = match cmd { CommentCommand::Merge(_) => { pr.head_sha().ok().map(|head_sha| head_sha.to_owned()) @@ -960,7 +960,7 @@ async fn handle_burnin_request( /// Errors related to core-devs and substrateteamleads API requests are ignored /// because the merge might succeed regardless of them, thus it does not make /// sense to fail this scenario completely if the request fails for some reason. -async fn merge_allowed( +async fn check_merge_is_allowed( state: &AppState, pr: &PullRequest, requested_by: &str, @@ -991,11 +991,11 @@ async fn merge_allowed( check_all_companions_are_mergeable(github_bot, &pr).await { match err { - Error::InvalidCompanionStatus { ref status, .. } => { - match (status, pending_companion_statuses_restriction) { + Error::InvalidCompanionStatus { ref value, .. } => { + match (pending_companion_statuses_restriction, value) { ( - Status::Pending, PendingCompanionStatusesRestriction::Allow, + InvalidCompanionStatusValue::Pending, ) => (), _ => return Err(err), } @@ -1300,7 +1300,7 @@ async fn register_merge_request( mr: &MergeRequest, ) -> Result<()> { let AppState { db, .. } = state; - log::info!("Serializing merge request: {:?}", mr); + log::info!("Registering merge request: {:?}", mr); let bytes = bincode::serialize(mr).context(Bincode)?; log::info!("Writing merge request to db (sha: {})", sha); db.put(sha.trim().as_bytes(), bytes).context(Db) @@ -1358,15 +1358,16 @@ pub fn check_cleanup_merged_pr(state: &AppState, pr: &PullRequest) -> bool { pub fn cleanup_pr(state: &AppState, owner: &str, repo: &str, number: i64) { let AppState { db, .. } = state; - let mut iter = db.iterator(rocksdb::IteratorMode::Start); + let iter = db.iterator(rocksdb::IteratorMode::Start); for (key, value) in iter { let db_mr: MergeRequest = match bincode::deserialize(&value).context(Bincode) { Ok(mr) => mr, Err(err) => { log::error!( - "Failed to deserialize {} during cleanup_pr", - String::from_utf8_lossy(&key) + "Failed to deserialize {} during cleanup_pr due to {:?}", + String::from_utf8_lossy(&key), + err ); continue; } @@ -1379,7 +1380,7 @@ pub fn cleanup_pr(state: &AppState, owner: &str, repo: &str, number: i64) { continue; } - if let Err(err) = db.delete(key) { + if let Err(err) = db.delete(&key) { log::error!( "Failed to delete {} during cleanup_pr due to {:?}", String::from_utf8_lossy(&key), @@ -1403,11 +1404,7 @@ pub async fn merge( return Ok(Ok(())); } - let AppState { - github_bot, - bot_config, - .. - } = state; + let AppState { github_bot, .. } = state; match pr.head_sha() { Ok(pr_head_sha) => match github_bot .merge_pull_request(&pr.base.repo.owner.login, &pr.base.repo.name, pr.number, pr_head_sha) @@ -1469,12 +1466,12 @@ pub async fn merge( .as_str() .parse::() .unwrap(); - match merge_allowed( + match check_merge_is_allowed( state, pr, requested_by, Some(min_approvals_required), - false + PendingCompanionStatusesRestriction::Disallow ) .await { @@ -1544,9 +1541,9 @@ Pull Request Merge Endpoint responded with unexpected body: `{}`", .map_err(|e| Error::Merge { source: Box::new(e), commit_sha: pr_head_sha.to_string(), - pr_url: pr.url, - owner: pr.base.repo.owner.login, - repo_name: pr.base.repo.name, + pr_url: pr.url.to_owned(), + owner: pr.base.repo.owner.login.to_owned(), + repo_name: pr.base.repo.name.to_owned(), pr_number: pr.number, created_approval_id }), @@ -1785,86 +1782,73 @@ async fn handle_error( } /// Try queueing this merge through the parent if some PR is depending on it -async fn attempt_merge_as_companion_fallback_merge( +async fn attempt_merge_as_companion_fallback_direct( state: &AppState, pr: &PullRequest, requested_by: &str, ) -> Result<()> { - let AppState { - db, - github_bot, - bot_config, - .. - } = state; + let AppState { db, github_bot, .. } = state; - let pr_head_sha = pr.head_sha()?; + let iter = db.iterator(rocksdb::IteratorMode::Start); - // Iteration is restarted after merge because merges might clean up not only this key, but also - // others which could come before it - 'restart_iteration: loop { - let mut iter = db.iterator(rocksdb::IteratorMode::Start); + let was_queued_as_companion = { + let mut was_queued_as_companion: Option = None; - for (sha, value) in iter { + 'whole_db_iteration: for (_, value) in iter { let db_mr: MergeRequest = - match bincode::deserialize(&value).context(Bincode) { - Ok(mr) => mr, - Err(err) => { - log::error!( - "Failed to deserialize {} during attempt_merge_as_companion_fallback_merge", - String::from_utf8_lossy(sha) - ); - continue; - } - }; + bincode::deserialize(&value).context(Bincode)?; - let companion_children = match db_mr.companion_children { + let companion_children = match db_mr.companion_children.as_ref() { Some(companion_children) => companion_children, None => continue, }; for child in companion_children { - if child.owner != pr.base.repo.owner + if child.owner != pr.base.repo.owner.login || child.repo != pr.base.repo.name || child.number != pr.number { continue; } + let html_url = db_mr.html_url.to_string(); + let html_url = &html_url; log::info!( "{} is listed as a companion of {} in the database", - pr.html_url, - db_mr.html_url + &pr.html_url, + html_url ); - if let Ok(true) = async { + let requested_by = db_mr.requested_by.to_string(); + let result: Result = async { let pr_from_db_mr = github_bot .pull_request( - db_mr.owner, - db_mr.repo_name, + &db_mr.owner, + &db_mr.repo_name, db_mr.number, ) .await?; - if check_cleanup_merged_pr(state, pr_from_db_mr, None) - .await? - { + if check_cleanup_merged_pr(state, &pr_from_db_mr) { return Ok(false); } let companions = match pr_from_db_mr .body + .as_ref() .map(|body| parse_all_companions(body)) { Some(companions) => companions, None => return Ok(false), }; - // Check if this MR would be queued as a companion if we were to merge the MR parsed from - // the database (the current iteration's value) + // Check if the MR provided to this function would be queued as a companion if we were to + // merge the MR parsed from the database (the current iteration's value) if companions .iter() .find(|(_, owner, repo, number)| { - owner == pr.owner - && repo == pr.repo && number == pr.number + owner == &pr.base.repo.owner.login + && repo == &pr.base.repo.name && *number + == pr.number }) .is_none() { @@ -1873,76 +1857,72 @@ async fn attempt_merge_as_companion_fallback_merge( log::info!( "{} is listed as a companion of {} in the database", - pr.html_url, - db_mr.html_url + &pr.html_url, + html_url ); - merge_allowed( + check_merge_is_allowed( state, &pr_from_db_mr, - &db_mr.requested_by, + &requested_by, None, - false, + PendingCompanionStatusesRestriction::Disallow, ) .await?; - if ready_to_merge(state.github_bot, &pr_from_db_mr).await? { + if ready_to_merge(github_bot, &pr_from_db_mr).await? { merge(state, &pr_from_db_mr, &db_mr.requested_by, None) .await??; merge_companions(state, &pr_from_db_mr).await?; } else { - wait_to_merge( + let pr_from_db_mr_head_sha = + pr_from_db_mr.head_sha()?.to_owned(); + register_merge_request( state, - pr_from_db_mr.head_sha()?, + &pr_from_db_mr_head_sha, &MergeRequest { - owner: pr_from_db_mr.base.repo.owner, + owner: pr_from_db_mr.base.repo.owner.login, repo_name: pr_from_db_mr.base.repo.name, number: pr_from_db_mr.number, html_url: pr_from_db_mr.html_url, - requested_by: db_mr.requested_by, - // Reparse the companion's bodies so that we'll be able to - companion_children: companions - .into_iter() - .map(|(_, owner, repo, number)| { - MergeRequestBase { - owner, - repo, - number, - } - }) - .collect(), + requested_by, + companion_children: Some( + companions + .into_iter() + .map(|(_, owner, repo, number)| { + MergeRequestBase { + owner, + repo, + number, + } + }) + .collect(), + ), }, - None, ) .await?; } Ok(true) } - .await - { - // See the note about restarting iteration at the top of this loop - continue 'restart_iteration; - }; + .await; + if let Ok(true) = result { + was_queued_as_companion = Some(true); + break 'whole_db_iteration; + } } } - break; - } - - // If the PR was created as a companion, or is already pending, there's no point in trying to - // merge it here - if db.get(pr_head_sha).context(Db)?.is_some() { - return Ok(()); - } + was_queued_as_companion.unwrap_or(false) + }; - merge_allowed(state, pr, requested_by, None, true).await?; - if ready_to_merge(github_bot, &pr).await? { - match merge(state, &pr, requested_by, None).await? { - Ok(_) => (), - Err(Error::MergeFailureWillBeSolvedLater { .. }) => return Ok(()), - Err(err) => return Err(err), + if !was_queued_as_companion && ready_to_merge(github_bot, &pr).await? { + return match merge(state, &pr, requested_by, None).await? { + Ok(_) => merge_companions(state, &pr).await, + Err(Error::MergeFailureWillBeSolvedLater { .. }) => Ok(()), + Err(err) => Err(err), }; - merge_companions(state, &pr).await?; } + + Ok(()) } From c135dfa2ea83faf5b956658f3ec1ef3d678cd1ee Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 03:10:50 -0300 Subject: [PATCH 07/45] wip --- src/companion.rs | 2 +- src/github.rs | 8 ++++---- src/main.rs | 21 +++++++++++++++++---- src/webhook.rs | 42 ++++++++++++++++++++---------------------- 4 files changed, 42 insertions(+), 31 deletions(-) diff --git a/src/companion.rs b/src/companion.rs index a62cf657..ea0002d4 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -234,7 +234,7 @@ async fn update_companion_repository( }), ) .await?; - if !String::from_utf8_lossy(&(&output).stdout[..]) + if !String::from_utf8_lossy(&output.stdout[..]) .trim() .is_empty() { diff --git a/src/github.rs b/src/github.rs index 5875b178..cde2b6fb 100644 --- a/src/github.rs +++ b/src/github.rs @@ -56,7 +56,7 @@ impl HasIssueDetails for PullRequest { .. }) = self.repository.as_ref() { - parse_repository_full_name(&full_name) + parse_repository_full_name(full_name) .map(|(owner, name)| (owner, name, self.number)) } else { None @@ -124,7 +124,7 @@ impl HasIssueDetails for Issue { None } }) - .or_else(|| parse_issue_details_from_pr_html_url(&html_url)), + .or_else(|| parse_issue_details_from_pr_html_url(html_url)), _ => None, } } @@ -503,7 +503,7 @@ pub fn parse_issue_details_from_pr_html_url( pr_html_url: &str, ) -> Option { let re = Regex::new(PR_HTML_URL_REGEX!()).unwrap(); - let matches = re.captures(&pr_html_url)?; + let matches = re.captures(pr_html_url)?; let owner = matches.name("owner")?.as_str().to_owned(); let repo = matches.name("repo")?.as_str().to_owned(); let number = matches @@ -516,7 +516,7 @@ pub fn parse_issue_details_from_pr_html_url( } pub fn parse_repository_full_name(full_name: &str) -> Option<(String, String)> { - let parts: Vec<&str> = full_name.split("/").collect(); + let parts: Vec<&str> = full_name.split('/').collect(); parts .get(0) .map(|owner| { diff --git a/src/main.rs b/src/main.rs index f2963845..73652c91 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,7 @@ use rocksdb::DB; +use std::fs; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; +use std::path::Path; use std::sync::Arc; use tokio::sync::Mutex; mod logging; @@ -25,6 +27,17 @@ async fn run() -> anyhow::Result<()> { .format(logging::gke::format) .init(); + let db_version_file = + Path::new(&config.db_path).join("__PROCESSBOT_VERSION__"); + if !db_version_file.exists() { + log::info!("Clearing database to start from version 1"); + for entry in fs::read_dir(&config.db_path)? { + let entry = entry?; + fs::remove_file(entry.path())?; + } + fs::write(db_version_file, "V1").unwrap(); + } + let db = DB::open_default(&config.db_path)?; log::info!( @@ -96,10 +109,10 @@ async fn run() -> anyhow::Result<()> { */ let app_state = Arc::new(Mutex::new(AppState { - db: db, - github_bot: github_bot, - matrix_bot: matrix_bot, - gitlab_bot: gitlab_bot, + db, + github_bot, + matrix_bot, + gitlab_bot, bot_config: BotConfig::from_env(), webhook_secret: config.webhook_secret, })); diff --git a/src/webhook.rs b/src/webhook.rs index ea3415c7..8cd04c22 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -245,14 +245,16 @@ async fn handle_payload( }, }, Payload::CommitStatus { sha, state: status } => { - (handle_status(&sha, status, state).await, Some(sha)) + (handle_status(state, status, &sha).await, Some(sha)) } Payload::CheckRun { check_run: CheckRun { - status, head_sha, .. + status, + head_sha: sha, + .. }, .. - } => (handle_check(&head_sha, status, state).await, Some(head_sha)), + } => (handle_check(state, status, &sha).await, Some(sha)), _ => (Ok(()), None), }; @@ -304,12 +306,12 @@ async fn handle_payload( /// If a check completes, query if all statuses and checks are complete. async fn handle_check( - commit_sha: &String, - status: CheckRunStatus, state: &AppState, + status: CheckRunStatus, + commit_sha: &str, ) -> Result<()> { if status == CheckRunStatus::Completed { - checks_and_status(state, &commit_sha).await + checks_and_status(state, commit_sha).await } else { Ok(()) } @@ -317,9 +319,9 @@ async fn handle_check( /// If we receive a status other than `Pending`, query if all statuses and checks are complete. async fn handle_status( - commit_sha: &String, - status: StatusState, state: &AppState, + status: StatusState, + commit_sha: &str, ) -> Result<()> { if status == StatusState::Pending { Ok(()) @@ -335,7 +337,7 @@ pub async fn get_latest_statuses_state( commit_sha: &str, html_url: &str, ) -> Result { - let status = github_bot.status(&owner, &repo, &commit_sha).await?; + let status = github_bot.status(owner, repo, commit_sha).await?; log::info!("{:?}", status); // Since Github only considers the latest instance of each status, we should abide by the same @@ -1651,7 +1653,7 @@ fn get_troubleshoot_msg() -> String { fn display_errors_along_the_way(errors: Option>) -> String { errors .map(|errors| { - if errors.len() == 0 { + if errors.is_empty() { "".to_string() } else { format!( @@ -1843,15 +1845,11 @@ async fn attempt_merge_as_companion_fallback_direct( }; // Check if the MR provided to this function would be queued as a companion if we were to // merge the MR parsed from the database (the current iteration's value) - if companions - .iter() - .find(|(_, owner, repo, number)| { - owner == &pr.base.repo.owner.login - && repo == &pr.base.repo.name && *number - == pr.number - }) - .is_none() - { + if !companions.iter().any(|(_, owner, repo, number)| { + owner == &pr.base.repo.owner.login + && repo == &pr.base.repo.name + && *number == pr.number + }) { return Ok(false); } @@ -1916,9 +1914,9 @@ async fn attempt_merge_as_companion_fallback_direct( was_queued_as_companion.unwrap_or(false) }; - if !was_queued_as_companion && ready_to_merge(github_bot, &pr).await? { - return match merge(state, &pr, requested_by, None).await? { - Ok(_) => merge_companions(state, &pr).await, + if !was_queued_as_companion && ready_to_merge(github_bot, pr).await? { + return match merge(state, pr, requested_by, None).await? { + Ok(_) => merge_companions(state, pr).await, Err(Error::MergeFailureWillBeSolvedLater { .. }) => Ok(()), Err(err) => Err(err), }; From 3ac90d37f8e1849a8f56a91bee222ada8947e174 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 03:15:07 -0300 Subject: [PATCH 08/45] wi --- src/constants.rs | 4 ++++ src/main.rs | 17 ++++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/constants.rs b/src/constants.rs index 24a2f0ae..c65577e8 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -7,3 +7,7 @@ pub const PROCESS_FILE: &str = "Process.json"; pub const MAIN_REPO_FOR_STAGING: &str = "main-for-processbot-staging"; pub const BOT_NAME_FOR_COMMITS: &str = "parity-processbot[bot]"; + +// Note: the old database will be deleted when switching to a new version, so do not change this +// without checking the implementation first +pub const DATABASE_VERSION: &str = "V1"; diff --git a/src/main.rs b/src/main.rs index 73652c91..b344bb81 100644 --- a/src/main.rs +++ b/src/main.rs @@ -8,6 +8,7 @@ mod logging; use parity_processbot::{ config::{BotConfig, MainConfig}, + constants::*, github_bot, gitlab_bot, matrix_bot, server::*, webhook::*, @@ -29,13 +30,23 @@ async fn run() -> anyhow::Result<()> { let db_version_file = Path::new(&config.db_path).join("__PROCESSBOT_VERSION__"); - if !db_version_file.exists() { - log::info!("Clearing database to start from version 1"); + let is_at_current_db_version = match db_version_file.exists() { + true => { + let str = fs::read_to_string(&db_version_file)?; + str == DATABASE_VERSION + } + false => false, + }; + if !is_at_current_db_version { + log::info!( + "Clearing database to start from version {}", + DATABASE_VERSION + ); for entry in fs::read_dir(&config.db_path)? { let entry = entry?; fs::remove_file(entry.path())?; } - fs::write(db_version_file, "V1").unwrap(); + fs::write(db_version_file, DATABASE_VERSION).unwrap(); } let db = DB::open_default(&config.db_path)?; From 4015492d08392ffeb5c643927204fb366dc2a2fe Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 03:24:51 -0300 Subject: [PATCH 09/45] fix --- src/main.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index b344bb81..b9a1c050 100644 --- a/src/main.rs +++ b/src/main.rs @@ -44,7 +44,11 @@ async fn run() -> anyhow::Result<()> { ); for entry in fs::read_dir(&config.db_path)? { let entry = entry?; - fs::remove_file(entry.path())?; + if entry.metadata()?.is_dir() { + fs::remove_dir_all(entry.path())?; + } else { + fs::remove_file(entry.path())?; + } } fs::write(db_version_file, DATABASE_VERSION).unwrap(); } From 246e8c215c289943886d6e12a85c05947a69f5cf Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 03:26:49 -0300 Subject: [PATCH 10/45] fix --- src/main.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/main.rs b/src/main.rs index b9a1c050..0efff695 100644 --- a/src/main.rs +++ b/src/main.rs @@ -28,11 +28,11 @@ async fn run() -> anyhow::Result<()> { .format(logging::gke::format) .init(); - let db_version_file = + let db_version_path = Path::new(&config.db_path).join("__PROCESSBOT_VERSION__"); - let is_at_current_db_version = match db_version_file.exists() { + let is_at_current_db_version = match db_version_path.exists() { true => { - let str = fs::read_to_string(&db_version_file)?; + let str = fs::read_to_string(&db_version_path)?; str == DATABASE_VERSION } false => false, @@ -44,13 +44,16 @@ async fn run() -> anyhow::Result<()> { ); for entry in fs::read_dir(&config.db_path)? { let entry = entry?; + if entry.path() == db_version_path { + continue; + } if entry.metadata()?.is_dir() { fs::remove_dir_all(entry.path())?; } else { fs::remove_file(entry.path())?; } } - fs::write(db_version_file, DATABASE_VERSION).unwrap(); + fs::write(db_version_path, DATABASE_VERSION).unwrap(); } let db = DB::open_default(&config.db_path)?; From 65cffe675a8e664353b2beedc3947d071bc5814b Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 04:14:15 -0300 Subject: [PATCH 11/45] wip --- src/companion.rs | 4 +- src/constants.rs | 2 +- src/webhook.rs | 134 +++++++++++++++++++++++++++++++---------------- 3 files changed, 91 insertions(+), 49 deletions(-) diff --git a/src/companion.rs b/src/companion.rs index ea0002d4..748ddc68 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -464,7 +464,7 @@ async fn update_then_merge_companion( let AppState { github_bot, .. } = state; let pr = github_bot.pull_request(&owner, &repo, *number).await?; - if check_cleanup_merged_pr(state, &pr) { + if check_cleanup_merged_pr(state, &pr)? { return Ok(()); } @@ -511,7 +511,7 @@ async fn update_then_merge_companion( &updated_sha, &MergeRequest { owner: owner.to_owned(), - repo_name: repo.to_owned(), + repo: repo.to_owned(), number: number.to_owned(), html_url: html_url.to_owned(), requested_by: BOT_NAME_FOR_COMMITS.to_owned(), diff --git a/src/constants.rs b/src/constants.rs index c65577e8..23c71a5a 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -10,4 +10,4 @@ pub const BOT_NAME_FOR_COMMITS: &str = "parity-processbot[bot]"; // Note: the old database will be deleted when switching to a new version, so do not change this // without checking the implementation first -pub const DATABASE_VERSION: &str = "V1"; +pub const DATABASE_VERSION: &str = "v1.0"; diff --git a/src/webhook.rs b/src/webhook.rs index 8cd04c22..987c4e0a 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -43,7 +43,7 @@ pub struct MergeRequestBase { #[repr(C)] pub struct MergeRequest { pub owner: String, - pub repo_name: String, + pub repo: String, pub number: i64, pub html_url: String, pub requested_by: String, @@ -264,24 +264,50 @@ async fn handle_payload( if err.stops_merge_attempt() { if let Some(sha) = sha { match state.db.get(sha.as_bytes()) { - Ok(Some(_)) => match state.db.delete(sha.as_bytes()) { - Ok(_) => { - log::info!( - "MR for {} was cancelled due to {:?}", - sha, - err - ); - MergeCancelOutcome::WasCancelled - } - Err(db_err) => { - log::error!( - "Failed to delete {} from the database due to {:?}", + Ok(Some(bytes)) => { + match bincode::deserialize::(&bytes) + .context(Bincode) + { + Ok(MergeRequest { + ref owner, + ref repo, + ref html_url, + number, + .. + }) => { + match cleanup_pr( + state, &sha, owner, repo, number, + ) { + Ok(_) => { + log::info!( + "Merge of {} (sha {}) was cancelled due to {:?}", + html_url, + sha, + err + ); + MergeCancelOutcome::WasCancelled + } + Err(err) => { + log::error!( + "Failed to cancel merge of {} (sha {}) in handle_payload due to {:?}", + html_url, + sha, + err + ); + MergeCancelOutcome::WasNotCancelled + } + } + } + Err(db_err) => { + log::error!( + "Failed to parse {} from the database due to {:?}", &sha, db_err ); - MergeCancelOutcome::WasNotCancelled + MergeCancelOutcome::WasNotCancelled + } } - }, + } Ok(None) => MergeCancelOutcome::ShaDidNotExist, Err(db_err) => { log::info!( @@ -452,7 +478,7 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { async { let MergeRequest { owner, - repo_name, + repo, number, html_url, requested_by, @@ -460,7 +486,7 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { } = &mr; let AppState { github_bot, .. } = state; - let pr = github_bot.pull_request(owner, repo_name, *number).await?; + let pr = github_bot.pull_request(owner, repo, *number).await?; let pr_head_sha = pr.head_sha()?; if sha != pr_head_sha { @@ -471,13 +497,13 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { } let status = get_latest_statuses_state( - github_bot, &owner, &repo_name, sha, &html_url, + github_bot, &owner, &repo, sha, &html_url, ) .await?; match status { Status::Success => { let checks = get_latest_checks_state( - github_bot, &owner, &repo_name, sha, &html_url, + github_bot, &owner, &repo, sha, &html_url, ) .await?; match checks { @@ -534,7 +560,7 @@ async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { } } .await - .map_err(|err| err.map_issue((mr.owner, mr.repo_name, mr.number))) + .map_err(|err| err.map_issue((mr.owner, mr.repo, mr.number))) } async fn handle_command( @@ -543,7 +569,7 @@ async fn handle_command( pr: &PullRequest, requested_by: &str, ) -> Result<()> { - let AppState { db, github_bot, .. } = state; + let AppState { github_bot, .. } = state; match cmd { CommentCommand::Merge(cmd) => { @@ -569,7 +595,7 @@ async fn handle_command( MergeCommentCommand::Normal => { let mr = MergeRequest { owner: pr.base.repo.owner.login.to_owned(), - repo_name: pr.base.repo.name.to_owned(), + repo: pr.base.repo.name.to_owned(), number: pr.number, html_url: pr.html_url.to_owned(), requested_by: requested_by.to_owned(), @@ -644,7 +670,6 @@ async fn handle_command( } } - db.delete(pr_head_sha.as_bytes()).context(Db)?; merge_companions(state, pr).await } CommentCommand::CancelMerge => { @@ -652,6 +677,7 @@ async fn handle_command( cleanup_pr( state, + pr.head_sha()?, &pr.base.repo.owner.login, &pr.base.repo.name, pr.number, @@ -802,7 +828,7 @@ async fn handle_comment( Some(cmd) => cmd, None => return (None, Ok(())), }; - log::info!("{:?} requested by {}", cmd, requested_by); + log::info!("{:?} requested by {} in {}", cmd, requested_by, html_url); let AppState { github_bot, .. } = state; @@ -1322,7 +1348,7 @@ pub async fn wait_to_merge( let MergeRequest { owner, - repo_name, + repo, number, .. } = mr; @@ -1330,7 +1356,7 @@ pub async fn wait_to_merge( let post_comment_result = github_bot .create_issue_comment( owner, - repo_name, + repo, *number, msg.unwrap_or("Waiting for commit status."), ) @@ -1342,22 +1368,32 @@ pub async fn wait_to_merge( Ok(()) } -pub fn check_cleanup_merged_pr(state: &AppState, pr: &PullRequest) -> bool { +pub fn check_cleanup_merged_pr( + state: &AppState, + pr: &PullRequest, +) -> Result { if !pr.merged { - return false; + return Ok(false); } + let key_to_guarantee_deleted = pr.head_sha()?; cleanup_pr( state, + key_to_guarantee_deleted, &pr.base.repo.owner.login, &pr.base.repo.name, pr.number, - ); - - true + ) + .map(|_| true) } -pub fn cleanup_pr(state: &AppState, owner: &str, repo: &str, number: i64) { +pub fn cleanup_pr( + state: &AppState, + key_to_guarantee_deleted: &str, + owner: &str, + repo: &str, + number: i64, +) -> Result<()> { let AppState { db, .. } = state; let iter = db.iterator(rocksdb::IteratorMode::Start); @@ -1375,9 +1411,7 @@ pub fn cleanup_pr(state: &AppState, owner: &str, repo: &str, number: i64) { } }; - if db_mr.owner != owner - || db_mr.repo_name != repo - || db_mr.number != number + if db_mr.owner != owner || db_mr.repo != repo || db_mr.number != number { continue; } @@ -1390,6 +1424,17 @@ pub fn cleanup_pr(state: &AppState, owner: &str, repo: &str, number: i64) { ); } } + + if let Some(_) = db.get(key_to_guarantee_deleted).context(Db)? { + return Err(Error::Message { + msg: format!( + "Key {} was not deleted from the database", + key_to_guarantee_deleted + ), + }); + } + + Ok(()) } /// Send a merge request. @@ -1402,7 +1447,7 @@ pub async fn merge( requested_by: &str, created_approval_id: Option, ) -> Result> { - if check_cleanup_merged_pr(state, pr) { + if check_cleanup_merged_pr(state, pr)? { return Ok(Ok(())); } @@ -1414,7 +1459,9 @@ pub async fn merge( { Ok(_) => { log::info!("{} merged successfully.", pr.html_url); - cleanup_pr(state, &pr.base.repo.owner.login, &pr.base.repo.name, pr.number); + if let Err(err) = cleanup_pr(state, pr_head_sha, &pr.base.repo.owner.login, &pr.base.repo.name, pr.number) { + log::error!("{}", err); + }; Ok(Ok(())) } Err(e) => match e { @@ -1791,11 +1838,10 @@ async fn attempt_merge_as_companion_fallback_direct( ) -> Result<()> { let AppState { db, github_bot, .. } = state; - let iter = db.iterator(rocksdb::IteratorMode::Start); - let was_queued_as_companion = { let mut was_queued_as_companion: Option = None; + let iter = db.iterator(rocksdb::IteratorMode::Start); 'whole_db_iteration: for (_, value) in iter { let db_mr: MergeRequest = bincode::deserialize(&value).context(Bincode)?; @@ -1824,14 +1870,10 @@ async fn attempt_merge_as_companion_fallback_direct( let requested_by = db_mr.requested_by.to_string(); let result: Result = async { let pr_from_db_mr = github_bot - .pull_request( - &db_mr.owner, - &db_mr.repo_name, - db_mr.number, - ) + .pull_request(&db_mr.owner, &db_mr.repo, db_mr.number) .await?; - if check_cleanup_merged_pr(state, &pr_from_db_mr) { + if check_cleanup_merged_pr(state, &pr_from_db_mr)? { return Ok(false); } @@ -1880,7 +1922,7 @@ async fn attempt_merge_as_companion_fallback_direct( &pr_from_db_mr_head_sha, &MergeRequest { owner: pr_from_db_mr.base.repo.owner.login, - repo_name: pr_from_db_mr.base.repo.name, + repo: pr_from_db_mr.base.repo.name, number: pr_from_db_mr.number, html_url: pr_from_db_mr.html_url, requested_by, From eebdd1d1ff94c331814e65405d1bae28bbf3a413 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 04:19:29 -0300 Subject: [PATCH 12/45] wip --- src/webhook.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webhook.rs b/src/webhook.rs index 987c4e0a..4460cb95 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -681,7 +681,7 @@ async fn handle_command( &pr.base.repo.owner.login, &pr.base.repo.name, pr.number, - ); + )?; if let Err(err) = github_bot .create_issue_comment( From 73ca619a91331e281a8d4c3e8ee447fa1bcd52b1 Mon Sep 17 00:00:00 2001 From: joao-paulo-parity Date: Sun, 10 Oct 2021 04:57:07 -0300 Subject: [PATCH 13/45] wip --- src/companion.rs | 15 ++++-- src/github.rs | 3 +- src/lib.rs | 5 -- src/webhook.rs | 127 ++++++++++++++++++++++++----------------------- 4 files changed, 76 insertions(+), 74 deletions(-) diff --git a/src/companion.rs b/src/companion.rs index 748ddc68..6ccaa40e 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -376,11 +376,16 @@ pub async fn check_all_companions_are_mergeable( }); } - let is_mergeable = companion - .mergeable - .map(|mergeable| mergeable) - .unwrap_or(false); - if !is_mergeable { + if !companion.maintainer_can_modify { + return Err(Error::Message { + msg: format!( + "Github API says \"Allow edits from maintainers\" is not enabled for {}. The bot would use that permission to push the lockfile update after merging this PR. Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.", + html_url + ), + }); + } + + if !companion.mergeable { return Err(Error::Message { msg: format!( "Github API says companion {} is not mergeable", diff --git a/src/github.rs b/src/github.rs index cde2b6fb..73c54ef9 100644 --- a/src/github.rs +++ b/src/github.rs @@ -31,11 +31,12 @@ pub struct PullRequest { pub user: Option, pub body: Option, pub labels: Vec