Re: [pve-devel] [PATCH v3 many 00/66] fix #4156: introduce new notification system

[Lukas Wagner <l.wagner@proxmox.com>]

On 7/18/23 14:34, Dominik Csapak wrote:
> gave the series a quick spin, review of the gui patches comes later ;)
> 
> a few high level comments from a user perspective:
> 
> * the node fencing/replication edit windows always shows the warning that it shouldn't be
>    disabled, that should imo only be there if i select 'never' ?
>    (conversely, the package update window never shows the warning...)
Good point, I'll try to make it only show up if 'never' is selected.
The warning came actually to be as an input from Aaron, so that user's don't turn
off stuff for critical events without knowing what they are doing.
I didn't add them to the package update notifications because there, the setting
already existed in datacenter config without a warning and it seemed less critical to me.
But yeah, I guess it would make sense to add it there as well.

> * when we have this, we could remove the pacakge notify line from the datacenter options, no?

Yes, you are right, forgot about that.

> * imho having "Notification Targets" directly below "Notifications" is a bit redundant
>    (and wasting space), but it's just a minor thing

True, I can probably remove that, since it's already clear from the menu on the left-hand side.

> * the filter 'mode' is also not exposed on the gui (intentionally?)> * also the mode is not quite clear since only one filter can be active per target?
>    (or is it meant when there are multiple filter by means of notification groups?)> * what is a filter without a minimum severity? what does it do?
>    (does it make sense to allow such filters in the first place?)

Filters will be extended in the future so that they can match on multiple properties.
Every notification has a severity as well as arbitrary metadata attached to it (e.g. could be
hostname, backup-job ID, etc.).

In future, a filter could be like

filter: foo
     mode and|or
     min-severity error
     match-property hostname=pali


So here, the mode determines if `min-severity` AND/OR `match-property` have to match.
That is also the reason why a filter without min-severity can exist.

Also, I'm thinking of supporting 'sub-filters':

filter: foo
     mode or
     sub-filter a
     sub-filter b

filter: a
     mode and
     min-severity error
     match-property hostname=pali

filter: b
     mode and
     min-severity info
     match-property hostname=haya

`mode`, `invert-match` and `sub-filter` together basically would allow users to
create arbitrarily complex filter 'formulas'.


I already have patches laying around that implement the additional filter matchers, but
decided to not include them in the patch series yet. Before the new matchers are merged,
I would need to 'stabilize' the properties associate with every single notification event,
as at that point those become part of our public facing API. At the moment, the properties
are only an implementation detail that is used for rendering notification templates.

This is also the reason why the filter implementation (filter.rs) is somewhat overkill
atm for _just_ severity filtering. Everything needed for these advanced features is already
in place - because I already implemented that stuff and cut it out later for the patch series.

> * the backup edit window is rather tall at this point, and if we assume a minimum
>    screen size of 1280x720 there is not quite enough space when you subtract
>    the (typical) os bar and browser window decoration, maybe a 'notification'
>    tab would be better (we already have some tabs there anyway)

Good point, will look into that.

> * i found one bug, but not quite sure yet where it comes from exactly,
>    putting in emojis into a field (e.g. a comment or author) it's accepted,
>    but editing a different entry fails with:
> 
> --->8---
> could not serialize configuration: writing 'notifications.cfg' failed: detected unexpected control character in section 'testgroup' key 'comment' (500)
> ---8<---
> 
> not sure where the utf-8 info gets lost. (or we could limit all fields to ascii?)
> such a notification target still works AFAICT (but if set as e.g. the author it's
> probably the wrong value)
> 
> (i used 😀 as a test)

I will investigate, thanks!
> 
> 
> 
> otherwise works AFAICT
> 

Thanks so much for giving this a spin!

-- 
- Lukas