-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat(notify-zulip): support notify-zulip.<label>
getting mapped to multiple actions
#1853
base: master
Are you sure you want to change the base?
Conversation
d97c5ea
to
1fb8344
Compare
hey @ismailarilik thank you for this contribution. This was actually something we in T-compiler wanted (was on my TODO list...), see Zulip discussion. One thing I can spot at a first look is that our discussion was favorable to publish these notification in a substream (e.g. cc @rust-lang/rustdoc so they can express their preference either EDIT: created the Zulip channel for the T-compiler backport notifications |
From a rustdoc side, my personal preference would be to keep those notifications in the main stream. It's low trafic enough I don't think it's worth (us) making a new stream for it, although I can imagine this being different for a larger/higher traffic team like compiler. Also relevant context: rust-lang/rust#116957 |
I was thinking that the docs were in that repo but it seems that they are in the Forge. I will update it after this PR is merged. BTW, here is the related page in Forge: https://forge.rust-lang.org/triagebot/zulip-notifications.html |
@rustbot review |
How can I request a review? =) |
Sorry for the noise but do you know how I can test this appropriately? I setup triagebot in my local by following README and tried this implementation; it seems that it is working. But of course, I didn't try to send any Zulip notification to a team. Even if I tried this, I couldn't totally ensure that it would work on production. |
@ismailarilik I'll have a look at your patch later. Maintainers of this repo are listed here (not an obvious place), you can ping any of them. Please allow some time for a review.
To test the Zulip part one needs a Zulip instance. The quickest way is probably to register a new free instance for testing purposes. r? @ehuss |
#[derive(PartialEq, Eq, Debug, serde::Deserialize)] | ||
pub(crate) struct NotifyZulipNameConfig { | ||
#[serde(flatten)] | ||
pub(crate) default: Option<NotifyZulipLabelConfig>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ismailarilik can you put a comment explaining what this field is (is this a label?) and why is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I gave a first look and this looks good to me :) |
…multiple actions
1fb8344
to
6260313
Compare
Another rustdoc point of view: I agree. :) |
@ismailarilik I am working on making it possible for the triagebot using another Zulip instance for local testing. See my work-in-progress patch, feel free to test it, if you like (I didn't yet). |
This is a requested feature in the Rust project: https://github.com/rust-lang/rust/blob/master/triagebot.toml#L492 (Notice FIXME comments there)
With this PR, multiple
notify-zulip
tables are possible. For example:According to the config above, for example, if an issue with the labels
beta-nominated
,T-rustdoc
andT-miri
opened, the streams266220
and266222
would be notified simultaneously.It is backward-compatible, there is no need to change any current configuration. New
notify-zulip
items should have names like above.Note: This is just a draft PR. I believe my approach is right but I am not confident on the implementation so I need your feedback. Another thing is that I forgot to update the docs. =)