Notify Zulip when issue is closed or reopened #1078

camelid · 2020-12-31T01:12:58Z

This adds optional message_on_close and message_on_reopen fields to
triagebot.toml to notify the Zulip topic when the issue is closed or
reopened.

Inspired by this comment by Ryan Levick.

cc @rylev

This adds optional `message_on_close` and `message_on_reopen` fields to `triagebot.toml` to notify the Zulip topic when the issue is closed or reopened. Inspired by [this comment] by Ryan Levick. [this comment]: https://rust-lang.zulipchat.com/#narrow/stream/245100-t-compiler.2Fwg-prioritization.2Falerts/topic/.2380375.20ICE.3A.20compiler.2Frustc_middle.2Fsrc.2Fty.2Fsubst.2Ers.3A568.3A17.3A.20c.E2.80.A6/near/221205328

src/handlers/notify_zulip.rs

LeSeulArtichaut · 2021-01-24T15:29:18Z

@camelid what's the status of this?

camelid · 2021-01-25T00:02:08Z

I need to fix the bug you found in #1078 (comment) :)

I'll try to get to this sometime soon.

camelid · 2021-02-26T04:09:08Z

I've returned to try working on this again, and I realized what I got stuck on:
If there are multiple labels with a message_on_close, we need to return
multiple NotifyZulipInputs, one for each of those labels. So I tried changing
the return type of parse_input from Result<Option<NotifyZulipInput>, String>
to Result<Vec<NotifyZulipInput>, String>. But that gives these two errors:

error[E0308]: mismatched types
   --> src/handlers.rs:105:20
    |
103 |               match $name::parse_input(ctx, event, config.$name.as_ref()) {
    |                     ----------------------------------------------------- this expression has type `std::result::Result<Vec<NotifyZulipInput>, std::string::String>`
104 |                   Err(err) => errors.push(HandlerError::Message(err)),
105 |                   Ok(Some(input)) => {
    |                      ^^^^^^^^^^^ expected struct `Vec`, found enum `std::option::Option`
...
127 | / issue_handlers! {
128 | |     autolabel,
129 | |     major_change,
130 | |     notify_zulip,
131 | | }
    | |_- in this macro invocation
    |
    = note: expected struct `Vec<NotifyZulipInput>`
                 found enum `std::option::Option<_>`
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0308]: mismatched types
   --> src/handlers.rs:117:20
    |
103 |               match $name::parse_input(ctx, event, config.$name.as_ref()) {
    |                     ----------------------------------------------------- this expression has type `std::result::Result<Vec<NotifyZulipInput>, std::string::String>`
...
117 |                   Ok(None) => {}
    |                      ^^^^ expected struct `Vec`, found enum `std::option::Option`
...
127 | / issue_handlers! {
128 | |     autolabel,
129 | |     major_change,
130 | |     notify_zulip,
131 | | }
    | |_- in this macro invocation
    |
    = note: expected struct `Vec<NotifyZulipInput>`
                 found enum `std::option::Option<_>`
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

I'm not familiar with this code, so I'm not sure what the right solution is. It
seems like solving these errors might require larger architectural changes.

This is my attempted change in case you want to play around with it to try to
find a solution for this problem:

diff --git a/src/handlers/notify_zulip.rs b/src/handlers/notify_zulip.rs
index 92f7b1f..12aa7ca 100644
--- a/src/handlers/notify_zulip.rs
+++ b/src/handlers/notify_zulip.rs
@@ -19,55 +19,63 @@ pub(super) fn parse_input(
     _ctx: &Context,
     event: &IssuesEvent,
     config: Option<&NotifyZulipConfig>,
-) -> Result<Option<NotifyZulipInput>, String> {
-    let applied_label = &event.label.as_ref().expect("label").name;
+) -> Result<Vec<NotifyZulipInput>, String> {
+    let config = match config {
+        Some(config) => config,
+        None => return Ok(vec![]),
+    };
+    //let applied_label = &event.label.as_ref().expect("label").name;
 
-    if let Some(config) = config.and_then(|c| c.labels.get(applied_label)) {
-        for label in &config.required_labels {
-            let pattern = match glob::Pattern::new(label) {
-                Ok(pattern) => pattern,
-                Err(err) => {
-                    log::error!("Invalid glob pattern: {}", err);
-                    continue;
+    Ok(event
+        .issue
+        .labels
+        .iter()
+        .filter_map(|label| config.labels.get(&label.name))
+        .flat_map(|config| {
+            for label in &config.required_labels {
+                let pattern = match glob::Pattern::new(label) {
+                    Ok(pattern) => pattern,
+                    Err(err) => {
+                        log::error!("Invalid glob pattern: {}", err);
+                        continue;
+                    }
+                };
+                if !event
+                    .issue
+                    .labels()
+                    .iter()
+                    .any(|l| pattern.matches(&l.name))
+                {
+                    // Issue misses a required label, ignore this event
+                    return None;
                 }
-            };
-            if !event
-                .issue
-                .labels()
-                .iter()
-                .any(|l| pattern.matches(&l.name))
-            {
-                // Issue misses a required label, ignore this event
-                return Ok(None);
             }
-        }
 
-        match event.action {
-            IssuesAction::Labeled if config.message_on_add.is_some() => {
-                Ok(Some(NotifyZulipInput {
-                    notification_type: NotificationType::Labeled,
-                }))
-            }
-            IssuesAction::Unlabeled if config.message_on_remove.is_some() => {
-                Ok(Some(NotifyZulipInput {
-                    notification_type: NotificationType::Unlabeled,
-                }))
-            }
-            IssuesAction::Closed if config.message_on_close.is_some() => {
-                Ok(Some(NotifyZulipInput {
-                    notification_type: NotificationType::Closed,
-                }))
-            }
-            IssuesAction::Reopened if config.message_on_reopen.is_some() => {
-                Ok(Some(NotifyZulipInput {
-                    notification_type: NotificationType::Reopened,
-                }))
+            match event.action {
+                IssuesAction::Labeled if config.message_on_add.is_some() => {
+                    Some(NotifyZulipInput {
+                        notification_type: NotificationType::Labeled,
+                    })
+                }
+                IssuesAction::Unlabeled if config.message_on_remove.is_some() => {
+                    Some(NotifyZulipInput {
+                        notification_type: NotificationType::Unlabeled,
+                    })
+                }
+                IssuesAction::Closed if config.message_on_close.is_some() => {
+                    Some(NotifyZulipInput {
+                        notification_type: NotificationType::Closed,
+                    })
+                }
+                IssuesAction::Reopened if config.message_on_reopen.is_some() => {
+                    Some(NotifyZulipInput {
+                        notification_type: NotificationType::Reopened,
+                    })
+                }
+                _ => None,
             }
-            _ => Ok(None),
-        }
-    } else {
-        Ok(None)
-    }
+        })
+        .collect())
 }
 
 pub(super) async fn handle_input<'a>(

camelid · 2021-02-26T04:10:00Z

If you have ideas, let me know! I'm kind of stuck.

LeSeulArtichaut · 2021-02-26T20:15:12Z

Does changing Result<Vec<NotifyZulipInput>> to Result<Vec<Option<NotifyZulipInput>>> (wrapping the Vec in an Option) work? It may look suboptimal but I think waiting a little bit of memory is not so bad, we're not in rustc here 😅

camelid · 2021-02-26T21:42:02Z

Does changing Result<Vec<NotifyZulipInput>> to Result<Vec<Option<NotifyZulipInput>>> (wrapping the Vec in an Option) work? It may look suboptimal but I think waiting a little bit of memory is not so bad, we're not in rustc here 😅

Based on your text, I assume you mean Result<Option<Vec<NotifyZulipInput>>> (not Result<Vec<Option<NotifyZulipInput>>>), and yes I did try that:

error[E0308]: mismatched types
   --> src/handlers.rs:107:65
    |
107 |                           $name::handle_input(ctx, config, event, input).await.unwrap_or_else(|err| errors.push(HandlerError::Other(err)));
    |                                                                   ^^^^^ expected struct `NotifyZulipInput`, found struct `Vec`
...
127 | / issue_handlers! {
128 | |     autolabel,
129 | |     major_change,
130 | |     notify_zulip,
131 | | }
    | |_- in this macro invocation
    |
    = note: expected struct `NotifyZulipInput`
               found struct `Vec<NotifyZulipInput>`
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

To solve this I might have to make every handler return a Vec, which might be feasible, but I was hoping for a solution that wouldn't require changing other things :)

LeSeulArtichaut · 2021-02-27T07:08:34Z

Did you change the signature of handle_input in handlers/notify_zulip.rs? It should take a Vec<NotifyZulipInput>

camelid · 2021-02-28T00:07:03Z

Why didn't I think of that? 🤦

Thanks :)

camelid · 2021-02-28T00:07:59Z

Unfortunately I had to do a bunch of refactoring and there don't seem to be any tests. Though as an extra benefit, the code is hopefully clearer :)

camelid

I had to make this refactoring because it turns out there's a fundamental difference between label/unlabel events and close/reopen events:

In the case of label changes, you either notify based on the label's triagebot.toml config, or you don't notify because there's no action specified in the config.

On the other hand, for close/reopen events, you have to notify for every label on the issue that has a message_on_close/message_on_reopen action in its config. E.g., if we decided to add an I-nominated Zulip notification, then an issue with I-nominated and I-prioritize would need to notify both the I-nominated stream and the I-prioritize stream upon close or reopen.

So, I split out two functions: one for label changes, and one for close/reopen. This seemed like the best way to separate their logic since they have unique logic. I also split out a helper function to check if the issue has all of the required_labels so that it isn't duplicated between the label change and close/reopen functions.

src/handlers/notify_zulip.rs

camelid · 2021-02-28T00:10:37Z

src/handlers/notify_zulip.rs

-                {
-                    // Issue misses a required label, ignore this event
-                    return Ok(None);
+) -> Result<Option<Vec<NotifyZulipInput>>, String> {


Unfortunately there's now two ways to represent a lack of notifications: None and Some(vec![]). However, I don't think there's a way to get rid of that without changing a lot of other code.

The only difference between None and Some(vec![]) in this case is that Some(vec![]) will spawn an additional Future that does nothing. I think we can always improve/cleanup later.

camelid · 2021-02-28T00:12:33Z

src/handlers/notify_zulip.rs

+        IssuesAction::Labeled if config.message_on_add.is_some() => Some(NotifyZulipInput {
+            notification_type: NotificationType::Labeled,
+            label,
+        }),
+        IssuesAction::Unlabeled if config.message_on_remove.is_some() => Some(NotifyZulipInput {
+            notification_type: NotificationType::Unlabeled,
+            label,
+        }),


This previously looked like:

if event.action == IssuesAction::Labeled && config.message_on_add.is_some() { return Ok(Some(NotifyZulipInput { notification_type: NotificationType::Labeled, })); } else if config.message_on_remove.is_some() { return Ok(Some(NotifyZulipInput { notification_type: NotificationType::Unlabeled, })); }

(notice the catch-all else if)

The new version should hopefully be clearer.

camelid · 2021-02-28T00:13:42Z

src/handlers/notify_zulip.rs

 pub(super) async fn handle_input<'a>(
    ctx: &Context,
    config: &NotifyZulipConfig,
    event: &IssuesEvent,
-    input: NotifyZulipInput,
+    inputs: Vec<NotifyZulipInput>,
 ) -> anyhow::Result<()> {


I recommend hiding whitespace changes when reviewing this function's diff.

LeSeulArtichaut

After a relatively quick review this looks good to me, left two potential nits.

@Mark-Simulacrum will you have time to review this?

LeSeulArtichaut · 2021-03-03T14:52:52Z

src/handlers/notify_zulip.rs

+        .cloned()
+        .filter_map(|label| {
+            global_config
+                .labels
+                .get(&label.name)
+                .map(|config| (label, config))
+        })
+        .flat_map(|(label, config)| {
+            if !has_all_required_labels(&event.issue, config) {
+                // Issue misses a required label, ignore this event
+                return None;
+            }
+
+            match event.action {
+                IssuesAction::Closed if config.message_on_close.is_some() => {
+                    Some(NotifyZulipInput {
+                        notification_type: NotificationType::Closed,
+                        label,
+                    })
+                }
+                IssuesAction::Reopened if config.message_on_reopen.is_some() => {
+                    Some(NotifyZulipInput {
+                        notification_type: NotificationType::Reopened,
+                        label,
+                    })
                }


Nit: I think we could only clone the labels we're actually interested in?

Suggested change

.cloned()

.filter_map(|label| {

global_config

.labels

.get(&label.name)

.map(|config| (label, config))

})

.flat_map(|(label, config)| {

if !has_all_required_labels(&event.issue, config) {

// Issue misses a required label, ignore this event

return None;

}

match event.action {

IssuesAction::Closed if config.message_on_close.is_some() => {

Some(NotifyZulipInput {

notification_type: NotificationType::Closed,

label,

})

}

IssuesAction::Reopened if config.message_on_reopen.is_some() => {

Some(NotifyZulipInput {

notification_type: NotificationType::Reopened,

label,

})

}

.filter_map(|label| {

global_config

.labels

.get(&label.name)

.map(|config| (label, config))

})

.flat_map(|(label, config)| {

if !has_all_required_labels(&event.issue, config) {

// Issue misses a required label, ignore this event

return None;

}

match event.action {

IssuesAction::Closed if config.message_on_close.is_some() => {

Some(NotifyZulipInput {

notification_type: NotificationType::Closed,

label: label.clone(),

})

}

IssuesAction::Reopened if config.message_on_reopen.is_some() => {

Some(NotifyZulipInput {

notification_type: NotificationType::Reopened,

label: label.clone(),

})

}

src/handlers/notify_zulip.rs

LeSeulArtichaut · 2021-03-03T15:13:27Z

Unfortunately I had to do a bunch of refactoring and there don't seem to be any tests. Though as an extra benefit, the code is hopefully clearer :)

Deploying is the test :D

Mark-Simulacrum · 2021-03-03T15:32:58Z

I am happy to review, but it'll likely be this weekend - not sure if I'll have time before then. It's in my todo list :)

Co-authored-by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>

camelid · 2021-06-12T22:14:02Z

@Mark-Simulacrum could you take a look at this sometime soon? Thanks!

By the way, this might help with reviewing: #1078 (comment)

spastorino · 2021-08-26T16:19:09Z

Was wondering, what's the status of this? is this still needed? should we merge it?.
In case we are positive about my questions :), I guess, if @Mark-Simulacrum doesn't have time to review it, I could do it.

camelid · 2021-08-26T16:23:18Z

Yeah, it just needs a review :)

camelid · 2021-08-26T20:53:14Z

Thanks!

…storino Notify when an `I-prioritize` issue is closed or reopened Companion PR to rust-lang/triagebot#1078, blocked on that PR. r? `@spastorino` cc `@rust-lang/wg-prioritization`

…storino Notify when an `I-prioritize` issue is closed or reopened Companion PR to rust-lang/triagebot#1078, blocked on that PR. r? ``@spastorino`` cc ``@rust-lang/wg-prioritization``

…storino Notify when an `I-prioritize` issue is closed or reopened Companion PR to rust-lang/triagebot#1078, blocked on that PR. r? ```@spastorino``` cc ```@rust-lang/wg-prioritization```

…storino Notify when an `I-prioritize` issue is closed or reopened Companion PR to rust-lang/triagebot#1078, blocked on that PR. r? ``@spastorino`` cc ``@rust-lang/wg-prioritization``

LeSeulArtichaut mentioned this pull request Dec 31, 2020

Notify when an I-prioritize issue is closed or reopened rust-lang/rust#80543

Merged

LeSeulArtichaut reviewed Dec 31, 2020

View reviewed changes

src/handlers/notify_zulip.rs Outdated Show resolved Hide resolved

Refactor to get closes/reopens to work properly

091df37

camelid commented Feb 28, 2021

View reviewed changes

camelid mentioned this pull request Mar 2, 2021

meta: Notify Zulip for rustdoc nominated issues rust-lang/rust#82689

Merged

LeSeulArtichaut approved these changes Mar 3, 2021

View reviewed changes

Use indexing instead of get and unwrap

e3c3985

Co-authored-by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>

camelid force-pushed the close-msg branch from 70fb692 to e3c3985 Compare March 14, 2021 22:17

spastorino merged commit 201b5d9 into rust-lang:master Aug 26, 2021

camelid deleted the close-msg branch August 26, 2021 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notify Zulip when issue is closed or reopened #1078

Notify Zulip when issue is closed or reopened #1078

camelid commented Dec 31, 2020 •

edited

Loading

LeSeulArtichaut commented Jan 24, 2021

camelid commented Jan 25, 2021

camelid commented Feb 26, 2021

camelid commented Feb 26, 2021

LeSeulArtichaut commented Feb 26, 2021

camelid commented Feb 26, 2021 •

edited

Loading

LeSeulArtichaut commented Feb 27, 2021

camelid commented Feb 28, 2021

camelid commented Feb 28, 2021

camelid left a comment

camelid Feb 28, 2021

LeSeulArtichaut Mar 3, 2021

camelid Feb 28, 2021

camelid Feb 28, 2021

LeSeulArtichaut left a comment •

edited

Loading

LeSeulArtichaut Mar 3, 2021

LeSeulArtichaut commented Mar 3, 2021

Mark-Simulacrum commented Mar 3, 2021

camelid commented Jun 12, 2021

spastorino commented Aug 26, 2021

camelid commented Aug 26, 2021

camelid commented Aug 26, 2021

Notify Zulip when issue is closed or reopened #1078

Notify Zulip when issue is closed or reopened #1078

Conversation

camelid commented Dec 31, 2020 • edited Loading

LeSeulArtichaut commented Jan 24, 2021

camelid commented Jan 25, 2021

camelid commented Feb 26, 2021

camelid commented Feb 26, 2021

LeSeulArtichaut commented Feb 26, 2021

camelid commented Feb 26, 2021 • edited Loading

LeSeulArtichaut commented Feb 27, 2021

camelid commented Feb 28, 2021

camelid commented Feb 28, 2021

camelid left a comment

Choose a reason for hiding this comment

camelid Feb 28, 2021

Choose a reason for hiding this comment

LeSeulArtichaut Mar 3, 2021

Choose a reason for hiding this comment

camelid Feb 28, 2021

Choose a reason for hiding this comment

camelid Feb 28, 2021

Choose a reason for hiding this comment

LeSeulArtichaut left a comment • edited Loading

Choose a reason for hiding this comment

LeSeulArtichaut Mar 3, 2021

Choose a reason for hiding this comment

LeSeulArtichaut commented Mar 3, 2021

Mark-Simulacrum commented Mar 3, 2021

camelid commented Jun 12, 2021

spastorino commented Aug 26, 2021

camelid commented Aug 26, 2021

camelid commented Aug 26, 2021

camelid commented Dec 31, 2020 •

edited

Loading

camelid commented Feb 26, 2021 •

edited

Loading

LeSeulArtichaut left a comment •

edited

Loading