-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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: add notifications filtering by processors #24731
Conversation
Changed Packages
|
Missing changeset + API report updates |
fixed |
e3828aa
to
74208fc
Compare
@drodil I made the requested changes. who approves/merges? you? |
@ydayagi no I cannot do that, it's either @mareklibra @Rugvip or @freben |
@drodil is it good to ship from your PoV? |
Hm. It's not a major release for starters. Also not sure if the naming convention is understandable as there are no docs. Maybe also there could be other filters included in the first place and not just to get one specific functionality to work; maximumSeverity, topics from the top of my head. Additionally this change itself does nothing yet and additional changes are required to the actual processors to make this work but that can be done in a separate PR. |
there is no real use for filters other than minSeverity at the moment. |
@ydayagi you mean you don't have a use case for those filters? Backstage is a framework and should be developed as such. |
You are right. I meant that maxSeverity to my knowledge/experience might not have a legitimate use case. But you are right. It is better to have it. |
@mareklibra @drodil @Rugvip |
Signed-off-by: Yaron Dayagi <ydayagi@redhat.com>
Honestly, I do not see a use-case for the Once we have several processors implemented, such filtering will be copy-pasted in each of them. I will not block merging of this PR on that, let's keep this idea for a future refactoring. |
It LGTM, just the change is |
Signed-off-by: Marek Libra <marek.libra@gmail.com>
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Uffizzi Cluster |
Hey, I just made a Pull Request!
Allow processors to distinguish low and high-importance notifications
The first use would be for email processor
Users usually want only high-importance notifications in their emails
Implementation is based on this suggestion #24322 (comment)