Re: [pve-devel] [PATCH many v8 00/13] notifications: notification metadata matching improvements

["Max Carrara" <m.carrara@proxmox.com>]

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 <m.carrara@proxmox.com>

>
> 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