Re: [pve-devel] [PATCH manager v5 08/16] api: notification: add API for getting known metadata fields/values
On 2024-04-19 15:45, Fiona Ebner wrote: > Am 15.04.24 um 10:26 schrieb Lukas Wagner: >> + >> +__PACKAGE__->register_method ({ >> +name => 'get_field_values', >> +path => 'values', >> +method => 'GET', >> +description => 'Returns known notification metadata fields and their >> known values', >> +permissions => { >> +check => ['or', >> +['perm', '/mapping/notifications', ['Mapping.Modify']], >> +['perm', '/mapping/notifications', ['Mapping.Audit']], >> +], >> +}, >> +protected => 1, >> +parameters => { >> +additionalProperties => 0, >> +}, >> +returns => { >> +type => 'array', >> +items => { >> +type => 'object', >> +properties => { >> +'value' => { >> +description => 'Notification metadata value known by the >> system.', >> +type => 'string' >> +}, >> +'comment' => { >> +description => 'Additional comment for this value.', >> +type => 'string', >> +optional => 1, >> +}, >> +'field' => { >> +description => 'Field this value belongs to.', >> +type => 'string', >> +optional => 1, >> +}, >> +'internal' => { >> +description => 'Set if "value" was generated by the system >> and can' >> + . ' safely be used as base for translations.', >> +type => 'boolean', >> +optional => 1, >> +}, > > And wouldn't it be nicer to return already grouped by field? Then maybe > we also don't really need the dedicated fields API endpoint either as > those are just the top-level of the result (with empty array when there > are no values so we don't ever miss any fields). > The design of both endpoints was mostly driven by the intention of keeping the ExtJS side as simple as possible. Two comboboxes, each with their own api endpoint to fetch data from, one setting a filter for the other one. I tried using a single endpoint at first, but was quickly frustrated by ExtJS and its documentation and settled for this approach as a consequence. So I'd prefer to leave it as is :D Regarding the 'internal' flag: Yes, you are right, right now we only need it for 'type'. I'll leave it out then and handle everything in the UI. -- - Lukas ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v5 08/16] api: notification: add API for getting known metadata fields/values
Am 15.04.24 um 10:26 schrieb Lukas Wagner: > + > +__PACKAGE__->register_method ({ > +name => 'get_field_values', > +path => 'values', > +method => 'GET', > +description => 'Returns known notification metadata fields and their > known values', > +permissions => { > + check => ['or', > + ['perm', '/mapping/notifications', ['Mapping.Modify']], > + ['perm', '/mapping/notifications', ['Mapping.Audit']], > + ], > +}, > +protected => 1, > +parameters => { > + additionalProperties => 0, > +}, > +returns => { > + type => 'array', > + items => { > + type => 'object', > + properties => { > + 'value' => { > + description => 'Notification metadata value known by the > system.', > + type => 'string' > + }, > + 'comment' => { > + description => 'Additional comment for this value.', > + type => 'string', > + optional => 1, > + }, > + 'field' => { > + description => 'Field this value belongs to.', > + type => 'string', > + optional => 1, > + }, > + 'internal' => { > + description => 'Set if "value" was generated by the system > and can' > +. ' safely be used as base for translations.', > + type => 'boolean', > + optional => 1, > + }, And wouldn't it be nicer to return already grouped by field? Then maybe we also don't really need the dedicated fields API endpoint either as those are just the top-level of the result (with empty array when there are no values so we don't ever miss any fields). > + }, > + }, > +}, ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v5 08/16] api: notification: add API for getting known metadata fields/values
Am 15.04.24 um 10:26 schrieb Lukas Wagner: > This new API route returns known notification metadata fields and > a list of known possible values. This will be used by the UI to > provide suggestions when adding/modifying match rules. > > Signed-off-by: Lukas Wagner > --- > PVE/API2/Cluster/Notifications.pm | 152 ++ > 1 file changed, 152 insertions(+) > > diff --git a/PVE/API2/Cluster/Notifications.pm > b/PVE/API2/Cluster/Notifications.pm > index 68fdda2a..16548bec 100644 > --- a/PVE/API2/Cluster/Notifications.pm > +++ b/PVE/API2/Cluster/Notifications.pm > @@ -79,12 +79,164 @@ __PACKAGE__->register_method ({ > { name => 'endpoints' }, > { name => 'matchers' }, > { name => 'targets' }, > + { name => 'fields' }, matcher-fields ? > + { name => 'values' }, (matcher-)field-values ? > ]; > > return $result; > } > }); > > +__PACKAGE__->register_method ({ > +name => 'get_fields', > +path => 'fields', > +method => 'GET', > +description => 'Returns known notification metadata fields and their > known values', It does not return their values. > +permissions => { > + check => ['or', > + ['perm', '/mapping/notifications', ['Mapping.Modify']], > + ['perm', '/mapping/notifications', ['Mapping.Audit']], > + ], > +}, > +protected => 1, No need for protected when all it's doing is returning static info. (This would have the privileged pvedaemon execute the request). > +parameters => { > + additionalProperties => 0, > + properties => {}, > +}, > +returns => { > + type => 'array', > + items => { > + type => 'object', > + properties => { > + name => { > + description => 'Name of the field.', > + type => 'string', > + }, > + }, > + }, > + links => [ { rel => 'child', href => '{name}' } ], > +}, > +code => sub { > + # TODO: Adapt this API handler once we have a 'notification registry' > + > + my $result = [ > + { name => 'type' }, > + { name => 'hostname' }, > + { name => 'backup-job' }, > + { name => 'replication-job' }, > + ]; > + > + return $result; > +} > +}); > + > +__PACKAGE__->register_method ({ > +name => 'get_field_values', > +path => 'values', > +method => 'GET', > +description => 'Returns known notification metadata fields and their > known values', > +permissions => { > + check => ['or', > + ['perm', '/mapping/notifications', ['Mapping.Modify']], > + ['perm', '/mapping/notifications', ['Mapping.Audit']], > + ], > +}, > +protected => 1, > +parameters => { > + additionalProperties => 0, > +}, > +returns => { > + type => 'array', > + items => { > + type => 'object', > + properties => { > + 'value' => { > + description => 'Notification metadata value known by the > system.', > + type => 'string' > + }, > + 'comment' => { > + description => 'Additional comment for this value.', > + type => 'string', > + optional => 1, > + }, > + 'field' => { > + description => 'Field this value belongs to.', > + type => 'string', > + optional => 1, Why is the field optional? All values are associated to a field below. > + }, > + 'internal' => { > + description => 'Set if "value" was generated by the system > and can' > +. ' safely be used as base for translations.', > + type => 'boolean', > + optional => 1, > + }, Hmm, not sure about this one. It's only used for the type field right? Can't we just handle that specially in the UI? ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel