From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id E08BB1FF2C5 for ; Mon, 8 Jul 2024 10:12:21 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0863B371FD; Mon, 8 Jul 2024 10:12:41 +0200 (CEST) Date: Mon, 08 Jul 2024 10:12:37 +0200 Message-Id: To: "Proxmox VE development discussion" From: "Max Carrara" Mime-Version: 1.0 X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240705134651.296607-1-l.wagner@proxmox.com> In-Reply-To: <20240705134651.296607-1-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH many v8 00/13] notifications: notification metadata matching improvements X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Fri Jul 5, 2024 at 3:46 PM CEST, Lukas Wagner wrote: > This patch series attempts to improve the user experience when creating > notification matchers. The below can pretty much just be considered "proofreading" as I haven't built and tested your changes, but since you already got a lot of feedback on the last couple versions, I think that's fine. ;) Just wanted to comment anyway. The patches are rather easy to follow, and even though I'm no expert when it comes to Ext JS, the UI changes look fine to me too. The new UI logic feels (and is) much cleaner than before. There's nothing I can otherwise comment on; everything's pretty straight-forward. The *only* things I have noticed are rather minor - there are two tiny typos in the commit messages of patch 01 and 02, but these can probably be fixed when applying the series: 01: Last sentence of message - "It it can be considered internal." 02: First sentence of message - "This allows us to access us the [...]" That's it otherwise from me - LGTM. Reviewed-by: Max Carrara > > Some of the noteworthy changes: > - Allow setting a custom backup job ID, similar how we handle it for > sync/prune jobs in PBS (to allow recognizable names used in matchers) > - New metadata fields: > - job-id: Job ID for backup-jobs or replication-jobs > - Add an API that enumerates known notification metadata fields/values > - Suggest known fields/values in match rule window > - Some code clean up for match rule edit window > - Extended the 'exact' match-field mode - it now allows setting multiple > allowed values, separated via ',': > e.g. `match-field exact:type=replication,fencing > Originally, I created a separate 'list' match type for this, but > since the semantics for a list with one value and 'exact' mode > are identical, I decided to just extend 'exact'. > This is a safe change since there are are no values where a ',' > makes any sense (config IDs, hostnames) > > NOTE: Might need a versionened break, since the widget-toolkit-patches > depend on new APIs provided by pve-manager. If the API is not present, > creating matchers with 'match-field' does not work (cannot load lists > of known values/fields) > > Inter-Dependencies: > - the widget-toolkit dep in pve-manager needs to be bumped > to at least 4.1.4 > (we need "utils: add mechanism to add and override translatable notification event > descriptions in the product specific UIs", otherwise the UI breaks > with the pve-manager patches applied) --> already included a patch for > this > - widget-toolkit relies on a new API endpoint provided by pve-manager: > --> we require a versioned break in widget-toolkit on pve-manager > - pve-manager needs bumped pve-guest-common (thx @Fabian) > > Changelog: > - v8: incorporate feedback from @Fabian, thx a lot! > - Made 'job-id' API param usable by root@pam only - this should prevent > abuse by spoofing job-id, potentially bothering other users with bogus > notifications. > - Don't set 'job-id' when starting a backup job via 'Run now' in the UI > - Add a note to the docs explaining when job-id is set and when not. > - Drop already applied patches > - v7: incorporated some more feedback from @Fiona, thx! > - Fixed error when switching from 'exact' to 'regex' if the text field > was empty > - rebased to latest master > - 'backport' doc improvements from PBS > - bumped widget-toolkit dep > - v6: incorporate feedback from @Fiona, thx! > - rename 'id' -> 'job-id' in VZDump API handler > - consolidate 'replication-job'/'backup-job' to 'job-id' > - Move 'job-id' setting to advanced tab in backup job edit. > - Don't use 'internal' flag to mark translatable fields, since > the only field where that's necessary is 'type' for now - so > just add a hardcoded check > - v5: > - Rebased onto latest master, resolving some small conflict > - v4: > - widget-toolkit: break out changes for the utils module so that they > can be applied ahead of time to ease dep bumping > - don't show Job IDs in the backup/replication job columns > - v3: > - Drop already applied patches for `proxmox` > - Rebase onto latest master - minor conflict resolution was needed > - v2: > - include 'type' metadata field for forwarded mails > --> otherwise it's not possible to match them > - include Maximilliano's T-b trailer in UI patches > > pve-guest-common: > > Lukas Wagner (1): > vzdump: common: allow 'job-id' as a parameter without being in schema > > src/PVE/VZDump/Common.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > > pve-manager: > > Lukas Wagner (5): > api: jobs: vzdump: pass job 'job-id' parameter > ui: dc: backup: allow to set custom job id in advanced settings > api: notification: add API for getting known metadata fields/values > ui: utils: add overrides for translatable notification fields/values > d/control: bump proxmox-widget-toolkit dependency to 4.1.4 > > PVE/API2/Backup.pm | 2 +- > PVE/API2/Cluster/Notifications.pm | 139 ++++++++++++++++++++ > PVE/API2/VZDump.pm | 13 +- > PVE/Jobs/VZDump.pm | 4 +- > PVE/VZDump.pm | 6 +- > debian/control | 2 +- > www/manager6/Utils.js | 11 ++ > www/manager6/dc/Backup.js | 4 - > www/manager6/panel/BackupAdvancedOptions.js | 23 ++++ > 9 files changed, 192 insertions(+), 12 deletions(-) > > > proxmox-widget-toolkit: > > Lukas Wagner (4): > notification: matcher: match-field: show known fields/values > notification: matcher: move match-field formulas to local viewModel > notification: matcher: move match-calendar fields to panel > notification: matcher: move match-severity fields to panel > > src/data/model/NotificationConfig.js | 12 + > src/window/NotificationMatcherEdit.js | 613 ++++++++++++++++++-------- > 2 files changed, 441 insertions(+), 184 deletions(-) > > > pve-docs: > > Lukas Wagner (3): > notifications: describe new notification metadata fields > notifications: match-field 'exact'-mode can now match multiple values > notifications: add note regarding when 'job-id' is set for backups > > notifications.adoc | 42 +++++++++++++++++++++--------------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > > Summary over all repositories: > 13 files changed, 655 insertions(+), 218 deletions(-) _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel