Skip to content
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

Configure team nominate label in prioritization alerts on Zulip #1766

Conversation

apiraino
Copy link
Contributor

@apiraino apiraino commented Jan 29, 2024

This patch tries to customize the prioritization Zulip notification (I-{team}-nominated) according to the team labels on the issue.

It's not complete, there are some cases not covered but I didn't feel complicating too much the logic.

And this is a brainstoming for @fmease that suggested doing so. Feel free to own this patch and bring it to completion (unsure you have permissions on this repo) or suggest how you would like the implementation to be.

This patch completes rust-lang/rust#119959 (review)

Thanks!

@apiraino apiraino force-pushed the configure-team-in-prioritization-alerts-on-zulip branch 2 times, most recently from fbe8cd9 to 48e3f3a Compare April 3, 2024 15:35
@apiraino apiraino marked this pull request as ready for review April 3, 2024 15:35
@apiraino
Copy link
Contributor Author

apiraino commented Apr 3, 2024

hey @fmease I was looking into completing this small change.

wdyt? Does it look like what you wanted to accomplish?

@fmease
Copy link
Member

fmease commented Apr 5, 2024

It definitely does what I had envisioned, thanks! 😊 The major issue I see is that this PR adds very rust-lang/rust-specific logic to triagebot. While triagebot still contains a lot of things specific to rust-lang/rust, I think the general vision for this bot is to move anything repo-specific to configs & to make it repo-agnostic.

cc @WaffleLapkin

@fmease
Copy link
Member

fmease commented Apr 5, 2024

And I really don't know how one would best abstract over this feature and make it configurable.
If the maintainers of this repo are fine with merging this as is, then that's awesome, if they aren't we need to change the approach.

@Mark-Simulacrum, what do you think?

src/handlers/notify_zulip.rs Outdated Show resolved Hide resolved
src/handlers/notify_zulip.rs Outdated Show resolved Hide resolved
@WaffleLapkin
Copy link
Member

@fmease all zulip stuff is highly specific to rust-lang. so while I'd like it to be more configurable, this specific change does not really change much.

@apiraino apiraino force-pushed the configure-team-in-prioritization-alerts-on-zulip branch from 48e3f3a to bf17d70 Compare April 8, 2024 16:10
@apiraino
Copy link
Contributor Author

apiraino commented Apr 8, 2024

The major issue I see is that this PR adds very rust-lang/rust-specific logic to triagebot.

That's true, I agree. However making it really generic is beyond the time I want to allocate on implementing this change (see also my first comment). So if it's not that bad and it's accepted, great, otherwise I'll have to drop the mic on this 😄

@apiraino apiraino force-pushed the configure-team-in-prioritization-alerts-on-zulip branch from bf17d70 to d50a53c Compare April 8, 2024 16:26
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

small stylistic nits, not blocking. sorry, forgot to submit a code review earlier.

src/handlers/notify_zulip.rs Outdated Show resolved Hide resolved
src/handlers/notify_zulip.rs Outdated Show resolved Hide resolved
src/handlers/notify_zulip.rs Outdated Show resolved Hide resolved
@apiraino apiraino force-pushed the configure-team-in-prioritization-alerts-on-zulip branch from d50a53c to 6fcc7be Compare April 9, 2024 08:21
@apiraino
Copy link
Contributor Author

apiraino commented Apr 9, 2024

applied the suggested nits!

thanks @fmease for your time :)

@fmease
Copy link
Member

fmease commented Apr 17, 2024

needs rebase

@apiraino apiraino force-pushed the configure-team-in-prioritization-alerts-on-zulip branch from 6fcc7be to 0db30d1 Compare April 17, 2024 10:15
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks! I'm going to go ahead and merge based on previous reviews, and since this seems pretty simple.

@apiraino Do you mind following up and post an update to the documentation at https://github.com/rust-lang/rust-forge/blob/master/src/triagebot/zulip-notifications.md to add a brief mention of this?

@ehuss ehuss merged commit 2830b5d into rust-lang:master May 10, 2024
2 checks passed
@apiraino apiraino deleted the configure-team-in-prioritization-alerts-on-zulip branch May 13, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants