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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
