make core_devs and substrateteamleads configurable

.env.example

@@ -24,11 +24,19 @@ WEBHOOK_SECRET=hunter2
 # 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 in the same organization where this bot is installed
+TEAM_LEADS_TEAM=team
+
+# The GitHub team which whose members will be used as core developers
+# Must be in the same organization where this bot is installed
+CORE_DEVS_TEAM=team
+
 
 # --- 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
+# WEBHOOK_PROXY_URL=https://smee.io/parity-processbot
 
 # Disable organization checks such as the substrateteamleads and core-devs checks.
-DISABLE_ORG_CHECK=true
+# DISABLE_ORG_CHECK=true

.gitlab-ci.yml

@@ -128,6 +128,8 @@ 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

helm/templates/processbot.yaml

@@ -79,6 +79,10 @@ spec:
               value: {{ quote .Values.app.WEBHOOK_PORT }}
             - 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: /usr/local/share/db
             - name: WEBHOOK_SECRET

helm/values.yaml

@@ -5,3 +5,5 @@ 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

src/config.rs

@@ -11,6 +11,8 @@ 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,
 }
 
 impl MainConfig {
@@ -52,6 +54,11 @@ 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,
@@ -64,6 +71,8 @@ impl MainConfig {
 			github_api_url,
 			merge_command_delay,
 			companion_status_settle_delay,
+			core_devs_team,
+			team_leads_team,
 		}
 	}
 }

src/constants.rs

@@ -1,7 +1,3 @@
-pub const SUBSTRATE_TEAM_LEADS_GROUP: &str = "substrateteamleads";
-
-pub const CORE_DEVS_GROUP: &str = "core-devs";
-
 // Note: the old database will be *DELETED* when changing this constant, so do not change this
 // without checking the implementation first
 pub const DATABASE_VERSION: &str = "v1.0";

src/github_bot/mod.rs

@@ -1,5 +1,4 @@
-use crate::{config::MainConfig, constants::*, github::*, Result};
-use futures_util::TryFutureExt;
+use crate::{config::MainConfig, github::*, Result};
 
 pub mod issue;
 pub mod project;
@@ -101,16 +100,4 @@ impl GithubBot {
 		});
 		self.client.put(url, body).await
 	}
-
-	pub async fn core_devs(&self, owner: &str) -> Result<Vec<User>> {
-		self.team(owner, CORE_DEVS_GROUP)
-			.and_then(|team| self.team_members(team.id))
-			.await
-	}
-
-	pub async fn substrate_team_leads(&self, owner: &str) -> Result<Vec<User>> {
-		self.team(owner, SUBSTRATE_TEAM_LEADS_GROUP)
-			.and_then(|team| self.team_members(team.id))
-			.await
-	}
 }

src/github_bot/team.rs

@@ -1,4 +1,5 @@
 use crate::{github, Result};
+use futures_util::TryFutureExt;
 
 use super::GithubBot;
 
@@ -20,4 +21,14 @@ impl GithubBot {
 			))
 			.await
 	}
+
+	pub async fn team_members_by_team_name(
+		&self,
+		owner: &str,
+		team: &str,
+	) -> Result<Vec<github::User>> {
+		self.team(owner, team)
+			.and_then(|team| self.team_members(team.id))
+			.await
+	}
 }

src/webhook.rs

@@ -12,7 +12,7 @@ use std::{collections::HashMap, sync::Arc, time::Duration};
 use tokio::{sync::Mutex, time::delay_for};
 
 use crate::{
-	companion::*, config::MainConfig, constants::*, error::*, github::*,
+	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,
@@ -1032,14 +1032,18 @@ pub async fn check_merge_is_allowed(
 	)
 	.await?;
 
-	let min_approvals = 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,
-		},
+	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
@@ -1091,12 +1095,15 @@ pub async fn check_merge_is_allowed(
 		vec![]
 	} else {
 		github_bot
-			.substrate_team_leads(&pr.base.repo.owner.login)
+			.team_members_by_team_name(
+				&pr.base.repo.owner.login,
+				&config.team_leads_team,
+			)
 			.await
 			.unwrap_or_else(|e| {
 				let msg = format!(
 					"Error getting {}: `{}`",
-					SUBSTRATE_TEAM_LEADS_GROUP, e
+					&config.team_leads_team, e
 				);
 				log::error!("{}", msg);
 				errors.push(msg);
@@ -1120,10 +1127,16 @@ pub async fn check_merge_is_allowed(
 		vec![]
 	} else {
 		github_bot
-			.core_devs(&pr.base.repo.owner.login)
+			.team_members_by_team_name(
+				&pr.base.repo.owner.login,
+				&config.core_devs_team,
+			)
 			.await
 			.unwrap_or_else(|e| {
-				let msg = format!("Error getting {}: `{}`", CORE_DEVS_GROUP, e);
+				let msg = format!(
+					"Error getting {}: `{}`",
+					&config.core_devs_team, e
+				);
 				log::error!("{}", msg);
 				errors.push(msg);
 				vec![]
@@ -1164,7 +1177,7 @@ pub async fn check_merge_is_allowed(
 			}
 		}?;
 
-	if relevant_approvals_count > min_approvals {
+	if relevant_approvals_count >= min_approvals {
 		return Ok(MergeAllowedOutcome::Allowed);
 	}
 
@@ -1567,11 +1580,12 @@ pub async fn merge(
 	Ok(Ok(()))
 }
 
-fn get_troubleshoot_msg() -> String {
+fn get_troubleshoot_msg(state: &AppState) -> String {
+	let AppState { config, .. } = state;
 	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
+		&config.team_leads_team,
+		&config.core_devs_team,
 	);
 }
 
@@ -1582,12 +1596,12 @@ fn display_errors_along_the_way(errors: Vec<String>) -> String {
 	)
 }
 
-fn format_error(err: Error) -> String {
+fn format_error(state: &AppState, err: Error) -> String {
 	match err {
 		Error::Approval { errors } => format!(
 			"Approval criteria was not satisfied.\n\n{}\n\n{}",
 			display_errors_along_the_way(errors),
-			get_troubleshoot_msg()
+			get_troubleshoot_msg(state)
 		),
 		Error::Response {
 			ref body,
@@ -1620,7 +1634,7 @@ pub async fn handle_error(
 					Error::MergeFailureWillBeSolvedLater { .. } => (),
 					err => {
 						let msg = {
-							let description = format_error(err);
+							let description = format_error(state, err);
 							let caption = match merge_cancel_outcome {
 								MergeCancelOutcome::ShaNotFound  => "",
 								MergeCancelOutcome::WasCancelled => "Merge cancelled due to error.",

tests/helpers/setup.rs

@@ -26,6 +26,8 @@ pub struct CommonSetupOutput {
 	pub github_app_id: usize,
 	pub next_team_id: i64,
 	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 =
@@ -161,11 +163,11 @@ GcZ0izY/30012ajdHY+/QK5lsMoxTnn0skdS+spLxaS5ZEO4qvPVb8RAoCkWMMal
 				.body(serde_json::to_string(&json!({})).unwrap()),
 		),
 	);
+
+	let core_devs_team = "core-devs";
+	let team_leads_team = "team-leads";
 	let mut next_team_id = 0;
-	for team in &[
-		parity_processbot::constants::CORE_DEVS_GROUP,
-		parity_processbot::constants::SUBSTRATE_TEAM_LEADS_GROUP,
-	] {
+	for team in &[core_devs_team, team_leads_team] {
 		next_team_id += 1;
 		setup_team(
 			&github_api,
@@ -193,6 +195,8 @@ GcZ0izY/30012ajdHY+/QK5lsMoxTnn0skdS+spLxaS5ZEO4qvPVb8RAoCkWMMal
 		private_key,
 		next_team_id,
 		initial_branch: initial_branch.to_string(),
+		core_devs_team,
+		team_leads_team,
 	}
 }
 

tests/merge.rs

@@ -26,6 +26,8 @@ async fn simple_merge_succeeds() {
 		github_app_id,
 		repo_name,
 		repo_full_name,
+		core_devs_team,
+		team_leads_team,
 		..
 	} = &common_setup;
 
@@ -87,6 +89,8 @@ 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(),
 	};
 	let github_bot = GithubBot::new(&config);
 	let db = DB::open_default(&config.db_path).unwrap();