From 596f0c5f60cd0b87175994393325b14f9615334e Mon Sep 17 00:00:00 2001 From: Urgau Date: Fri, 7 Mar 2025 23:31:19 +0100 Subject: [PATCH 1/4] Add no GitHub mentions in commits handler Move `no_mentions` handler to being a sub-handler of `check_commits` --- src/config.rs | 12 ++++- src/handlers/check_commits.rs | 6 +++ src/handlers/check_commits/no_mentions.rs | 56 +++++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 src/handlers/check_commits/no_mentions.rs diff --git a/src/config.rs b/src/config.rs index 7fbb16703..ddf431fe6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -47,6 +47,7 @@ pub(crate) struct Config { pub(crate) bot_pull_requests: Option, pub(crate) rendered_link: Option, pub(crate) canonicalize_issue_links: Option, + pub(crate) no_mentions: Option, } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] @@ -420,6 +421,11 @@ pub(crate) struct RenderedLinkConfig { #[serde(deny_unknown_fields)] pub(crate) struct CanonicalizeIssueLinksConfig {} +#[derive(PartialEq, Eq, Debug, serde::Deserialize)] +#[serde(rename_all = "kebab-case")] +#[serde(deny_unknown_fields)] +pub(crate) struct NoMentionsConfig {} + fn get_cached_config(repo: &str) -> Option, ConfigurationError>> { let cache = CONFIG_CACHE.read().unwrap(); cache.get(repo).and_then(|(config, fetch_time)| { @@ -545,6 +551,8 @@ mod tests { [rendered-link] trigger-files = ["posts/"] + + [no-mentions] "#; let config = toml::from_str::(&config).unwrap(); let mut ping_teams = HashMap::new(); @@ -608,6 +616,7 @@ mod tests { trigger_files: vec!["posts/".to_string()] }), canonicalize_issue_links: Some(CanonicalizeIssueLinksConfig {}), + no_mentions: Some(NoMentionsConfig {}), } ); } @@ -671,7 +680,8 @@ mod tests { merge_conflicts: None, bot_pull_requests: None, rendered_link: None, - canonicalize_issue_links: None + canonicalize_issue_links: None, + no_mentions: None, } ); } diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index 719214626..14fd9823e 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -8,6 +8,7 @@ use crate::{ }; mod modified_submodule; +mod no_mentions; mod non_default_branch; /// Key for the state in the database @@ -41,6 +42,7 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any event.issue.number ) }; + let commits = event.issue.commits(&ctx.github).await?; let mut warnings = Vec::new(); @@ -58,6 +60,10 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any warnings.extend(modified_submodule::modifies_submodule(diff)); } + if let Some(no_mentions) = &config.no_mentions { + warnings.extend(no_mentions::mentions_in_commits(no_mentions, &commits)); + } + handle_warnings(ctx, event, warnings).await } diff --git a/src/handlers/check_commits/no_mentions.rs b/src/handlers/check_commits/no_mentions.rs new file mode 100644 index 000000000..cf17272e0 --- /dev/null +++ b/src/handlers/check_commits/no_mentions.rs @@ -0,0 +1,56 @@ +//! Purpose: When opening a PR, or pushing new changes, check for github mentions +//! in commits and notify the user of our no-mentions in commits policy. + +use std::fmt::Write; + +use crate::{config::NoMentionsConfig, github::GithubCommit}; + +pub(super) fn mentions_in_commits( + _conf: &NoMentionsConfig, + commits: &[GithubCommit], +) -> Option { + let mut mentions_commits = Vec::new(); + + for commit in commits { + if !parser::get_mentions(&commit.commit.message).is_empty() { + mentions_commits.push(&*commit.sha); + } + } + + if mentions_commits.is_empty() { + None + } else { + Some(mentions_in_commits_warn(mentions_commits)) + } +} + +fn mentions_in_commits_warn(commits: Vec<&str>) -> String { + let mut warning = format!("There are username mentions (such as `@user`) in the commit messages of the following commits.\n *Please remove the mentions to avoid spamming these users.*\n"); + + for commit in commits { + let _ = writeln!(warning, " - {commit}"); + } + + warning +} + +#[test] +fn test_warning_printing() { + let commits_to_warn = vec![ + "4d6ze57403udfrzefrfe6574", + "f54efz57405u46z6ef465z4f6ze57", + "404u57403uzf5fe5f4f5e57405u4zf", + ]; + + let msg = mentions_in_commits_warn(commits_to_warn); + + assert_eq!( + msg, + r#"There are username mentions (such as `@user`) in the commit messages of the following commits. + *Please remove the mentions to avoid spamming these users.* + - 4d6ze57403udfrzefrfe6574 + - f54efz57405u46z6ef465z4f6ze57 + - 404u57403uzf5fe5f4f5e57405u4zf +"# + ); +} From e1ede9dbffe9f0eb0bd599d3a48ea15eb797c18f Mon Sep 17 00:00:00 2001 From: Urgau Date: Tue, 8 Apr 2025 18:43:02 +0200 Subject: [PATCH 2/4] Render submodule warning less ambiguous with the no mention warning --- src/handlers/check_commits/modified_submodule.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handlers/check_commits/modified_submodule.rs b/src/handlers/check_commits/modified_submodule.rs index f0e63b5af..dcd3d7867 100644 --- a/src/handlers/check_commits/modified_submodule.rs +++ b/src/handlers/check_commits/modified_submodule.rs @@ -1,6 +1,6 @@ use crate::github::FileDiff; -const SUBMODULE_WARNING_MSG: &str = "These commits modify **submodules**."; +const SUBMODULE_WARNING_MSG: &str = "Some commits in this PR modify **submodules**."; /// Returns a message if the PR modifies a git submodule. pub(super) fn modifies_submodule(diff: &[FileDiff]) -> Option { From 724f0c7403aab6a3b89afd74d7112bd4939411c1 Mon Sep 17 00:00:00 2001 From: Urgau Date: Tue, 8 Apr 2025 19:10:11 +0200 Subject: [PATCH 3/4] Add end-to-end test for `mentions_in_commits` method handler --- src/handlers/check_commits/no_mentions.rs | 48 +++++++++++++++++------ 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/src/handlers/check_commits/no_mentions.rs b/src/handlers/check_commits/no_mentions.rs index cf17272e0..cefa9d464 100644 --- a/src/handlers/check_commits/no_mentions.rs +++ b/src/handlers/check_commits/no_mentions.rs @@ -35,22 +35,44 @@ fn mentions_in_commits_warn(commits: Vec<&str>) -> String { } #[test] -fn test_warning_printing() { - let commits_to_warn = vec![ - "4d6ze57403udfrzefrfe6574", - "f54efz57405u46z6ef465z4f6ze57", - "404u57403uzf5fe5f4f5e57405u4zf", - ]; +fn test_mentions_in_commits() { + fn dummy_commit_from_body(sha: &str, body: &str) -> GithubCommit { + use chrono::{DateTime, FixedOffset}; - let msg = mentions_in_commits_warn(commits_to_warn); + GithubCommit { + sha: sha.to_string(), + commit: crate::github::GithubCommitCommitField { + author: crate::github::GitUser { + date: DateTime::::MIN_UTC.into(), + }, + message: body.to_string(), + tree: crate::github::GitCommitTree { + sha: "60ff73dfdd81aa1e6737eb3dacdfd4a141f6e14d".to_string(), + }, + }, + parents: vec![], + } + } + + let mut commits = vec![dummy_commit_from_body( + "d1992a392617dfb10518c3e56446b6c9efae38b0", + "This is simple without mentions!", + )]; + + assert_eq!(mentions_in_commits(&NoMentionsConfig {}, &commits), None); + + commits.push(dummy_commit_from_body( + "d7daa17bc97df9377640b0d33cbd0bbeed703c3a", + "This is a body with a @mention!", + )); assert_eq!( - msg, - r#"There are username mentions (such as `@user`) in the commit messages of the following commits. + mentions_in_commits(&NoMentionsConfig {}, &commits), + Some( + r#"There are username mentions (such as `@user`) in the commit messages of the following commits. *Please remove the mentions to avoid spamming these users.* - - 4d6ze57403udfrzefrfe6574 - - f54efz57405u46z6ef465z4f6ze57 - - 404u57403uzf5fe5f4f5e57405u4zf -"# + - d7daa17bc97df9377640b0d33cbd0bbeed703c3a +"#.to_string() + ) ); } From 88dff5484f3060a92c3216db7f3dc1e55099bb99 Mon Sep 17 00:00:00 2001 From: Urgau Date: Wed, 9 Apr 2025 18:39:23 +0200 Subject: [PATCH 4/4] Fix `check_commits` previous state warnings recording --- src/handlers/check_commits.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index 14fd9823e..a13e150ce 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -94,12 +94,7 @@ async fn handle_warnings( .await?; } - // Format the warnings for user consumption on Github - let warnings: Vec<_> = warnings - .iter() - .map(|warning| format!("* {warning}")) - .collect(); - let warning = format!(":warning: **Warning** :warning:\n\n{}", warnings.join("\n")); + let warning = warning_from_warnings(&warnings); let comment = event.issue.post_comment(&ctx.github, &warning).await?; // Save new state in the database @@ -126,3 +121,12 @@ async fn handle_warnings( Ok(()) } + +// Format the warnings for user consumption on Github +fn warning_from_warnings(warnings: &[String]) -> String { + let warnings: Vec<_> = warnings + .iter() + .map(|warning| format!("* {warning}")) + .collect(); + format!(":warning: **Warning** :warning:\n\n{}", warnings.join("\n")) +}