Re: [pve-devel] [PATCH pve-manager 6/7] fabrics: Add main FabricView

[Gabriel Goller <g.goller@proxmox.com>]

On 02.04.2025 11:50, Christoph Heiss wrote:
>Some comments inline - did the review mostly in tandem with testing the
>UI, to get a better context.
>
>On Fri Mar 28, 2025 at 6:13 PM CET, Gabriel Goller wrote:
>[..]
>> diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
>> index 74728c8320e9..68f7be8d6042 100644
>> --- a/www/manager6/dc/Config.js
>> +++ b/www/manager6/dc/Config.js
>> @@ -229,6 +229,14 @@ Ext.define('PVE.dc.Config', {
>>  		    hidden: true,
>>  		    iconCls: 'fa fa-shield',
>>  		    itemId: 'sdnfirewall',
>> +		},
>> +		{
>> +		    xtype: 'pveSDNFabricView',
>> +		    groups: ['sdn'],
>> +		    title: gettext('Fabrics'),
>
>Do we really want to translate 'Fabrics'? Or rather just keep the name
>always at 'Fabrics' as the name of the concept/feature itself? Since it
>is a established term, after all. I'd keep it, IMHO.
>
>(But we seem to translate everything else here in the file, so really
>not sure.)

Hmm I mean it's probably the same in every language, but we also seem to
translate everything, even "VNET Firewall" and "IPAM", so IMO I'd leave
it just for the sake of consistency.

>> +		    hidden: true,
>> +		    iconCls: 'fa fa-road',
>> +		    itemId: 'sdnfabrics',
>>  		});
>>  	    }
>>
>> diff --git a/www/manager6/sdn/FabricsView.js b/www/manager6/sdn/FabricsView.js
>> new file mode 100644
>> index 000000000000..0ef12defb1a8
>> --- /dev/null
>> +++ b/www/manager6/sdn/FabricsView.js
>[..]
>> +Ext.define('PVE.sdn.Fabric.View', {
>[..]
>> +	{
>> +	    text: gettext('Protocol'),
>> +	    dataIndex: 'protocol',
>> +	    width: 100,
>> +	    renderer: function(value, metaData, rec) {
>> +		if (rec.data.type !== 'fabric') {
>> +		    return "";
>> +		}
>> +
>> +		return PVE.Utils.render_sdn_pending(rec, value, 'protocol');
>> +	    },
>
>A mapping for the internal protocol name to a user-visible would be nice
>here, i.e. 'ospf' -> 'OSPF' and 'openfabric' -> 'OpenFabric'. Not much
>difference currently, but would look a bit better in the UI :)

Oh, that's a nice one, added this.

>> [snip]
>> +		    handler: 'addActionTreeColumn',
>> +		    getTip: (_v, _m, _rec) => gettext('Add'),
>
>nit: "Add Node" would be better here for node rows, if that's possible.

I agree.

>> +		    getClass: (_v, _m, { data }) => {
>> +			if (data.type === 'fabric') {
>> +			    return 'fa fa-plus-circle';
>> +			}
>> +
>> +			return 'pmx-hidden';
>> +		    },
>> +		    isActionDisabled: (_v, _r, _c, _i, { data }) => data.type !== 'fabric',
>> +		},
>> +		{
>> +		    tooltip: gettext('Edit'),
>
>^ same as above

Here it's a bit different, because the Edit button is on Fabrics and
Nodes, so I'd have to add the getTip callback and format the type in it.
IMO this is not worth it.

>> +		    handler: 'editAction',
>> +		    getClass: (_v, _m, { data }) => {
>> +			// the fabric type (openfabric, ospf, etc.) cannot be edited
>> +			if (data.type) {
>> +			    return 'fa fa-pencil fa-fw';
>> +			}
>> +
>> +			return 'pmx-hidden';
>> +		    },
>> +		    isActionDisabled: (_v, _r, _c, _i, { data }) => !data.type,
>> +		},
>> +		{
>> +		    tooltip: gettext('Delete'),
>
>^ and same here as well

Same as my answer above.

>> +		    handler: 'deleteAction',
>> +		    getClass: (_v, _m, { data }) => {
>> +			// the fabric type (openfabric, ospf, etc.) cannot be deleted
>> +			if (data.type) {
>> +			    return 'fa critical fa-trash-o';
>> +			}
>> +
>> +			return 'pmx-hidden';
>> +		    },
>> +		    isActionDisabled: (_v, _r, _c, _i, { data }) => !data.type,
>> +		},
>> +	    ],
>> +	},
>> +	{
>> +	    header: gettext('Interfaces'),
>> +	    width: 100,
>> +	    dataIndex: 'interface',
>> +	    renderer: function(value, metaData, rec) {
>> +		let interfaces = rec.data.pending?.interface || rec.data.interface || [];
>
>nit: could be const (see [0] tho regarding rules for const/let)
>
>[0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables

Yep.

>> [snip]
>> +    initComponent: function() {
>> +	let me = this;
>> +
>> +
>> +	let add_button = new Proxmox.button.Button({
>
>New variables should generally be camelCase [0].
>
>[1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

Fixed as well.

>> +	    text: gettext('Add Node'),
>> +	    handler: 'addActionTbar',
>> +	    disabled: true,
>> +	});
>
>Since there is also an "add node" button in the "Action" column for each
>fabric, do we need a separate one in the top-bar as well?

We talked with Dominik off-list and he mentioned that in the
ResourceMapping the users didn't always understand what the button did or
how to add a "child". That's why we add both buttons.

>> +
>> +	let set_add_button_status = function() {
>> +	    let selection = me.view.getSelection();
>> +
>> +	    if (selection.length === 0) {
>> +		return;
>> +	    }
>> +
>> +	    add_button.setDisabled(selection[0].data.type !== 'fabric');
>> +	};
>> +
>> +	Ext.apply(me, {
>> +	    tbar: [
>> +		{
>> +		    text: gettext('Add Fabric'),
>> +		    menu: [
>> +			{
>> +			    text: gettext('OpenFabric'),
>
>'OpenFabric' also should not be translated, right? As its the name of
>the implementation/protocol.

Right.

>> +			    handler: 'openAddOpenFabricWindow',
>> +			},
>> +			{
>> +			    text: gettext('OSPF'),
>
>^ same here

Fixed it also.

>> [snip]
>> +    controller: {
>> +	xclass: 'Ext.app.ViewController',
>> +
>> +	reload: function() {
>> +	    let me = this;
>> +
>> +	    Proxmox.Utils.API2Request({
>> +		url: `/cluster/sdn/fabrics/?pending=1`,
>> +		method: 'GET',
>> +		success: function(response, opts) {
>> +		    let ospf = response.result.data.ospf;
>> +		    let openfabric = response.result.data.openfabric;
>> +
>> +		    // add some metadata so we can merge the objects later and still know the protocol/type
>> +		    ospf = ospf.map(x => {
>> +			if (x.state && x.state === 'new') {
>> +			    Object.assign(x, x.pending);
>> +			}
>> +
>> +			if (x.ty === 'fabric') {
>> +			    return Object.assign(x, { protocol: "ospf", name: x.area });
>> +			} else if (x.ty === 'node') {
>> +			    let id = x.node_id.split("_");
>> +			    return Object.assign(x,
>> +				{
>> +				    protocol: "ospf",
>> +				    node: id[1],
>> +				    fabric: id[0],
>> +				},
>> +			    );
>> +			} else {
>> +			    return x;
>> +			}
>> +		    });
>
>Perhaps factor this out a bit into a separate function, making the
>success() definition here a bit more succinct?

Done, created a few functions mapOpenFabric, mapOspf and mapTree.

>> [snip]
>^ and same here, moving it into a separate function instead of
>open-coding it here. Would make the whole bit here more readable/easier
>to understand.

See above.

>> +			}
>> +
>> +			Object.assign(fabric, {
>> +			    type: 'fabric',
>> +			    protocol: fabric.protocol,
>> +			    expanded: true,
>> +			    name: fabric.fabric_id || fabric.area,
>> +			    iconCls: 'fa fa-road x-fa-treepanel',
>> +			});
>> +
>> +			return fabric;
>> +		    });
>> +
>[..]
>> +
>> +	addActionTreeColumn: function(_grid, _rI, _cI, _item, _e, rec) {
>> +	    let me = this;
>
>nit: never used, just drop it

Done.

>> +
>> +	deleteAction: function(table, rI, cI, item, e, { data }) {
>> +	    let me = this;
>> +	    let view = me.getView();
>> +
>> +	    Ext.Msg.show({
>> +		title: gettext('Confirm'),
>> +		icon: Ext.Msg.WARNING,
>> +		message: Ext.String.format(gettext('Are you sure you want to remove the fabric {0}?'), `${data.name}`),
>
>nit: unnecessary string interpolation

Oh, yes. Also added some quotes around the name, as it's more readable
that way.

>> +		buttons: Ext.Msg.YESNO,
>> +		defaultFocus: 'no',
>> +		callback: function(btn) {
>> +		    if (btn !== 'yes') {
>> +			return;
>> +		    }
>> +
>> +		    let url;
>> +		    if (data.type === "node") {
>> +			url = `/cluster/sdn/fabrics/${data.protocol}/${data._fabric}/node/${data.name}`;
>> +		    } else if (data.type === "fabric") {
>> +			url = `/cluster/sdn/fabrics/${data.protocol}/${data.name}`;
>> +		    } else {
>> +			console.warn("deleteAction: missing type");
>> +		    }
>> +
>> +		    Proxmox.Utils.API2Request({
>> +			url,
>> +			method: 'DELETE',
>> +			waitMsgTarget: view,
>> +			failure: function(response, opts) {
>> +			    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
>
>Could use `Proxmox.Utils.errorText` here instead of `gettext('Error')`.

Nice!

>> +			},
>> +			callback: me.reload.bind(me),
>
>nit: `() => me.reload()` is already used multiple times in this file, so
>use that here too for consistency? Think it's the preferred style
>anyway.

Right.

>> [snip]

Thanks for the review!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel