Try to use less fields for the payloads in order to avoid parsing errors

kubernetes/processbot/templates/processbot.yaml

@@ -99,8 +99,6 @@ spec:
               value: "300"
             - name: MATRIX_HOMESERVER
               value: "https://matrix.parity.io"
-            - name: CORE_SORTING_REPO_NAME
-              value: "core-sorting"
             - name: STATUS_FAILURE_PING
               value: "86400"
             - name: ISSUE_NOT_ADDRESSED_PING
@@ -113,8 +111,6 @@ spec:
               value: "259200"
             - name: NO_PROJECT_AUTHOR_UNKNOWN_CLOSE_PR
               value: "900"
-            - name: PROJECT_CONFIRMATION_TIMEOUT
-              value: "900"
             - name: REVIEW_REQUEST_PING
               value: "86400"
             - name: PRIVATE_REVIEW_REMINDER_PING

src/companion.rs

@@ -236,11 +236,11 @@ async fn update_companion_repository(
 	Ok(updated_sha)
 }
 
-fn companion_parse(body: &str) -> Option<(String, String, String, i64)> {
+fn companion_parse(body: &str) -> Option<IssueDetailsWithRepositoryURL> {
 	companion_parse_long(body).or(companion_parse_short(body))
 }
 
-fn companion_parse_long(body: &str) -> Option<(String, String, String, i64)> {
+fn companion_parse_long(body: &str) -> Option<IssueDetailsWithRepositoryURL> {
 	let re = RegexBuilder::new(COMPANION_LONG_REGEX!())
 		.case_insensitive(true)
 		.build()
@@ -258,7 +258,7 @@ fn companion_parse_long(body: &str) -> Option<(String, String, String, i64)> {
 	Some((html_url, owner, repo, number))
 }
 
-fn companion_parse_short(body: &str) -> Option<(String, String, String, i64)> {
+fn companion_parse_short(body: &str) -> Option<IssueDetailsWithRepositoryURL> {
 	let re = RegexBuilder::new(COMPANION_SHORT_REGEX!())
 		.case_insensitive(true)
 		.build()

src/config.rs

@@ -33,8 +33,6 @@ pull request author or publicly to the default channel if the author's Matrix ha
 
 `NO_PROJECT_AUTHOR_UNKNOWN_CLOSE_PR`: Seconds before closing a pull request opened by an external developer that has no project attached.
 
-`PROJECT_CONFIRMATION_TIMEOUT`: Seconds before reverting an unconfirmed change of project by a non-whitelisted developer (currently unimplemented).
-
 `MIN_REVIEWERS`: Minimum number of reviewers needed before a pull request can be accepted.
 
 `REVIEW_REQUEST_PING`: Seconds between notifications requesting reviews on a pull request, sent publicly to the relevant project room, via Matrix.
@@ -159,8 +157,6 @@ pub struct BotConfig {
 	pub no_project_author_is_core_close_pr: u64,
 	/// seconds before pr gets closed
 	pub no_project_author_unknown_close_pr: u64,
-	/// seconds before unconfirmed change gets reverted
-	pub project_confirmation_timeout: u64,
 	/// seconds between pings
 	pub review_request_ping: u64,
 	/// seconds between pings
@@ -171,8 +167,6 @@ pub struct BotConfig {
 	pub public_review_reminder_delay: u64,
 	/// mininum number of reviewers
 	pub min_reviewers: usize,
-	/// name of repo for issues without a project
-	pub core_sorting_repo_name: String,
 	/// matrix room id for sending app logs
 	pub logs_room_id: String,
 }
@@ -219,13 +213,6 @@ impl BotConfig {
 			.parse::<u64>()
 			.expect("failed parsing NO_PROJECT_AUTHOR_UNKNOWN_CLOSE_PR"),
 
-			project_confirmation_timeout: dotenv::var(
-				"PROJECT_CONFIRMATION_TIMEOUT",
-			)
-			.expect("PROJECT_CONFIRMATION_TIMEOUT")
-			.parse::<u64>()
-			.expect("failed parsing PROJECT_CONFIRMATION_TIMEOUT"),
-
 			review_request_ping: dotenv::var("REVIEW_REQUEST_PING")
 				.expect("REVIEW_REQUEST_PING")
 				.parse::<u64>()
@@ -257,9 +244,6 @@ impl BotConfig {
 				.parse::<usize>()
 				.expect("failed parsing MIN_REVIEWERS"),
 
-			core_sorting_repo_name: dotenv::var("CORE_SORTING_REPO_NAME")
-				.expect("CORE_SORTING_REPO_NAME"),
-
 			logs_room_id: dotenv::var("LOGS_ROOM_ID").expect("LOGS_ROOM_ID"),
 		}
 	}

src/constants.rs

@@ -1,9 +1,11 @@
+// NOTE: If you add a new command, put it in the BOT_COMMANDS array
 pub const AUTO_MERGE_REQUEST: &str = "bot merge";
 pub const AUTO_MERGE_FORCE: &str = "bot merge force";
 pub const AUTO_MERGE_CANCEL: &str = "bot merge cancel";
 pub const REBASE: &str = "bot rebase";
 pub const BURNIN_REQUEST: &str = "bot burnin";
 pub const COMPARE_RELEASE_REQUEST: &str = "bot compare substrate";
+// NOTE: Put all commands here, otherwise the bot will not detect them
 pub const BOT_COMMANDS: [&str; 6] = [
 	AUTO_MERGE_REQUEST,
 	AUTO_MERGE_FORCE,
@@ -13,52 +15,6 @@ pub const BOT_COMMANDS: [&str; 6] = [
 	COMPARE_RELEASE_REQUEST,
 ];
 
-pub const AUTO_MERGE_FAILED: &str = "Cannot merge; please ensure the pull request is mergeable and has approval from the project owner or at least {min_reviewers} core devs.";
-pub const AUTO_MERGE_CHECKS_FAILED: &str = "Checks failed; cannot auto-merge.";
-pub const AUTO_MERGE_CHECKS_ERROR: &str =
-	"Checks returned an error; cannot auto-merge.";
-pub const AUTO_MERGE_INVALIDATED: &str =
-	"Something has changed since auto-merge was requested; cancelling.";
-
-pub const FEATURES_KEY: &str = "features";
-
-pub const PROJECT_NEEDS_BACKLOG: &str =
-	"@{owner}, {project_url} needs a backlog column.";
-
-pub const MISMATCHED_PROCESS_FILE: &str = "Process.toml for repo {repo_url} lists projects that do not exist in the repo, so it will be treated as invalid.";
-
-pub const MALFORMED_PROCESS_FILE: &str = "Process.toml for repo {repo_url} is malformed or missing some fields. Please ensure that every listed project contains an owner and a matrix_room_id.";
-
-pub const WARN_FOR_NO_ISSUE: &str = "@{author}, this will be closed if it does not explicitly mention the issue it addresses.";
-
-pub const CLOSE_FOR_NO_ISSUE: &str = "@{author}, this is being closed because it does not explicitly address an issue.";
-
-pub const WARN_FOR_NO_PROJECT: &str =
-	"@{author}, this will be closed if it is not attached to a project.";
-
-pub const PRIVATE_ISSUE_NEEDS_REASSIGNMENT: &str = "{pr_url} addressing {issue_url} has been opened by {author}. Please reassign the issue to the PR author, or close the pull request.";
-
-pub const PUBLIC_ISSUE_NEEDS_REASSIGNMENT: &str = "@{owner}, {pr_url} addressing {issue_url} has been opened by {author}. Please reassign the issue to the PR author, or close the pull request.";
-
-pub const PROJECT_CONFIRMATION: &str = "{issue_url} has been attached to the project column '{column_name}' in project '{project_name}'. To confirm the change, {owner} or a whitelisted developer should post, \"confirm {issue_id} {column_id}\", to this channel in the next {seconds} seconds.";
-
-pub const ISSUE_REVERT_PROJECT_NOTIFICATION: &str = "The change you made to {issue_url} (attaching a project) has been denied or gone unconfirmed for too long, and so has been reverted. Changes require confirmation from the project owner or a whitelisted developer.";
-
-pub const REQUESTING_REVIEWS_MESSAGE: &str =
-	"@{author}, {pr_url} needs reviewers.";
-
-pub const REQUEST_DELEGATED_REVIEW_MESSAGE: &str = "{1} needs your review in the next 72 hours, as you are the owner or delegated reviewer.";
-
-pub const PRIVATE_REVIEW_REMINDER_MESSAGE: &str = "{1} needs your review.";
-
-pub const PUBLIC_REVIEW_REMINDER_MESSAGE: &str = "@{2}, please review {1}.";
-
-pub const CORE_SORTING_REPO: &str = "core-sorting";
-
-pub const BACKLOG_DEFAULT_NAME: &str = "backlog";
-
-pub const LOCAL_STATE_KEY: &str = "LOCAL_STATE_KEY";
-
 pub const SUBSTRATE_TEAM_LEADS_GROUP: &str = "substrateteamleads";
 
 pub const CORE_DEVS_GROUP: &str = "core-devs";

src/error.rs

@@ -3,6 +3,9 @@ use snafu::Snafu;
 // TODO this really should be struct { repository, owner, number }
 pub type IssueDetails = (String, String, i64);
 
+// TODO this really should be struct { repository_url, repository, owner, number }
+pub type IssueDetailsWithRepositoryURL = (String, String, String, i64);
+
 #[derive(Debug, Snafu)]
 #[snafu(visibility = "pub")]
 pub enum Error {

src/github.rs

@@ -22,7 +22,7 @@ pub struct PullRequest {
 }
 
 impl HasIssueDetails for PullRequest {
-	fn get_issue_details(&self) -> Option<(String, String, i64)> {
+	fn get_issue_details(&self) -> Option<IssueDetails> {
 		if let Some(Repository {
 			owner: Some(User { login, .. }),
 			name,
@@ -77,7 +77,7 @@ pub struct Issue {
 }
 
 impl HasIssueDetails for Issue {
-	fn get_issue_details(&self) -> Option<(String, String, i64)> {
+	fn get_issue_details(&self) -> Option<IssueDetails> {
 		match self {
 			Issue {
 				number,
@@ -208,7 +208,6 @@ pub struct RequestedReviewers {
 
 #[derive(PartialEq, Debug, Clone, Serialize, Deserialize)]
 pub enum UserType {
-	User,
 	Bot,
 	#[serde(other)]
 	Unknown,
@@ -347,20 +346,36 @@ pub struct CheckRun {
 	pub head_sha: String,
 }
 
+#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct WebhookIssueComment {
+	pub number: i64,
+	pub html_url: String,
+	pub repository_url: Option<String>,
+	pub pull_request: Option<IssuePullRequest>,
+}
+
+impl HasIssueDetails for WebhookIssueComment {
+	fn get_issue_details(&self) -> Option<IssueDetails> {
+		None
+	}
+}
+
 #[derive(PartialEq, Deserialize)]
 #[serde(untagged)]
 pub enum Payload {
 	IssueComment {
 		action: IssueCommentAction,
-		issue: Issue,
+		issue: WebhookIssueComment,
 		comment: Comment,
 	},
 	CommitStatus {
+		// FIXME: This payload also has a field `repository` for the repository where the status
+		// originated from which should be used *together* with commit SHA for indexing pull requests.
+		// Currently, because merge requests are indexed purely by their head SHA into the database,
+		// there's no way to disambiguate between two different PRs in two different repositories with
+		// the same head SHA.
 		sha: String,
 		state: StatusState,
-		description: String,
-		target_url: String,
-		repository: Repository,
 	},
 	CheckRun {
 		check_run: CheckRun,
@@ -400,7 +415,7 @@ pub struct DetectUserCommentPullRequest {
 }
 
 impl HasIssueDetails for DetectUserCommentPullRequest {
-	fn get_issue_details(&self) -> Option<(String, String, i64)> {
+	fn get_issue_details(&self) -> Option<IssueDetails> {
 		if let DetectUserCommentPullRequest {
 			action: IssueCommentAction::Created,
 			issue:
@@ -411,51 +426,59 @@ impl HasIssueDetails for DetectUserCommentPullRequest {
 			comment:
 				Some(DetectUserCommentPullRequestComment { body: Some(body) }),
 			repository,
-			sender:
-				Some(User {
-					type_field: Some(UserType::User),
-					..
-				}),
+			..
 		} = self
 		{
-			let body = body.trim();
-
-			if BOT_COMMANDS.iter().find(|cmd| **cmd == body).is_some() {
-				if let Some(DetectUserCommentPullRequestRepository {
-					name: Some(name),
-					owner: Some(User { login, .. }),
+			match self.sender {
+				Some(User {
+					type_field: Some(UserType::Bot),
 					..
-				}) = repository
-				{
-					Some((login.to_owned(), name.to_owned(), *number))
-				} else {
-					None
-				}
-				.or_else(|| {
-					if let Some(DetectUserCommentPullRequestRepository {
-						full_name: Some(full_name),
-						..
-					}) = repository
-					{
-						parse_repository_full_name(full_name)
-							.map(|(owner, name)| (owner, name, *number))
+				}) => None,
+				_ => {
+					let body = body.trim();
+
+					if BOT_COMMANDS.iter().find(|cmd| **cmd == body).is_some() {
+						if let Some(DetectUserCommentPullRequestRepository {
+							name: Some(name),
+							owner: Some(User { login, .. }),
+							..
+						}) = repository
+						{
+							Some((login.to_owned(), name.to_owned(), *number))
+						} else {
+							None
+						}
+						.or_else(|| {
+							if let Some(
+								DetectUserCommentPullRequestRepository {
+									full_name: Some(full_name),
+									..
+								},
+							) = repository
+							{
+								parse_repository_full_name(full_name)
+									.map(|(owner, name)| (owner, name, *number))
+							} else {
+								None
+							}
+						})
+						.or_else(|| {
+							if let DetectUserCommentPullRequestPullRequest {
+								html_url: Some(html_url),
+							} = pr
+							{
+								parse_issue_details_from_pr_html_url(html_url)
+									.map(|(owner, name, _)| {
+										(owner, name, *number)
+									})
+							} else {
+								None
+							}
+						})
 					} else {
 						None
 					}
-				})
-				.or_else(|| {
-					if let DetectUserCommentPullRequestPullRequest {
-						html_url: Some(html_url),
-					} = pr
-					{
-						parse_issue_details_from_pr_html_url(html_url)
-							.map(|(owner, name, _)| (owner, name, *number))
-					} else {
-						None
-					}
-				})
-			} else {
-				None
+				}
 			}
 		} else {
 			None
@@ -465,7 +488,7 @@ impl HasIssueDetails for DetectUserCommentPullRequest {
 
 pub fn parse_issue_details_from_pr_html_url(
 	pr_html_url: &str,
-) -> Option<(String, String, i64)> {
+) -> Option<IssueDetails> {
 	let re = Regex::new(PR_HTML_URL_REGEX!()).unwrap();
 	let matches = re.captures(&pr_html_url)?;
 	let owner = matches.name("owner")?.as_str().to_owned();