Skip to content

Add alias for compound labels #2172

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.

Sign up for GitHub

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

Add alias for compound labels #2172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion parser/src/command/relabel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub enum LabelDelta {
}

#[derive(Debug, PartialEq, Eq, Clone)]
pub struct Label(String);
pub struct Label(pub String);

#[derive(PartialEq, Eq, Debug)]
pub enum ParseError {
Expand Down Expand Up @@ -103,6 +103,7 @@ fn delta_empty() {
}

impl RelabelCommand {
/// Parse and validate command tokens
pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result<Option<Self>, Error<'a>> {
let mut toks = input.clone();

Expand Down
88 changes: 87 additions & 1 deletion src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::changelogs::ChangelogFormat;
use crate::github::{GithubClient, Repository};
use parser::command::relabel::{Label, LabelDelta, RelabelCommand};
use std::collections::{HashMap, HashSet};
use std::fmt;
use std::sync::{Arc, LazyLock, RwLock};
Expand Down Expand Up @@ -238,10 +239,62 @@ pub(crate) struct MentionsPathConfig {

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
#[serde(rename_all = "kebab-case")]
#[serde(deny_unknown_fields)]
pub(crate) struct RelabelConfig {
#[serde(default)]
pub(crate) allow_unauthenticated: Vec<String>,
// alias identifier -> labels
#[serde(flatten)]
pub(crate) configs: Option<HashMap<String, RelabelRuleConfig>>,
}

impl RelabelConfig {
pub(crate) fn retrieve_command_from_alias(&self, input: RelabelCommand) -> RelabelCommand {
match &self.configs {
Some(configs) => {
dbg!(&configs);
// get only the first token from the command
// extract the "alias" from the RelabelCommand
Comment on lines +255 to +256
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for only considering the first token as an alias?

I would expect aliases to work at any position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see previous comment

if input.0.len() > 0 {
let name = input.0.get(0).unwrap();
let name = name.label().as_str();
// check if this alias matches any RelabelRuleConfig key in our config
// extract the labels and build a new command
if configs.contains_key(name) {
let (_alias, cfg) = configs.get_key_value(name).unwrap();
return cfg.to_command();
}
}
}
None => {
return input;
}
};
input
}
}

#[derive(Default, PartialEq, Eq, Debug, serde::Deserialize)]
#[serde(rename_all = "kebab-case")]
#[serde(deny_unknown_fields)]
pub(crate) struct RelabelRuleConfig {
/// Labels to be added
pub(crate) add_labels: Vec<String>,
/// Labels to be removed
pub(crate) rem_labels: Vec<String>,
}

impl RelabelRuleConfig {
/// Translate a RelabelRuleConfig into a RelabelCommand for GitHub consumption
pub fn to_command(&self) -> RelabelCommand {
let mut deltas = Vec::new();
for l in self.add_labels.iter() {
deltas.push(LabelDelta::Add(Label(l.into())));
}
for l in self.rem_labels.iter() {
deltas.push(LabelDelta::Remove(Label(l.into())));
}
RelabelCommand(deltas)
}
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
Expand Down Expand Up @@ -756,6 +809,7 @@ mod tests {
Config {
relabel: Some(RelabelConfig {
allow_unauthenticated: vec!["C-*".into()],
configs: Some(HashMap::new())
}),
assign: Some(AssignConfig {
warn_non_default_branch: WarnNonDefaultBranchConfig::Simple(false),
Expand Down Expand Up @@ -926,4 +980,36 @@ mod tests {
})
);
}

#[test]
fn relabel_new_config() {
let config = r#"
[relabel]
allow-unauthenticated = ["ABCD-*"]

[relabel.to-stable]
add-labels = ["regression-from-stable-to-stable"]
rem-labels = ["regression-from-stable-to-beta", "regression-from-stable-to-nightly"]
"#;
let config = toml::from_str::<Config>(&config).unwrap();

let mut relabel_configs = HashMap::new();
relabel_configs.insert(
"to-stable".into(),
RelabelRuleConfig {
add_labels: vec!["regression-from-stable-to-stable".to_string()],
rem_labels: vec![
"regression-from-stable-to-beta".to_string(),
"regression-from-stable-to-nightly".to_string(),
],
},
);

let expected_cfg = RelabelConfig {
allow_unauthenticated: vec!["ABCD-*".to_string()],
configs: Some(relabel_configs),
};

assert_eq!(config.relabel, Some(expected_cfg));
}
}
30 changes: 22 additions & 8 deletions src/handlers/relabel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,16 @@ pub(super) async fn handle_command(
event: &Event,
input: RelabelCommand,
) -> anyhow::Result<()> {
let mut results = vec![];
let mut to_rem = vec![];
let mut to_add = vec![];
for delta in &input.0 {

// If the input matches a valid alias, read the [relabel] config.
// if any alias matches, extract the alias config (RelabelRuleConfig) and build a new RelabelCommand.
// Discard anything after the alias.
Copy link
Member

@Urgau Urgau Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should discard anything, at least not silently.

In my opinion we should simply expand all the aliases, and if someone does @rustbot label +label +alias +other-label I would expect both label and other-label to be added + whatever alias expands to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I implemented this patch I thought about allowing both labels and aliases but ultimately discarded the option. My reasoning was that it would open up to contradicting labelling cases:

[config.alias]
add-label = ["P-high"]
rem-label = ["I-prioritize"]

# should I-prioritize stay or not?
@rustbot label alias I-prioritize

I think a user should either send ONE command alias or a set of labels, not both. If a user wants to send many labels, they can configure an alias.

Copy link
Member

@Urgau Urgau Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We already have the issue when someone does @rustbot labels I-prioritize -I-prioritize I-prioritize, currently we adds all the labels first and then removes the other one (resulting in always no label, no matter the position).

IMO the behavior should be made consistent by using the precedence, from left to right, so in either case (your example and mine) I-prioritize would be added to the issue.

But whatever the behavior I don't this issue as being big enough compare to not allow regular labels and labels to be mixed together, which would just looked like an arbitrary limitation. I could very well see my-self wanting to do @rustbot labels +diagnostic-alias +F-my-feature (where diagnostic-alias = +T-compiler +A-diagnostics) and expecting it to work.

We could also error out if the alias and labels conflict, it's less pretty but still an option.

let new_input = config.retrieve_command_from_alias(input);

// Parse input label command, checks permissions, built GitHub commands
for delta in &new_input.0 {
let name = delta.label().as_str();
let err = match check_filter(name, config, is_member(&event.user(), &ctx.team).await) {
Ok(CheckFilterResult::Allow) => None,
Expand All @@ -53,14 +60,12 @@ pub(super) async fn handle_command(
});
}
LabelDelta::Remove(label) => {
results.push((
label,
event.issue().unwrap().remove_label(&ctx.github, &label),
));
to_rem.push(label);
}
Comment on lines -56 to 64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here I changed a bit the logic: instead of storing the futures I just store the labels to be removed and then execute the futures later. Functionally I think it changes nothing but the original version (introduced in b6ccaa0) created a lifetime error after my my patch.

}
}

// Add new labels (if needed)
if let Err(e) = event
.issue()
.unwrap()
Expand All @@ -84,8 +89,14 @@ pub(super) async fn handle_command(
return Err(e);
}

for (label, res) in results {
if let Err(e) = res.await {
// Remove labels (if needed)
for label in to_rem {
if let Err(e) = event
.issue()
.unwrap()
.remove_label(&ctx.github, &label)
.await
{
tracing::error!(
"failed to remove {:?} from issue {}: {:?}",
label,
Expand Down Expand Up @@ -124,6 +135,8 @@ enum CheckFilterResult {
DenyUnknown,
}

/// Check if the team member is allowed to apply labels
/// configured in `allow_unauthenticated`
fn check_filter(
label: &str,
config: &RelabelConfig,
Expand Down Expand Up @@ -220,6 +233,7 @@ mod tests {
($($member:ident { $($label:expr => $res:ident,)* })*) => {
let config = RelabelConfig {
allow_unauthenticated: vec!["T-*".into(), "I-*".into(), "!I-*nominated".into()],
configs: None
};
$($(assert_eq!(
check_filter($label, &config, TeamMembership::$member),
Expand Down
Loading