From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 1933F1FF0AF for ; Thu, 22 Oct 2026 11:52:04 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 51F6F21612; Thu, 22 Oct 2026 11:52:00 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 22 Oct 2026 11:51:55 +0200 Message-Id: Subject: Re: [PATCH many v4 00/39] notifications: add nested match expressions From: "Lukas Wagner" To: =?utf-8?q?Michael_K=C3=B6ppl?= , "Lukas Wagner" , , X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20261012131413.314088-1-l.wagner@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792662715702 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.277 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 4XALSLAVVK24OP6C45WZLPMMJVYMDBQV X-Message-ID-Hash: 4XALSLAVVK24OP6C45WZLPMMJVYMDBQV X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed Oct 21, 2026 at 4:40 PM CEST, Michael K=C3=B6ppl 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=C3=B6ppl > Tested-by: Michael K=C3=B6ppl > Will include these in v5, thanks again!