Skip to content

Auto-nominate for backport a pull request fixing a regression #2092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ pub(crate) struct Config {
pub(crate) issue_links: Option<IssueLinksConfig>,
pub(crate) no_mentions: Option<NoMentionsConfig>,
pub(crate) behind_upstream: Option<BehindUpstreamConfig>,
pub(crate) backport: Option<BackportTeamConfig>,
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
Expand Down Expand Up @@ -522,6 +523,24 @@ fn default_true() -> bool {
true
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
pub(crate) struct BackportTeamConfig {
// Config identifier -> labels
#[serde(flatten)]
pub(crate) configs: HashMap<String, BackportConfig>,
}

#[derive(Default, PartialEq, Eq, Debug, serde::Deserialize)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost all of our fields use kebab-case, let's use that here as well.

Suggested change
#[derive(Default, PartialEq, Eq, Debug, serde::Deserialize)]
#[derive(Default, PartialEq, Eq, Debug, serde::Deserialize)]
#[serde(rename_all = "kebab-case")]

#[serde(deny_unknown_fields)]
pub(crate) struct BackportConfig {
/// Prerequisite label(s) (one of them) to trigger this handler for a specific team
pub(crate) team_labels: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the name team_labels quite misleading, it's not restricted to "team" labels, it can be any labels. What about required_pr_labels? That makes it clear that it's a list of required labels on the pull request.

Suggested change
pub(crate) team_labels: Vec<String>,
pub(crate) required_pr_labels: Vec<String>,

/// Prerequisite label for an issue to qualify as regression
pub(crate) needs_label: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar thing here, "needs_labels" doesn't indicate at all that it's for the issue.

Suggested change
pub(crate) needs_label: String,
pub(crate) required_issue_labels: String,

/// Labels to be added to a pull request closing the regression
pub(crate) add_labels: Vec<String>,
}

fn get_cached_config(repo: &str) -> Option<Result<Arc<Config>, ConfigurationError>> {
let cache = CONFIG_CACHE.read().unwrap();
cache.get(repo).and_then(|(config, fetch_time)| {
Expand Down Expand Up @@ -727,6 +746,7 @@ mod tests {
concern: Some(ConcernConfig {
labels: vec!["has-concerns".to_string()],
}),
backport: None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you populate the toml above (this test data) or the one below and add the expected de-serialized output here. Mainly for sanity check.

}
);
}
Expand Down Expand Up @@ -812,6 +832,7 @@ mod tests {
behind_upstream: Some(BehindUpstreamConfig {
days_threshold: Some(7),
}),
backport: None
}
);
}
Expand Down
2 changes: 2 additions & 0 deletions src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ impl fmt::Display for HandlerError {

mod assign;
mod autolabel;
mod backport;
mod bot_pull_requests;
mod check_commits;
mod close;
Expand Down Expand Up @@ -225,6 +226,7 @@ macro_rules! issue_handlers {
issue_handlers! {
assign,
autolabel,
backport,
issue_links,
major_change,
mentions,
Expand Down
227 changes: 227 additions & 0 deletions src/handlers/backport.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,227 @@
use std::collections::HashMap;
use std::sync::LazyLock;

use crate::config::BackportTeamConfig;
use crate::github::{IssuesAction, IssuesEvent, Label};
use crate::handlers::Context;
use regex::Regex;
use tracing as log;

// see https://docs.github.com/en/issues/tracking-your-work-with-issues/creating-issues/linking-a-pull-request-to-an-issue
static CLOSES_ISSUE_REGEXP: LazyLock<Regex> =
LazyLock::new(|| Regex::new("(?i)(close[sd]*|fix([e]*[sd]*)?|resolve[sd]*) #(\\d+)").unwrap());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't take into account canonicalized issue links, ie rust-lang/rust#123 which are norm now a days on rust-lang/rust. It also doesn't take into account the s pac es. It's also missing the : in closes: #123.

I took the liberty of "fixing" your regex.

Suggested change
LazyLock::new(|| Regex::new("(?i)(close[sd]*|fix([e]*[sd]*)?|resolve[sd]*) #(\\d+)").unwrap());
LazyLock::new(|| Regex::new("(?i)(?P<action>close[sd]*|fix([e]*[sd]*)?|resolve[sd]*)(?P<spaces>:? +)(?<org_repo>[a-zA-Z0-9_-]*\/[a-zA-Z0-9_-]*)?(?P<issue>#[0-9]+)").unwrap());

Note that I named the capture groups so ease of read, can be removed if you don't like them.

Comment on lines +11 to +12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen contributor (probably my-self included using) Fixes (after beta-backport) #123). How do we want to take into account that extra marker?


const BACKPORT_LABELS: [&str; 4] = [
"beta-nominated",
"beta-accepted",
"stable-nominated",
"stable-accepted",
];

#[derive(Default)]
pub(crate) struct BackportInput {
// Issue(s) fixed by this PR
ids: Vec<u64>,
// Labels profile, compound value of (needs_label -> add_labels)
profile_labels: HashMap<String, Vec<String>>,
}

pub(super) async fn parse_input(
_ctx: &Context,
event: &IssuesEvent,
config: Option<&BackportTeamConfig>,
) -> Result<Option<BackportInput>, String> {
let config = match config {
Some(config) => config,
None => return Ok(None),
};

if !matches!(event.action, IssuesAction::Opened) && !event.issue.is_pr() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contributors often add labels after opening the PR, we should probably also listen to "Edited" events, but that becomes expansive.

What about only triggering the logic when the PR is merged? That way we reduce the false-positives and false-negatives.

log::info!(
"Skipping backport event because: IssuesAction = {:?} issue.is_pr() {}",
event.action,
event.issue.is_pr()
);
return Ok(None);
}
let pr = &event.issue;

let pr_labels: Vec<&str> = pr.labels.iter().map(|l| l.name.as_str()).collect();
if contains_any(&pr_labels, &BACKPORT_LABELS) {
log::debug!("PR #{} already has a backport label", pr.number);
return Ok(None);
}

// Retrieve backport config for this PR, based on its team label(s)
// If the PR has no team label matching any [backport.*.team_labels] config, the backport labelling will be skipped
let mut input = BackportInput::default();
let valid_configs: Vec<_> = config
.configs
.iter()
.clone()
.filter(|(_cfg_name, cfg)| {
let team_labels: Vec<&str> = cfg.team_labels.iter().map(|l| l.as_str()).collect();
if !contains_any(&pr_labels, &team_labels) {
log::warn!(
"Skipping backport nomination: PR is missing one required team label: {:?}",
pr_labels
);
return false;
}
input
.profile_labels
.insert(cfg.needs_label.clone(), cfg.add_labels.clone());
true
})
.collect();
if valid_configs.is_empty() {
log::warn!(
"Skipping backport nomination: could not find a suitable backport config. Please ensure the triagebot.toml has a `[backport.*.team_labels]` section matching the team label(s) for PR #{}.",
pr.number
);
return Ok(None);
}

// Check marker text in the opening comment of the PR to retrieve the issue(s) being fixed
for caps in CLOSES_ISSUE_REGEXP.captures_iter(&event.issue.body) {
Comment on lines +85 to +86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, capturing over the issue body without taking into account the markdown is insufficient as people might use Fixes #123 and not expect us to trigger on it.

Preventing that is bit finicky so let's defer that.

let id = caps.get(3).unwrap().as_str();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let id = caps.get(3).unwrap().as_str();
let id = caps.name("issue").unwrap().as_str();

let id = match id.parse::<u64>() {
Ok(id) => id,
Err(err) => {
return Err(format!("Failed to parse issue id `{id}`, error: {err}"));
}
};
input.ids.push(id);
}
log::info!(
"Will handle event action {:?} in backport. Regression IDs found {:?}",
event.action,
input.ids
);

Ok(Some(input))
}

pub(super) async fn handle_input(
ctx: &Context,
_config: &BackportTeamConfig,
event: &IssuesEvent,
input: BackportInput,
) -> anyhow::Result<()> {
let pr = &event.issue;

// Retrieve the issue(s) this pull request closes
let issues = input
.ids
.iter()
.copied()
.map(|id| async move { event.repository.get_issue(&ctx.github, id).await });

// auto-nominate for backport only patches fixing high/critical regressions
// For `P_{medium,low}` regressions, let the author decide
let priority_labels = ["P-high", "P-critical"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this should be configurable instead of being hard-coded.


// Add backport nomination label to the pull request
for issue in issues {
let issue = issue.await.unwrap();
let mut regression_label = String::new();
let issue_labels: Vec<&str> = issue
.labels
.iter()
.map(|l| {
// save regression label for later
if l.name.starts_with("regression-from-") {
regression_label = l.name.clone();
}
l.name.as_str()
})
.collect();

// Check issue for a prerequisite regression label
let regression_labels = [
"regression-from-stable-to-nightly",
"regression-from-stable-to-beta",
"regression-from-stable-to-stable",
];
Comment on lines +141 to +145
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with those, I think it we would be better if they were configurable, but that's not load bearing, we can merge without making them configurable at first.

if regression_label.is_empty() {
return Ok(());
}

// Check issue for a prerequisite priority label
if !contains_any(&issue_labels, &priority_labels) {
return Ok(());
}

// figure out the labels to be added according to the regression label
let add_labels = input.profile_labels.get(&regression_label);
if add_labels.is_none() {
log::warn!(
"Skipping backport nomination: nothing to do for issue #{}. No config found for regression label ({:?})",
issue.number,
regression_labels
);
return Ok(());
}

// Add backport nomination label(s) to PR
let mut new_labels = pr.labels().to_owned();
new_labels.extend(
add_labels
.unwrap()
.iter()
.cloned()
.map(|name| Label { name }),
);
log::debug!(
"PR#{} adding labels for backport {:?}",
pr.number,
new_labels
);
return pr.add_labels(&ctx.github, new_labels).await;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return pr.add_labels(&ctx.github, new_labels).await;
pr.add_labels(&ctx.github, new_labels).await.context("failed to add backport labels to the PR")?;

}

Ok(())
}

fn contains_any(haystack: &[&str], needles: &[&str]) -> bool {
needles.iter().any(|needle| haystack.contains(needle))
}

#[cfg(test)]
mod tests {
use crate::handlers::backport::CLOSES_ISSUE_REGEXP;

#[tokio::test]
async fn backport_match_comment() {
let test_strings = vec![
("close #10", vec![10]),
("closes #10", vec![10]),
("closed #10", vec![10]),
("fix #10", vec![10]),
("fixes #10", vec![10]),
("fixed #10", vec![10]),
("resolve #10", vec![10]),
("resolves #10", vec![10]),
("resolved #10", vec![10]),
(
"Fixes #20, Resolves #21, closed #22, LOL #23",
vec![20, 21, 22],
),
("Resolved #10", vec![10]),
("Fixes #10", vec![10]),
("Closes #10", vec![10]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
("Closes #10", vec![10]),
("Closes #10", vec![10]),
("close #10", vec![10]),
("close rust-lang/rust#10", vec![10]),
("cLose: rust-lang/rust#10", vec![10]),

];
for test_case in test_strings {
let mut ids: Vec<u64> = vec![];
let test_str = test_case.0;
let expected = test_case.1;
for caps in CLOSES_ISSUE_REGEXP.captures_iter(test_str) {
// println!("caps {:?}", caps);
let id = caps.get(3).unwrap().as_str();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let id = caps.get(3).unwrap().as_str();
let id = caps["issue"].as_str();

ids.push(id.parse::<u64>().unwrap());
}
// println!("ids={:?}", ids);
assert_eq!(ids, expected);
}
}
}
Loading