Rework review & approval features for pr-custom-review

.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

.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

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 <a name="criteria-for-merge"></a>
 
-## Approvals <a name="criteria-for-merge-approvals"></a>
-
-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 <a name="criteria-for-merge-checks-and-statuses"></a>
 
 All [Important and above](#commands-relation-to-ci) checks should be green when

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

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:

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 { .. } => {}

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,

src/error.rs

@@ -35,12 +35,6 @@ pub enum Error {
 		actual: String,
 	},
 
-	#[snafu(display("Missing approval."))]
-	Approval {
-		errors: Vec<String>,
-		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<CompanionDetailsWithErrorMessage>,
-	},
 }
 
 impl Error {

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<User>,
-	pub state: Option<ReviewState>,
-}
-
 #[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<Repository>,

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<Review> {
-		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<Review> {
-		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
-	}
 }