diff --git a/.env.example b/.env.example index 5fb9ac6..36e1840 100644 --- a/.env.example +++ b/.env.example @@ -26,21 +26,13 @@ WEBHOOK_SECRET=placeholder # The Github App ID according to the Github App's settings. GITHUB_APP_ID=123 -# The GitHub team which whose members will be used as team leads -# Must be from the organization where the GitHub App is installed -TEAM_LEADS_TEAM=placeholder - -# The GitHub team which whose members will be used as core developers -# Must be from the organization where the GitHub App is installed -CORE_DEVS_TEAM=placeholder - - # --- OPTIONAL VARIABLES --- # If you set this variable, the application will receive events from Smee and a # local server will not be started # WEBHOOK_PROXY_URL=https://smee.io/parity-processbot -# Disable organization checks such as the substrateteamleads and core-devs checks. +# Disable organization checks for using the bot. Useful if you're using the bot +# in your own account and not an organization. # DISABLE_ORG_CHECK=true # Configure which prefix to use for detecting sources in dependencies diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 4802e16..3de3461 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -106,8 +106,6 @@ dockerize-processbot: --set "app.PROCESSBOT_KEY=${PROCESSBOT_KEY}" --set "app.GITHUB_APP_ID=${GITHUB_APP_ID}" --set "app.WEBHOOK_SECRET=${WEBHOOK_SECRET}" - --set "app.TEAM_LEADS_TEAM=${TEAM_LEADS_TEAM}" - --set "app.CORE_DEVS_TEAM=${CORE_DEVS_TEAM}" deploy-staging: stage: deploy diff --git a/README.md b/README.md index 66322ef..1b1850d 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,6 @@ Before starting to work on this project we recommend reading the - [Commands](#commands) - [Relation to CI](#commands-relation-to-ci) - [Criteria for merge](#criteria-for-merge) - - [Approvals](#criteria-for-merge-approvals) - [Checks and statuses](#criteria-for-merge-checks-and-statuses) - [GitHub App](#github-app) - [Configuration](#github-app-configuration) @@ -51,14 +50,13 @@ description of how that works internally is provided in the The following commands should be posted as pull request comments. **Your whole comment should only have the command**. -- `bot merge`: [if approved](#criteria-for-merge), merge once checks pass. -- `bot merge force`: [if approved](#criteria-for-merge), merge immediately - while disregarding checks - ([not all of them can be disregarded](#criteria-for-merge-checks-and-statuses)). +- `bot merge`: merge once checks pass +- `bot merge force`: merge immediately while disregarding checks + ([not all of them can be disregarded](#criteria-for-merge-checks-and-statuses)) - `bot merge cancel`: cancel a pending `bot merge`; does not affect anything outside of processbot, only stops the bot from following through with the - merge. -- `bot rebase`: create a merge commit from origin/master into the PR. + merge +- `bot rebase`: create a merge commit from origin/master into the PR Note: The commands will only work if you are a member of the organization where the GitHub App is installed. Organization membership is fetched from the GitHub @@ -94,45 +92,6 @@ which will allow processbot to detect and disregard them. # Criteria for merge -## Approvals - -A Pull Request needs either (meaning, only **one of** the following -requirements needs to be fulfilled) - -- [core-dev](https://github.com/orgs/paritytech/teams/core-devs) member approvals (2 for Substrate, 1 otherwise), or -- One [substrateteamleads](https://github.com/orgs/paritytech/teams/substrateteamleads) member approval - -This criterion strictly matters only for the bot's internal logic irrespective -of GitHub Repository Settings and will not trump the latter in any case. For -instance, the rule: - -> One substrateteamleads member approval - -does not imply that the pull request will be mergeable if the GitHub Settings -require more approvals than that. The bot's rules work *in addition* to the -repository's settings while still respecting them. Specifically when it comes -to the approvals' count, however, the bot might able to help if a -[team lead](https://github.com/orgs/paritytech/teams/substrateteamleads) -is requesting the merge. - -When the bot is commanded to merge, if the PR is short of 1 approval and the -command's requester might not be able to fulfill the approval count on their -own, then the bot will try to pitch in the missing approval if the requester is -a [team lead](https://github.com/orgs/paritytech/teams/substrateteamleads). -The reasoning for this feature is as follows: - -1. PR authors cannot approve their own merge requests, although - [team leads](https://github.com/orgs/paritytech/teams/substrateteamleads) - should have the means to bypass that requirement e.g. for trivial or urgent - changes. - -2. If the - [team lead](https://github.com/orgs/paritytech/teams/substrateteamleads) - has already approved and it's still - short of one, they cannot "approve twice" in order to meet the quota. In that - case the bot should contribute one approval in order to help them meet the - approval count. - ## Checks and statuses All [Important and above](#commands-relation-to-ci) checks should be green when diff --git a/helm/templates/processbot.yaml b/helm/templates/processbot.yaml index 8f87d39..3ed8b45 100644 --- a/helm/templates/processbot.yaml +++ b/helm/templates/processbot.yaml @@ -81,10 +81,6 @@ spec: value: {{ quote .Values.app.START_FROM_CWD }} - name: GITHUB_APP_ID value: {{ quote .Values.app.GITHUB_APP_ID }} - - name: CORE_DEVS_TEAM - value: {{ .Values.app.CORE_DEVS_TEAM }} - - name: TEAM_LEADS_TEAM - value: {{ .Values.app.TEAM_LEADS_TEAM }} - name: DB_PATH value: {{ .Values.config.storagePath }}/db - name: REPOSITORIES_PATH diff --git a/helm/values.yaml b/helm/values.yaml index 543cd41..b696d9b 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -5,8 +5,6 @@ app: PROCESSBOT_KEY: from-gitlab-vars WEBHOOK_SECRET: from-gitlab-vars KUBE_NAMESPACE: from-gitlab-vars - CORE_DEVS_TEAM: from-gitlab-vars - TEAM_LEADS_TEAM: from-gitlab-vars START_FROM_CWD: true config: diff --git a/src/companion.rs b/src/companion.rs index 29ed1ad..6a04129 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -12,11 +12,12 @@ use crate::{ error::*, github::*, webhook::{ - check_merge_is_allowed, cleanup_pr, handle_dependents_after_merge, - handle_merged_pr, merge, ready_to_merge, wait_to_merge, AppState, - MergeRequest, PullRequestCleanupReason, WaitToMergeMessage, + check_merge_is_allowed, cleanup_pr, get_latest_statuses_state, + handle_dependents_after_merge, handle_merged_pr, merge, ready_to_merge, + wait_to_merge, AppState, MergeRequest, PullRequestCleanupReason, + WaitToMergeMessage, }, - MergeAllowedOutcome, Result, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, + Result, COMPANION_LONG_REGEX, COMPANION_PREFIX_REGEX, COMPANION_SHORT_REGEX, OWNER_AND_REPO_SEQUENCE, PR_HTML_URL_REGEX, }; @@ -507,6 +508,32 @@ pub async fn check_all_companions_are_mergeable( }); } + /* + FIXME: Get rid of this ugly hack once the Companion Build System doesn't + ignore the companion's CI + */ + let latest_statuses = get_latest_statuses_state( + github_bot, + &companion.base.repo.owner.login, + &companion.base.repo.name, + &companion.head.sha, + &companion.html_url, + ) + .await? + .1; + let reviews_are_passing = latest_statuses + .get("Check reviews") + .map(|(_, state)| state == &StatusState::Success) + .unwrap_or(false); + if !reviews_are_passing { + return Err(Error::Message { + msg: format!( + "pr-custom-review is not passing for {}", + &companion.html_url + ), + }); + } + // Keeping track of the trail of references is necessary to break chains like A -> B -> C -> A // TODO: of course this should be tested let next_companion_reference_trail = { @@ -519,17 +546,14 @@ pub async fn check_all_companions_are_mergeable( }); next_trail }; - if let MergeAllowedOutcome::Disallowed(msg) = check_merge_is_allowed( + + check_merge_is_allowed( state, &companion, requested_by, - None, &next_companion_reference_trail, ) - .await? - { - return Err(Error::Message { msg }); - } + .await?; } Ok(()) @@ -564,14 +588,8 @@ pub async fn update_then_merge( } (None, comp_pr) } else { - check_merge_is_allowed( - state, - &comp_pr, - &comp.requested_by, - None, - &[], - ) - .await?; + check_merge_is_allowed(state, &comp_pr, &comp.requested_by, &[]) + .await?; let dependencies_to_update = if let Some(ref dependencies) = comp.dependencies { @@ -654,8 +672,7 @@ pub async fn update_then_merge( "Attempting to merge {} after companion update", comp_pr.html_url ); - if let Err(err) = - merge(state, &comp_pr, &comp.requested_by, None).await? + if let Err(err) = merge(state, &comp_pr, &comp.requested_by).await? { match err { Error::MergeFailureWillBeSolvedLater { .. } => {} diff --git a/src/config.rs b/src/config.rs index 4a3b43d..5b620ff 100644 --- a/src/config.rs +++ b/src/config.rs @@ -14,8 +14,6 @@ pub struct MainConfig { pub github_api_url: String, pub companion_status_settle_delay: u64, pub merge_command_delay: u64, - pub core_devs_team: String, - pub team_leads_team: String, pub github_source_prefix: String, pub github_source_suffix: String, } @@ -88,11 +86,6 @@ impl MainConfig { let companion_status_settle_delay = 4096; - let core_devs_team = - dotenv::var("CORE_DEVS_TEAM").expect("CORE_DEVS_TEAM"); - let team_leads_team = - dotenv::var("TEAM_LEADS_TEAM").expect("TEAM_LEADS_TEAM"); - Self { installation_login, webhook_secret, @@ -105,8 +98,6 @@ impl MainConfig { github_api_url, merge_command_delay, companion_status_settle_delay, - core_devs_team, - team_leads_team, repos_path, github_source_prefix, github_source_suffix, diff --git a/src/error.rs b/src/error.rs index 32530e8..7069c6c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -35,12 +35,6 @@ pub enum Error { actual: String, }, - #[snafu(display("Missing approval."))] - Approval { - errors: Vec, - html_url: String, - }, - #[snafu(display("{}", msg))] Message { msg: String, @@ -77,11 +71,6 @@ pub enum Error { source: serde_json::Error, }, - #[snafu(display("Base64: {}", source))] - Base64 { - source: base64::DecodeError, - }, - Jwt { source: jsonwebtoken::errors::Error, }, @@ -110,11 +99,6 @@ pub enum Error { MergeFailureWillBeSolvedLater { msg: String, }, - - #[snafu(display("Failed to merge companions: {:?}", errors))] - CompanionsFailedMerge { - errors: Vec, - }, } impl Error { diff --git a/src/github.rs b/src/github.rs index 9deec13..84e65c6 100644 --- a/src/github.rs +++ b/src/github.rs @@ -122,14 +122,6 @@ pub struct Base { pub repo: BaseRepo, } -#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct Review { - pub id: i64, - // User might be missing when it has been deleted - pub user: Option, - pub state: Option, -} - #[derive(PartialEq, Debug, Clone, Serialize, Deserialize)] pub enum UserType { User, @@ -181,15 +173,6 @@ pub enum StatusState { Unknown, } -#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] -#[serde(rename_all = "SCREAMING_SNAKE_CASE")] -pub enum ReviewState { - Approved, - ChangesRequested, - #[serde(other)] - Unknown, -} - #[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct InstallationRepositories { pub repositories: Vec, diff --git a/src/github_bot/mod.rs b/src/github_bot/mod.rs index cfcae4e..c7f13f2 100644 --- a/src/github_bot/mod.rs +++ b/src/github_bot/mod.rs @@ -3,8 +3,6 @@ use crate::{config::MainConfig, github::*, Result}; pub mod issue; pub mod project; pub mod pull_request; -pub mod review; -pub mod team; pub struct GithubBot { pub client: crate::http::Client, @@ -69,35 +67,4 @@ impl GithubBot { let status = self.client.get_status(url).await?; Ok(status == 204) // Github API returns HTTP 204 (No Content) if the user is a member } - - pub async fn approve_merge_request( - &self, - owner: &str, - repo: &str, - pr_number: i64, - ) -> Result { - let url = &format!( - "{}/repos/{}/{}/pulls/{}/reviews", - self.github_api_url, owner, repo, pr_number - ); - let body = &serde_json::json!({ "event": "APPROVE" }); - self.client.post(url, body).await - } - - pub async fn clear_merge_request_approval( - &self, - owner: &str, - repo: &str, - pr_number: i64, - review_id: i64, - ) -> Result { - let url = &format!( - "{}/repos/{}/{}/pulls/{}/reviews/{}/dismissals", - self.github_api_url, owner, repo, pr_number, review_id - ); - let body = &serde_json::json!({ - "message": "Merge failed despite bot approval, therefore the approval will be dismissed." - }); - self.client.put(url, body).await - } } diff --git a/src/github_bot/review.rs b/src/github_bot/review.rs deleted file mode 100644 index 92c47e0..0000000 --- a/src/github_bot/review.rs +++ /dev/null @@ -1,10 +0,0 @@ -use crate::{github, Result}; - -use super::GithubBot; - -impl GithubBot { - pub async fn reviews(&self, pr_url: &str) -> Result> { - let url = format!("{}/reviews", pr_url); - self.client.get_all(url).await - } -} diff --git a/src/github_bot/team.rs b/src/github_bot/team.rs deleted file mode 100644 index e6c4bde..0000000 --- a/src/github_bot/team.rs +++ /dev/null @@ -1,18 +0,0 @@ -use crate::{github, Result}; - -use super::GithubBot; - -impl GithubBot { - pub async fn team_members( - &self, - org: &str, - team: &str, - ) -> Result> { - self.client - .get_all(format!( - "{}/orgs/{}/teams/{}/members", - self.github_api_url, org, team, - )) - .await - } -} diff --git a/src/lib.rs b/src/lib.rs index 2687f1e..46b18e0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -50,9 +50,3 @@ pub enum MergeCancelOutcome { WasCancelled, WasNotCancelled, } - -pub enum MergeAllowedOutcome { - Allowed, - GrantApprovalForRole(String), - Disallowed(String), -} diff --git a/src/webhook.rs b/src/webhook.rs index 0f62577..6d9a811 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -2,7 +2,7 @@ use async_recursion::async_recursion; use futures::StreamExt; use html_escape; use hyper::{Body, Request, Response, StatusCode}; -use itertools::Itertools; + use regex::RegexBuilder; use ring::hmac; use rocksdb::DB; @@ -15,8 +15,8 @@ use tokio::{sync::Mutex, time::delay_for}; use crate::{ companion::*, config::MainConfig, error::*, github::*, github_bot::GithubBot, rebase::*, utils::parse_bot_comment_from_text, - vanity_service, CommentCommand, MergeAllowedOutcome, MergeCancelOutcome, - MergeCommentCommand, Result, Status, WEBHOOK_PARSING_ERROR_TEMPLATE, + vanity_service, CommentCommand, MergeCancelOutcome, MergeCommentCommand, + Result, Status, WEBHOOK_PARSING_ERROR_TEMPLATE, }; pub struct AppState { @@ -386,7 +386,7 @@ pub async fn get_latest_statuses_state( repo: &str, commit_sha: &str, html_url: &str, -) -> Result { +) -> Result<(Status, HashMap)> { // Set up a loop so that we can recover from failed jobs which might have been // retried let mut did_check_for_retried_jobs = false; @@ -396,8 +396,7 @@ pub async fn get_latest_statuses_state( // Since Github only considers the latest instance of each status, we should // abide by the same rule. Each instance is uniquely identified by "context". - let mut latest_statuses: HashMap = - HashMap::new(); + let mut latest_statuses = HashMap::new(); for s in status.statuses { if s.description .as_ref() @@ -429,13 +428,13 @@ pub async fn get_latest_statuses_state( .all(|(_, state)| *state == StatusState::Success) { log::info!("{} has success status", html_url); - return Ok(Status::Success); + return Ok((Status::Success, latest_statuses)); } else if latest_statuses.values().any(|(_, state)| { *state == StatusState::Error || *state == StatusState::Failure }) { if did_check_for_retried_jobs { log::info!("{} has failed status", html_url); - return Ok(Status::Failure); + return Ok((Status::Failure, latest_statuses)); } else { /* Some jobs might be retried automatically, as pointed out in: @@ -453,7 +452,7 @@ pub async fn get_latest_statuses_state( } } else { log::info!("{} has pending status", html_url); - return Ok(Status::Pending); + return Ok((Status::Pending, latest_statuses)); } } } @@ -470,10 +469,7 @@ pub async fn get_latest_checks_state( // Since Github only considers the latest instance of each check, we should abide by the same // rule. Each instance is uniquely identified by "name". - let mut latest_checks: HashMap< - String, - (i64, CheckRunStatus, Option), - > = HashMap::new(); + let mut latest_checks = HashMap::new(); for c in checks.check_runs { if latest_checks .get(&c.name) @@ -483,7 +479,7 @@ pub async fn get_latest_checks_state( latest_checks.insert(c.name, (c.id, c.status, c.conclusion)); } } - log::info!("{} latest_checks,: {:?}", html_url, latest_checks); + log::info!("{} latest_checks: {:?}", html_url, latest_checks); Ok( if latest_checks.values().all(|(_, _, conclusion)| { @@ -542,7 +538,7 @@ pub async fn checks_and_status(state: &AppState, sha: &str) -> Result<()> { return Ok(()); } - check_merge_is_allowed(state, &pr, &mr.requested_by, None, &[]).await?; + check_merge_is_allowed(state, &pr, &mr.requested_by, &[]).await?; if let Some(dependencies) = &mr.dependencies { for dependency in dependencies { @@ -1066,12 +1062,12 @@ async fn handle_command( dependencies: None, }; - check_merge_is_allowed(state, pr, requested_by, None, &[]).await?; + check_merge_is_allowed(state, pr, requested_by, &[]).await?; match cmd { MergeCommentCommand::Normal => { if ready_to_merge(github_bot, pr).await? { - match merge(state, pr, requested_by, None).await? { + match merge(state, pr, requested_by).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 { @@ -1103,7 +1099,7 @@ async fn handle_command( } } MergeCommentCommand::Force => { - match merge(state, pr, requested_by, None).await? { + match merge(state, pr, requested_by).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 }) => { @@ -1251,230 +1247,27 @@ async fn handle_comment( (sha, result) } -/// Check if the pull request is mergeable and is able to meet the approval criteria. -/// Errors related to core-devs and substrateteamleads API requests are not propagated as actual -/// errors because the merge might succeed regardless of them, thus it does not make sense to fail -/// this scenario completely if those request fail for some reason. pub async fn check_merge_is_allowed( state: &AppState, pr: &PullRequest, requested_by: &str, - min_approvals_required: Option, companion_reference_trail: &[CompanionReferenceTrailItem], -) -> Result { - let AppState { - github_bot, config, .. - } = state; - - if let Some(min_approvals_required) = &min_approvals_required { - log::info!( - "Attempting to reach minimum number of approvals {}", - min_approvals_required - ); - } else if pr.mergeable.unwrap_or(false) { - log::info!("{} is mergeable", pr.html_url); - } else { - log::info!("{} is not mergeable", pr.html_url); - } - - if !pr.mergeable.unwrap_or(false) && min_approvals_required.is_none() { +) -> Result<()> { + if !pr.mergeable.unwrap_or(false) { return Err(Error::Message { msg: format!("Github API says {} is not mergeable", pr.html_url), }); + } else { + log::info!("{} is mergeable", pr.html_url); } - check_all_companions_are_mergeable( + return check_all_companions_are_mergeable( state, pr, requested_by, companion_reference_trail, ) - .await?; - - let min_approvals = if config.disable_org_check { - 0 - } else { - match min_approvals_required { - Some(min_approvals_required) => min_approvals_required, - None => match pr.base.repo.name.as_str() { - // TODO have this be based on the repository's settings from the API - // (https://github.com/paritytech/parity-processbot/issues/319) - "substrate" => 2, - _ => 1, - }, - } - }; - - // 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 - }) - .unwrap_or(false) - { - 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 { - None - } - }) - .collect::>(); - - let mut errors: Vec = Vec::new(); - - let team_leads = if config.disable_org_check { - vec![] - } else { - github_bot - .team_members(&pr.base.repo.owner.login, &config.team_leads_team) - .await - .unwrap_or_else(|e| { - let msg = format!( - "Error getting {}: `{}`", - &config.team_leads_team, e - ); - log::error!("{}", msg); - errors.push(msg); - vec![] - }) - }; - let team_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 = if config.disable_org_check { - vec![] - } else { - github_bot - .team_members(&pr.base.repo.owner.login, &config.core_devs_team) - .await - .unwrap_or_else(|e| { - let msg = format!( - "Error getting {}: `{}`", - &config.core_devs_team, e - ); - log::error!("{}", msg); - errors.push(msg); - vec![] - }) - }; - let core_dev_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_dev_approvals > team_lead_approvals { - core_dev_approvals - } else { - team_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 core_approved = core_dev_approvals >= min_approvals; - let leads_approved = team_lead_approvals >= 1; - - if core_approved || leads_approved { - log::info!("{} has core or team lead approval.", pr.html_url); - Ok(relevant_approvals_count) - } else { - Err(Error::Approval { - errors, - html_url: (&pr.html_url).to_owned(), - }) - } - }?; - - if relevant_approvals_count >= min_approvals { - return Ok(MergeAllowedOutcome::Allowed); - } - - 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 - { - return Ok(MergeAllowedOutcome::Disallowed(format!( - "It's not possible to meet the minimal approval count of {} in {}", - min_approvals, pr.html_url - ))); - } - - let role = if team_leads - .iter() - .any(|team_lead| team_lead.login == requested_by) - { - "a team lead".to_string() - } else { - return Ok(MergeAllowedOutcome::Disallowed(format!( - "@{} 's approval is not enough to make {} mergeable by processbot's rules", - requested_by, pr.html_url - ))); - }; - - Ok(MergeAllowedOutcome::GrantApprovalForRole(role)) + .await; } pub async fn ready_to_merge( @@ -1489,6 +1282,7 @@ pub async fn ready_to_merge( &pr.html_url, ) .await? + .0 { Status::Success => { match get_latest_checks_state( @@ -1788,13 +1582,10 @@ pub async fn cleanup_pr( Ok(()) } -/// This function might recursively call itself when attempting to solve a recoverable merge error. -#[async_recursion] pub async fn merge( state: &AppState, pr: &PullRequest, requested_by: &str, - created_approval_id: Option, ) -> Result> { if handle_merged_pr(state, pr, requested_by).await? { return Ok(Ok(())); @@ -1802,217 +1593,84 @@ pub async fn merge( let AppState { github_bot, .. } = state; - if let Err(err) = async { - let err = match github_bot - .merge_pull_request( + let err = match github_bot + .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); + // Merge succeeded! Now clean it from the database + if let Err(err) = cleanup_pr( + state, + &pr.head.sha, &pr.base.repo.owner.login, &pr.base.repo.name, pr.number, - &pr.head.sha, + &PullRequestCleanupReason::AfterMerge, ) .await - { - Ok(_) => { - log::info!("{} merged successfully.", pr.html_url); - // Merge succeeded! Now clean it from the database - if let Err(err) = cleanup_pr( - state, - &pr.head.sha, - &pr.base.repo.owner.login, - &pr.base.repo.name, - pr.number, - &PullRequestCleanupReason::AfterMerge - ).await { - log::error!("Failed to cleanup PR on the database after merge: {}", err); - }; - return Ok(()); - } - Err(err) => err, - }; - - let msg = match err { - Error::Response { - ref status, - ref body, - } if *status == StatusCode::METHOD_NOT_ALLOWED => { - match body.get("message") { - Some(msg) => match msg.as_str() { - Some(msg) => msg, - None => { - log::error!("Expected \"message\" of Github API merge failure response to be a string"); - return Err(err); - }, - }, - None => { - log::error!("Expected \"message\" of Github API merge failure response to be available"); - return Err(err); - }, - } - } - _ => return Err(err), - }; - - // Matches the following - // - "Required status check ... is {pending,expected}." - // - "... required status checks have not succeeded: ... {pending,expected}." - let missing_status_matcher = RegexBuilder::new( - r"required\s+status\s+.*(pending|expected)", - ) - .case_insensitive(true) - .build() - .unwrap(); - - if missing_status_matcher - .find(msg) - .is_some() - { - // This problem will be solved automatically when all the required statuses are delivered, thus - // it can be ignored here - log::info!( - "Ignoring merge failure due to pending required status; message: {}", - msg - ); - return Err(Error::MergeFailureWillBeSolvedLater { msg: msg.to_string() }); - } - - // From this point onwards we'll attempt to recover from "Missing N approvals case" - - // If the bot has already approved once, the missing approval can't be fulfilled by going - // further, so exit early. - if created_approval_id.is_some() { - log::info!("Failed to recover from merge error even after granting the bot approval"); - return Err(Error::Message { msg: msg.to_string() }) + { + log::error!( + "Failed to cleanup PR on the database after merge: {}", + err + ); + }; + return Ok(Ok(())); } + Err(err) => err, + }; - let min_approvals_required = { - // Matches the following - // - "At least N approving reviews are required by reviewers with write access." - let insufficient_approval_quota_matcher = - RegexBuilder::new(r"([[:digit:]]+).*approving\s+reviews?\s+(is|are)\s+required") - .case_insensitive(true) - .build() - .unwrap(); - - match insufficient_approval_quota_matcher.captures(msg) { - Some(matches) => matches - .get(1) - .unwrap() - .as_str() - .parse::() - .unwrap(), - None => return Err(Error::Message { msg: msg.to_string() }) - } - }; - - let review = match check_merge_is_allowed( - state, - pr, - requested_by, - Some(min_approvals_required), - &[] - ) - .await { - Ok(MergeAllowedOutcome::Allowed) => None, - Ok(MergeAllowedOutcome::GrantApprovalForRole(role)) => { - if let Err(err) = github_bot - .create_issue_comment( - &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", - requested_by, - role, - ), - ) - .await - { - log::error!("Failed to post comment on bot approval of {} due to {}", pr.html_url, err); - } - - match github_bot.approve_merge_request( - &pr.base.repo.owner.login, - &pr.base.repo.name, - pr.number - ).await { - Ok(review) => Some(review.id), - Err(err) => { - log::error!( - "Failed to create a review for approving the merge request {} due to {:?}", - pr.html_url, - err - ); - return Err(Error::Message { msg: msg.to_string() }); - } + let msg = match err { + Error::Response { + ref status, + ref body, + } if *status == StatusCode::METHOD_NOT_ALLOWED => match body.get("message") { + Some(msg) => match msg.as_str() { + Some(msg) => msg, + None => { + log::error!("Expected \"message\" of Github API merge failure response to be a string"); + return Err(err); } }, - Ok(MergeAllowedOutcome::Disallowed(msg)) => { - return Err(Error::Message { msg }); - }, - Err(err) => { - log::info!("Failed to get requested role for approval of {} due to {}", pr.html_url, err); - return Err(Error::Message { msg: msg.to_string() }); + None => { + log::error!("Expected \"message\" of Github API merge failure response to be available"); + return Err(err); } - }; - - merge( - state, - pr, - requested_by, - review, - ).await??; + }, + _ => return Err(err), + }; - Ok(()) - } - .await - { - if let Some(approval_id) = created_approval_id { - if let Err(clear_err) = github_bot - .clear_merge_request_approval( - &pr.base.repo.owner.login, - &pr.base.repo.name, - pr.number, - approval_id, - ) - .await - { - log::error!( - "Failed to cleanup a bot review in {} due to {}", - pr.html_url, - clear_err - ) - } - } - return Err(err); + // Matches the following + // - "Required status check ... is {pending,expected}." + // - "... required status checks have not succeeded: ... {pending,expected}." + let missing_status_matcher = + RegexBuilder::new(r"required\s+status\s+.*(pending|expected)") + .case_insensitive(true) + .build() + .unwrap(); + + if missing_status_matcher.find(msg).is_some() { + // This problem will be solved automatically when all the required statuses are delivered, thus + // it can be ignored here + log::info!( + "Ignoring merge failure due to pending required status; message: {}", + msg + ); + return Ok(Err(Error::MergeFailureWillBeSolvedLater { + msg: msg.to_string(), + })); } - Ok(Ok(())) -} - -fn get_troubleshoot_msg(state: &AppState) -> String { - let AppState { config, .. } = state; - 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 team members of {} or {}.", - &config.team_leads_team, - &config.core_devs_team, - ) + Err(Error::Message { msg: msg.into() }) } -fn display_errors_along_the_way(errors: Vec) -> String { - format!( - "The following errors **might** have affected the outcome of this attempt:\n{}", - errors.iter().map(|e| format!("- {}", e)).join("\n") - ) -} - -fn format_error(state: &AppState, err: Error) -> String { +fn format_error(_state: &AppState, err: Error) -> String { match err { - Error::Approval { errors, html_url } => format!( - "Approval criteria was not satisfied in {}.\n\n{}\n\n{}", - html_url, - display_errors_along_the_way(errors), - get_troubleshoot_msg(state) - ), Error::Response { ref body, ref status, diff --git a/tests/helpers/setup.rs b/tests/helpers/setup.rs index a2754c7..cc3cc6e 100644 --- a/tests/helpers/setup.rs +++ b/tests/helpers/setup.rs @@ -25,8 +25,6 @@ pub struct CommonSetupOutput { pub repo_full_name: String, pub github_app_id: usize, pub initial_branch: String, - pub core_devs_team: &'static str, - pub team_leads_team: &'static str, } pub fn common_setup() -> CommonSetupOutput { let git_daemon_base_path_tracker = @@ -163,12 +161,6 @@ GcZ0izY/30012ajdHY+/QK5lsMoxTnn0skdS+spLxaS5ZEO4qvPVb8RAoCkWMMal ), ); - let core_devs_team = "core-devs"; - let team_leads_team = "team-leads"; - for team in &[core_devs_team, team_leads_team] { - setup_team(&github_api, &owner.login, team, vec![owner.clone()]); - } - let db_dir = tempfile::tempdir().unwrap(); CommonSetupOutput { @@ -185,27 +177,9 @@ GcZ0izY/30012ajdHY+/QK5lsMoxTnn0skdS+spLxaS5ZEO4qvPVb8RAoCkWMMal repo_full_name, private_key, initial_branch: initial_branch.to_string(), - core_devs_team, - team_leads_team, } } -pub fn setup_team( - github_api: &Server, - org: &str, - team: &str, - users: Vec, -) { - github_api.expect( - Expectation::matching(request::method_path( - "GET", - format!("/orgs/{}/teams/{}/members", org, team), - )) - .times(0..) - .respond_with(json_encoded(users)), - ); -} - pub fn setup_commit(setup: &CommonSetupOutput, sha: &str) { let CommonSetupOutput { owner, @@ -394,19 +368,6 @@ pub fn setup_pull_request( })), ); - github_api.expect( - Expectation::matching(request::method_path( - "GET", - format!("{}/reviews", pr_api_path), - )) - .times(0..) - .respond_with(json_encoded(vec![github::Review { - id: 1, - user: Some(owner.clone()), - state: Some(github::ReviewState::Approved), - }])), - ); - github_api.expect( Expectation::matching(request::method_path( "POST", diff --git a/tests/merge.rs b/tests/merge.rs index c34a71d..fb3da9c 100644 --- a/tests/merge.rs +++ b/tests/merge.rs @@ -26,8 +26,6 @@ async fn simple_merge_succeeds() { github_app_id, repo_name, repo_full_name, - core_devs_team, - team_leads_team, git_daemon_dir, .. } = &common_setup; @@ -91,8 +89,6 @@ async fn simple_merge_succeeds() { github_app_id: *github_app_id, merge_command_delay: 0, companion_status_settle_delay: 0, - core_devs_team: core_devs_team.to_string(), - team_leads_team: team_leads_team.to_string(), github_source_prefix: "https://github.com".into(), github_source_suffix: "".into(), }; diff --git a/tests/snapshots/merge__simple_merge_succeeds.snap b/tests/snapshots/merge__simple_merge_succeeds.snap index 638f95a..cde65ba 100644 --- a/tests/snapshots/merge__simple_merge_succeeds.snap +++ b/tests/snapshots/merge__simple_merge_succeeds.snap @@ -1,16 +1,14 @@ --- source: tests/merge.rs expression: "read_snapshot(log_dir.path().to_path_buf(), &[&pr_head_sha])" - --- INFO [parity_processbot::webhook] Merge(Normal) requested by owner in https://localhost/owner/repo/pull/1 INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 is mergeable -INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 merge requested by a team lead. INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 statuses: CombinedStatus { statuses: [Status { id: 1, context: "does not matter", state: Success, description: Some("does not matter") }] } INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 latest_statuses: {"does not matter": (1, Success)} INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 has success status INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 checks: CheckRuns { check_runs: [CheckRun { id: 1, name: "does not matter", status: Completed, conclusion: Some(Success), head_sha: "{REDACTED}" }] } -INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 latest_checks,: {"does not matter": (1, Completed, Some(Success))} +INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 latest_checks: {"does not matter": (1, Completed, Some(Success))} INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 has successful checks INFO [parity_processbot::webhook] https://localhost/owner/repo/pull/1 merged successfully. INFO [parity_processbot::webhook] Acquiring cleanup_pr's recursion prevention lock