Re: [PATCH many v4 00/39] notifications: add nested match expressions

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

On Wed Oct 21, 2026 at 4:40 PM CEST, Michael Köppl wrote:
> Thanks for your hard work on this! Tested this version of the series as
> well.

Huge thanks for the in-depth testing and review, this is much
appreciated.

>
> Upgrade path:
> - Create multiple legacy matchers to check that the conversion works as
>   expected:
>   - all mode with match field and match severity
>   - any mode with inverted match
>   - matching multiple fields with exact or regex
>   - "at least one does not match" with calendar and severity
>   - no clauses but inverted (always matching)
>   - calendar match with "mon8-12" (no space).
> - The above were all converted as expected. The only noteworthy
>   conversion is the one with the "mon8-12" calendar match. The UI when
>   does not display any selected days and saving is blocked because
>   "Calendar rules must match at least one day."

Thanks, fixed the regex to properly parse the range if the space is
missing. Will be included in v5.

'Unfortunately', the time range parser is extremely lenient, so it is
really tricky to get the parser 100% correct.

>
> Partial upgrade, 1 node not upgraded,
> - Attempted to edit an existing legacy matcher from an upgraded node.
>   Was rejected as expected.
> - Attempted to edit a target with and without the default-matcher
>   present in the config. Noticed that when the legacy default-matcher is
>   not written to the config yet, editing the notification target is not
>   possible, failing with "At least one cluster node does not support
>   matcher expressions". Probably because the implementation wants to
>   write the new default-matcher with the expression to the config and
>   performing the version check before. If the legacy default-matcher is
>   already present, editing the target works.

Yes, I had to completely block all editing if any node is still
outdated, since any edit could bring in the default matcher which
contains an expression. An annoying side effect of how the built-ins are
implemented.

> - On any not-yet-upgraded node, editing targets and matchers continues
>   to work.
>
> Fully upgraded cluster, UI and API tests:
> - Through the API/pvesh, adding expressions with both exact matches and
>   regex is possible, resulting in an expression containing only
>   whichever came first in the expression being stored. Noted in on the
>   patch that implements this behavior. Not a blocker IMO, but wanted to
>   point it out nonetheless.
> - Through the API/pvesh, field names that would previously have been
>   rejected by the legacy matchers are now accepted (e.g. spaces,
>   umlauts, etc.). Added a comment to the individual patch, but could
>   also be a follow-up IMO.
> - Played around with a match expression with variations of nested
>   not(not(...)) and compared the resulting tree in the UI to logically
>   match.
> - Edited the tree in the UI so see if children of expression nodes were
>   moved as expected, if inversions are applied correctly, and if nodes
>   cannot be dropped to the root level.
> - Changed "All sub-rules match" in the UI tree to "Match Severity". This
>   makes all the children disappear without any confirmation dialog. The
>   entire editing process can of course be aborted to avoid this, but
>   considering the deletion of the "All sub-rules match" requires
>   confirmation, I think this case would also benefit from such a
>   confirmation. Can easily be a follow-up, though. This is also true
>   when changing the root node.

I tried implementing such a confirmation, but it turned out to be much
trickier than the already existing 'delete' case; so I left it for now.

I will revisit it if I find the time and/or there are bug reports about
this behavior.

> - Triggered various notifications, checking that the expected matchers
>   matched.
>
> Other than what's noted above on the individual scenarios, the new match
> expressions as well as upgrading from the legacy ones worked as expected
> and IMO the UI is really intuitive and solid regarding what it allows.
> Really nice work!
>
> With my comments addressed, consider this:
> Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
> Tested-by: Michael Köppl <m.koeppl@proxmox.com>
>

Will include these in v5, thanks again!