Re: [PATCH pve-manager v2] fix #7300: acl path include pre-generated zones and fabrics

[Stefan Hanreich <s.hanreich@proxmox.com>]

Thanks for looking into this!

Tested this patch series on my machine and works as advertised -
comments inline.

On 3/11/26 10:42 AM, David Riley wrote:
> Permission Path selection will show:
> '/sdn/zones/<zone>'
> '/sdn/fabrics/<fabric>'
> 
> The case 'network' is used because this will act as the top-level
> resource for all networking entities (including SDN).
> 
> see: https://git.proxmox.com/?p=pve-manager.git;a=commit;h=9ac04d9572a458aeb891feb9b695d793cf7b122d
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---

When sending a new version of a patch series it is good to include a
changelog for the patch series. This makes life easier for people
reviewing the patch series and allows for easily seeing what changed.

Even if nothing really changed (afaict the Signed-off-by tag was missing
and has been added) it makes it easy to tell that nothing significant
changed from v1 ;)

>  www/manager6/data/PermPathStore.js | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/data/PermPathStore.js b/www/manager6/data/PermPathStore.js
> index c7ec4231..bba7c7e7 100644
> --- a/www/manager6/data/PermPathStore.js
> +++ b/www/manager6/data/PermPathStore.js
> @@ -22,7 +22,7 @@ Ext.define('PVE.data.PermPathStore', {
>      ],
>  
>      constructor: function (config) {
> -        var me = this;
> +        let me = this;

nit: this is not strictly related to the change and it might be better
to send as an upfront / separate patch.

>  
>          config = config || {};
>  
> @@ -36,6 +36,9 @@ Ext.define('PVE.data.PermPathStore', {
>                  case 'node':
>                      path = '/nodes/' + record.get('text');
>                      break;
> +                case 'network':
> +                    path = '/sdn/' + record.data['network-type'] + 's/' + record.data.network;

nit: the surrounding code utilizes .get() - so it'd make sense to do the
same here for the sake of consistency.
Unless there is a specific reason for accessing record.data directly?

> +                    break;
>                  case 'qemu':
>                      path = '/vms/' + record.get('vmid');
>                      break;