Re: [pbs-devel] [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

Re: [pbs-devel] [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Thomas Lamprecht]

FYI, it seems you sent the series to PVE devel by mistake.

On 15.09.21 15:41, Fabian Grünbichler wrote:
> like for manual pulls, but persisted in the sync job config and visible
> in the relevant GUI parts.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     GUI is read-only for now (and defaults to no filtering on creation), as this is
>     a rather advanced feature that requires a complex GUI to be user-friendly
>     (regex-freeform, type-combobox, remote group scanning + selector with
>     additional freeform input).

above paragraph would belong into the commit message IMO.

>     
>     I did test the API manually though to see whether it works as expected, and
>     updating the filter list by overwriting with a new one passed in as multiple
>     parameters works as expected.
>     
>     if we want to make this configurable over the GUI, we probably want to switch
>     the job edit window to a tabpanel and add a second grid tab for selecting
>     the groups

we could also get away by adding it in the advanced section for now.
> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
> index 47e65ae3..2399f11f 100644
> --- a/www/window/SyncJobEdit.js
> +++ b/www/window/SyncJobEdit.js
> @@ -199,6 +199,18 @@ Ext.define('PBS.window.SyncJobEdit', {
>  	],
>  
>  	columnB: [
> +	    {
> +		fieldLabel: gettext('Backup Groups'),
> +		xtype: 'displayfield',
> +		name: 'groups',
> +		renderer: function(value, metadata, record) {
> +		    if (!value) return gettext('All');
> +		    return Ext.String.htmlEncode(value, metadata, record);

Ext.String.htmlEncode only takes a single parameter
https://docs.sencha.com/extjs/7.0.0/classic/Ext.String.html#method-htmlEncode

besides that you could use a arrow fn here, the following would seem quite ok for me for
a renderer:

renderer: v => v ? Ext.String.htmlEncode(v) : gettext('All'),

> +		},
> +		cbind: {
> +		    hidden: '{isCreate}',
> +		},
> +	    },
>  	    {
>  		fieldLabel: gettext('Comment'),
>  		xtype: 'proxmoxtextfield',
> 

Re: [pbs-devel] [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Thomas Lamprecht]

On 16.09.21 09:19, Thomas Lamprecht wrote:
>>     if we want to make this configurable over the GUI, we probably want to switch
>>     the job edit window to a tabpanel and add a second grid tab for selecting
>>     the groups
> we could also get away by adding it in the advanced section for now.

I had a quick talk with Dominik which noticed me that there can be a list of filters.

So a separate tab-panel would seem OK to me. For UX I'd could imagine having a
"Add Filter" button that'd work out similarly to how we have the knet link add in PVE
cluster creation nowadays. In addition to that a preview window would be nice to have,
maybe similar to the "Prune now" one, but we only need the backup-groups here not the
full list of snapshots, so it would be relatively cheap to get.

Re: [pbs-devel] [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Fabian Grünbichler]

On September 16, 2021 9:38 am, Thomas Lamprecht wrote:
> On 16.09.21 09:19, Thomas Lamprecht wrote:
>>>     if we want to make this configurable over the GUI, we probably want to switch
>>>     the job edit window to a tabpanel and add a second grid tab for selecting
>>>     the groups
>> we could also get away by adding it in the advanced section for now.
> 
> I had a quick talk with Dominik which noticed me that there can be a list of filters.
> 
> So a separate tab-panel would seem OK to me. For UX I'd could imagine having a
> "Add Filter" button that'd work out similarly to how we have the knet link add in PVE
> cluster creation nowadays. In addition to that a preview window would be nice to have,
> maybe similar to the "Prune now" one, but we only need the backup-groups here not the
> full list of snapshots, so it would be relatively cheap to get.
> 

yeah, that was kind of what I was going for - but with the added 
complexity of having different filter types with different 
"completion"/selection mechanisms
- the type filter has three static choices
- the regex is freeform (but maybe a dropdown with some common 
  suggestions would be nice? those could also live in the docs though)
- the group filter has a list of existing groups on the remote end to 
  choose from, but also requires freeform entry as option (a group might 
  not exist yet, or not exist any longer, but still needs to be filtered 
  for in the sync job, either to catch the group when it has been 
  created, or to clean it up when it was previously synced, or just for 
  handling the currently set filters correctly)

the last part I tried when writing v1, and it was quite the mess 
(Dominik's suggestion back then was to move it to a tab with a grid 
view, because the combobox selector was too limited IIRC).

a preview/dry-run for pull/sync would be nice in general, and shouldn't 
be too hard to add. it would need to run in a worker though (as it needs 
to query remote and local groups to decide what would be synced, even if 
the bulk work of actually downloading chunks is skipped). it could be 
done with the pull API without persisting the sync config, as those line 
up 1:1.

a preview of just the filterung with the already available (cached) 
remote groups is of course instant. could even be done GUI-side I guess, 
since the current filters are quite trivial to re-implement there. not 
sure how to fit that inside the already big dialogue tab though ;)

Re: [pbs-devel] [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Thomas Lamprecht]

On 16.09.21 09:57, Fabian Grünbichler wrote:
> On September 16, 2021 9:38 am, Thomas Lamprecht wrote:
>> On 16.09.21 09:19, Thomas Lamprecht wrote:
>>>>     if we want to make this configurable over the GUI, we probably want to switch
>>>>     the job edit window to a tabpanel and add a second grid tab for selecting
>>>>     the groups
>>> we could also get away by adding it in the advanced section for now.
>>
>> I had a quick talk with Dominik which noticed me that there can be a list of filters.
>>
>> So a separate tab-panel would seem OK to me. For UX I'd could imagine having a
>> "Add Filter" button that'd work out similarly to how we have the knet link add in PVE
>> cluster creation nowadays. In addition to that a preview window would be nice to have,
>> maybe similar to the "Prune now" one, but we only need the backup-groups here not the
>> full list of snapshots, so it would be relatively cheap to get.
>>
> 
> yeah, that was kind of what I was going for - but with the added 
> complexity of having different filter types with different 
> "completion"/selection mechanisms
> - the type filter has three static choices
> - the regex is freeform (but maybe a dropdown with some common 
>   suggestions would be nice? those could also live in the docs though)

yes, can be similar in spirit of what we do for the calendar-event schedule
field.

> - the group filter has a list of existing groups on the remote end to 
>   choose from, but also requires freeform entry as option (a group might 
>   not exist yet, or not exist any longer, but still needs to be filtered 
>   for in the sync job, either to catch the group when it has been 
>   created, or to clean it up when it was previously synced, or just for 
>   handling the currently set filters correctly)

IMO that fits all in the approach with a [+] button, that could add a row that
looks somewhat like:

Type: [ Combobox ] | [ INPUT ]

Where the type combo-box allows to select for the regex-like filter or
a editabled group selection combobox where one multi-select the existing
but can also add others (i.e., only format not store/content validation).

> 
> the last part I tried when writing v1, and it was quite the mess 
> (Dominik's suggestion back then was to move it to a tab with a grid 
> view, because the combobox selector was too limited IIRC).
> 
> a preview/dry-run for pull/sync would be nice in general, and shouldn't 
> be too hard to add. it would need to run in a worker though (as it needs 
> to query remote and local groups to decide what would be synced, even if 
> the bulk work of actually downloading chunks is skipped). it could be 
> done with the pull API without persisting the sync config, as those line 
> up 1:1.

that's not required IMO, I would only show what the filter affects, that
stuff is then not synced if already present on the remote does not matters
much here on that level.

> a preview of just the filterung with the already available (cached) 
> remote groups is of course instant. could even be done GUI-side I guess, 
> since the current filters are quite trivial to re-implement there. not 
> sure how to fit that inside the already big dialogue tab though ;)

Yeah, replicating the filter on the client side would be nice, single thing
that could cause trouble is difference between JS regex engines and the rust
regex crate we use (the latter is probably much saner).

Re: [pbs-devel] [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Fabian Grünbichler]

On September 16, 2021 9:19 am, Thomas Lamprecht wrote:
> FYI, it seems you sent the series to PVE devel by mistake.

dang, yeah, shell history got me there I guess.

> 
> On 15.09.21 15:41, Fabian Grünbichler wrote:
>> like for manual pulls, but persisted in the sync job config and visible
>> in the relevant GUI parts.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> 
>> Notes:
>>     GUI is read-only for now (and defaults to no filtering on creation), as this is
>>     a rather advanced feature that requires a complex GUI to be user-friendly
>>     (regex-freeform, type-combobox, remote group scanning + selector with
>>     additional freeform input).
> 
> above paragraph would belong into the commit message IMO.
> 
>>     
>>     I did test the API manually though to see whether it works as expected, and
>>     updating the filter list by overwriting with a new one passed in as multiple
>>     parameters works as expected.
>>     
>>     if we want to make this configurable over the GUI, we probably want to switch
>>     the job edit window to a tabpanel and add a second grid tab for selecting
>>     the groups
> 
> we could also get away by adding it in the advanced section for now.

it has the problem of "adding an arbitrary amount of filters", which 
might take up a lot of space, which might make it nicer to move to a 
separate tab. I am not yet sure which direction to go for this.

>> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
>> index 47e65ae3..2399f11f 100644
>> --- a/www/window/SyncJobEdit.js
>> +++ b/www/window/SyncJobEdit.js
>> @@ -199,6 +199,18 @@ Ext.define('PBS.window.SyncJobEdit', {
>>  	],
>>  
>>  	columnB: [
>> +	    {
>> +		fieldLabel: gettext('Backup Groups'),
>> +		xtype: 'displayfield',
>> +		name: 'groups',
>> +		renderer: function(value, metadata, record) {
>> +		    if (!value) return gettext('All');
>> +		    return Ext.String.htmlEncode(value, metadata, record);
> 
> Ext.String.htmlEncode only takes a single parameter
> https://docs.sencha.com/extjs/7.0.0/classic/Ext.String.html#method-htmlEncode

seems like I copied that from an earlier mistake - render_optional_owner 
- made by me as well ;)

> besides that you could use a arrow fn here, the following would seem quite ok for me for
> a renderer:
> 
> renderer: v => v ? Ext.String.htmlEncode(v) : gettext('All'),

yeah, that looks cleaner.

>> +		},
>> +		cbind: {
>> +		    hidden: '{isCreate}',
>> +		},
>> +	    },
>>  	    {
>>  		fieldLabel: gettext('Comment'),
>>  		xtype: 'proxmoxtextfield',
>> 
> 
>