Skip to content

Add/remove labels instead of setting a new set of labels #1540

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

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Add/remove labels instead of setting a new set of labels #1540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca4b23d
Add/remove labels instead of setting a new set of labels
Mark-Simulacrum Dec 13, 2021
31494a0
Do not attempt to diff on synchronize
Mark-Simulacrum Dec 13, 2021
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
127 changes: 63 additions & 64 deletions src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use tracing as log;

use async_trait::async_trait;
use chrono::{DateTime, FixedOffset, Utc};
use futures::stream::{FuturesUnordered, StreamExt};
use futures::{future::BoxFuture, FutureExt};
use hyper::header::HeaderValue;
use once_cell::sync::OnceCell;
Expand Down Expand Up @@ -233,18 +232,6 @@ pub struct Label {
pub name: String,
}

impl Label {
async fn exists<'a>(&'a self, repo_api_prefix: &'a str, client: &'a GithubClient) -> bool {
#[allow(clippy::redundant_pattern_matching)]
let url = format!("{}/labels/{}", repo_api_prefix, self.name);
match client.send_req(client.get(&url)).await {
Ok(_) => true,
// XXX: Error handling if the request failed for reasons beyond 'label didn't exist'
Err(_) => false,
}
}
}

#[derive(Debug, serde::Deserialize)]
pub struct PullRequestDetails {
// none for now
Expand Down Expand Up @@ -468,39 +455,68 @@ impl Issue {
Ok(())
}

pub async fn set_labels(
pub async fn remove_label(&self, client: &GithubClient, label: &str) -> anyhow::Result<()> {
log::info!("remove_label from {}: {:?}", self.global_id(), label);
// DELETE /repos/:owner/:repo/issues/:number/labels/{name}
let url = format!(
"{repo_url}/issues/{number}/labels/{name}",
repo_url = self.repository().url(),
number = self.number,
name = label,
);

if !self.labels().iter().any(|l| l.name == label) {
log::info!(
"remove_label from {}: {:?} already not present, skipping",
self.global_id(),
label
);
return Ok(());
}

client
._send_req(client.delete(&url))
.await
.context("failed to delete label")?;

Ok(())
}

pub async fn add_labels(
&self,
client: &GithubClient,
labels: Vec<Label>,
) -> anyhow::Result<()> {
log::info!("set_labels {} to {:?}", self.global_id(), labels);
// PUT /repos/:owner/:repo/issues/:number/labels
log::info!("add_labels: {} +{:?}", self.global_id(), labels);
// POST /repos/:owner/:repo/issues/:number/labels
// repo_url = https://api.github.com/repos/Codertocat/Hello-World
let url = format!(
"{repo_url}/issues/{number}/labels",
repo_url = self.repository().url(),
number = self.number
);

let mut stream = labels
// Don't try to add labels already present on this issue.
let labels = labels
.into_iter()
.map(|label| async { (label.exists(&self.repository().url(), &client).await, label) })
.collect::<FuturesUnordered<_>>();
let mut labels = Vec::new();
while let Some((true, label)) = stream.next().await {
labels.push(label);
.filter(|l| !self.labels().contains(&l))
.map(|l| l.name)
.collect::<Vec<_>>();

log::info!("add_labels: {} filtered to {:?}", self.global_id(), labels);

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

#[derive(serde::Serialize)]
struct LabelsReq {
labels: Vec<String>,
}
client
._send_req(client.put(&url).json(&LabelsReq {
labels: labels.iter().map(|l| l.name.clone()).collect(),
}))
._send_req(client.post(&url).json(&LabelsReq { labels }))
.await
.context("failed to set labels")?;
.context("failed to add labels")?;

Ok(())
}
Expand Down Expand Up @@ -659,6 +675,25 @@ impl Issue {
.context("failed to close issue")?;
Ok(())
}

/// Returns the diff in this event, for Open and Synchronize events for now.
pub async fn diff(&self, client: &GithubClient) -> anyhow::Result<Option<String>> {
let (before, after) = if let (Some(base), Some(head)) = (&self.base, &self.head) {
(base.sha.clone(), head.sha.clone())
} else {
return Ok(None);
};

let mut req = client.get(&format!(
"{}/compare/{}...{}",
self.repository().url(),
before,
after
));
req = req.header("Accept", "application/vnd.github.v3.diff");
let diff = client.send_req(req).await?;
Ok(Some(String::from(String::from_utf8_lossy(&diff))))
}
}

#[derive(serde::Serialize)]
Expand Down Expand Up @@ -762,13 +797,6 @@ pub struct IssuesEvent {
pub repository: Repository,
/// Some if action is IssuesAction::Labeled, for example
pub label: Option<Label>,

// These fields are the sha fields before/after a synchronize operation,
// used to compute the diff between these two commits.
#[serde(default)]
before: Option<String>,
#[serde(default)]
after: Option<String>,
}

#[derive(Debug, serde::Deserialize)]
Expand All @@ -794,37 +822,7 @@ pub fn files_changed(diff: &str) -> Vec<&str> {
files
}

impl IssuesEvent {
/// Returns the diff in this event, for Open and Synchronize events for now.
pub async fn diff_between(&self, client: &GithubClient) -> anyhow::Result<Option<String>> {
let (before, after) = if self.action == IssuesAction::Synchronize {
(
self.before
.clone()
.expect("synchronize has before populated"),
self.after.clone().expect("synchronize has after populated"),
)
} else if self.action == IssuesAction::Opened {
if let (Some(base), Some(head)) = (&self.issue.base, &self.issue.head) {
(base.sha.clone(), head.sha.clone())
} else {
return Ok(None);
}
} else {
return Ok(None);
};

let mut req = client.get(&format!(
"{}/compare/{}...{}",
self.issue.repository().url(),
before,
after
));
req = req.header("Accept", "application/vnd.github.v3.diff");
let diff = client.send_req(req).await?;
Ok(Some(String::from(String::from_utf8_lossy(&diff))))
}
}
impl IssuesEvent {}

#[derive(Debug, serde::Deserialize)]
pub struct IssueSearchResult {
Expand Down Expand Up @@ -1282,6 +1280,7 @@ impl GithubClient {
self.client.post(url).configure(self)
}

#[allow(unused)]
fn put(&self, url: &str) -> RequestBuilder {
log::trace!("put {:?}", url);
self.client.put(url).configure(self)
Expand Down
94 changes: 59 additions & 35 deletions src/handlers/autolabel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,41 +3,55 @@ use crate::{
github::{files_changed, IssuesAction, IssuesEvent, Label},
handlers::Context,
};
use anyhow::Context as _;
use tracing as log;

pub(super) struct AutolabelInput {
labels: Vec<Label>,
add: Vec<Label>,
remove: Vec<Label>,
}

pub(super) async fn parse_input(
ctx: &Context,
event: &IssuesEvent,
config: Option<&AutolabelConfig>,
) -> Result<Option<AutolabelInput>, String> {
// On opening a new PR or sync'ing the branch, look at the diff and try to
// add any appropriate labels.
//
// FIXME: This will re-apply labels after a push that the user had tried to
// remove. Not much can be done about that currently; the before/after on
// synchronize may be straddling a rebase, which will break diff generation.
if let Some(config) = config {
if let Some(diff) = event
.diff_between(&ctx.github)
.await
.map_err(|e| {
log::error!("failed to fetch diff: {:?}", e);
})
.unwrap_or_default()
{
let files = files_changed(&diff);
let mut autolabels = Vec::new();
for (label, cfg) in config.labels.iter() {
if cfg
.trigger_files
.iter()
.any(|f| files.iter().any(|diff_file| diff_file.starts_with(f)))
{
autolabels.push(Label {
name: label.to_owned(),
});
if event.action == IssuesAction::Opened || event.action == IssuesAction::Synchronize {
if let Some(diff) = event
.issue
.diff(&ctx.github)
.await
.map_err(|e| {
log::error!("failed to fetch diff: {:?}", e);
})
.unwrap_or_default()
{
let files = files_changed(&diff);
let mut autolabels = Vec::new();
for (label, cfg) in config.labels.iter() {
if cfg
.trigger_files
.iter()
.any(|f| files.iter().any(|diff_file| diff_file.starts_with(f)))
{
autolabels.push(Label {
name: label.to_owned(),
});
}
}
if !autolabels.is_empty() {
return Ok(Some(AutolabelInput {
add: autolabels,
remove: vec![],
}));
}
}
if !autolabels.is_empty() {
return Ok(Some(AutolabelInput { labels: autolabels }));
}
}
}
Expand Down Expand Up @@ -75,17 +89,21 @@ pub(super) async fn parse_input(
});
}
if !autolabels.is_empty() {
return Ok(Some(AutolabelInput { labels: autolabels }));
return Ok(Some(AutolabelInput {
add: autolabels,
remove: vec![],
}));
}
}
}
if event.action == IssuesAction::Closed {
let labels = event.issue.labels();
if let Some(x) = labels.iter().position(|x| x.name == "I-prioritize") {
let mut labels_excluded = labels.to_vec();
labels_excluded.remove(x);
if labels.iter().any(|x| x.name == "I-prioritize") {
return Ok(Some(AutolabelInput {
labels: labels_excluded,
add: vec![],
remove: vec![Label {
name: "I-prioritize".to_owned(),
}],
}));
}
}
Expand All @@ -98,13 +116,19 @@ pub(super) async fn handle_input(
event: &IssuesEvent,
input: AutolabelInput,
) -> anyhow::Result<()> {
let mut labels = event.issue.labels().to_owned();
for label in input.labels {
// Don't add the label if it's already there
if !labels.contains(&label) {
labels.push(label);
}
event.issue.add_labels(&ctx.github, input.add).await?;
for label in input.remove {
event
.issue
.remove_label(&ctx.github, &label.name)
.await
.with_context(|| {
format!(
"failed to remove {:?} from {:?}",
label,
event.issue.global_id()
)
})?;
}
event.issue.set_labels(&ctx.github, labels).await?;
Ok(())
}
4 changes: 1 addition & 3 deletions src/handlers/major_change.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,7 @@ async fn handle(
label_to_add: String,
new_proposal: bool,
) -> anyhow::Result<()> {
let mut labels = issue.labels().to_owned();
labels.push(Label { name: label_to_add });
let github_req = issue.set_labels(&ctx.github, labels);
let github_req = issue.add_labels(&ctx.github, vec![Label { name: label_to_add }]);

let partial_issue = issue.to_zulip_github_reference();
let zulip_topic = zulip_topic_from_issue(&partial_issue);
Expand Down
Loading