Re: [pve-devel] [PATCH pve-manager 5/7] fabrics: add NodeEdit components

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

On 03.04.2025 11:16, Christoph Heiss wrote:
>On Fri Mar 28, 2025 at 6:13 PM CET, Gabriel Goller wrote:
>[..]
>> diff --git a/www/manager6/sdn/fabrics/openfabric/NodeEdit.js b/www/manager6/sdn/fabrics/openfabric/NodeEdit.js
>> new file mode 100644
>> index 000000000000..f2d204c22542
>> --- /dev/null
>> +++ b/www/manager6/sdn/fabrics/openfabric/NodeEdit.js
>> @@ -0,0 +1,205 @@
>> +Ext.define('PVE.sdn.Fabric.OpenFabric.Node.InputPanel', {
>> +    extend: 'Proxmox.panel.InputPanel',
>> +
>> +    viewModel: {},
>> +
>> +    isCreate: undefined,
>> +    loadClusterInterfaces: undefined,
>> +
>> +    interface_selector: undefined,
>> +    node_not_accessible_warning: undefined,
>
>All new variables should be camelCase [0] here too, as noted in a later
>patch.
>
>[0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

Done, I hope I didn't miss anything.

>> +
>[..]
>> +    initComponent: function() {
>> +	let me = this;
>> +	me.interface_selector = Ext.create('PVE.sdn.Fabric.OpenFabric.InterfacePanel', {
>> +	    name: 'interfaces',
>> +	    parentClass: me.isCreate ? me : undefined,
>> +	});
>> +	me.items = [
>> +	    {
>> +		xtype: 'pveNodeSelector',
>> +		reference: 'nodeselector',
>> +		fieldLabel: gettext('Node'),
>> +		labelWidth: 120,
>> +		name: 'node',
>> +		allowBlank: false,
>> +		disabled: !me.isCreate,
>> +		onlineValidator: me.isCreate,
>> +		autoSelect: me.isCreate,
>> +		listeners: {
>> +		    change: function(f, value) {
>> +			if (me.isCreate) {
>> +			    me.loadClusterInterfaces(value, (result) => {
>> +				me.setValues({ network_interfaces: result });
>> +			    });
>> +			}
>> +		    },
>> +		},
>> +		listConfig: {
>> +		    columns: [
>> +			{
>> +			    header: gettext('Node'),
>> +			    dataIndex: 'node',
>> +			    sortable: true,
>> +			    hideable: false,
>> +			    flex: 1,
>> +			},
>> +		    ],
>> +		},
>
>For the node selector, it would be great if existing nodes could be
>excluded from the dropdown. It isn't possible anyway and throws an error
>on submit, so excluding them here would be good UX.
>
>The `allowedNodes` and/or `disallowedNodes` properties on the
>selector can be used for this.

Yep, implemented this.

>
>> +
>> +	    },
>> +	    me.node_not_accessible_warning,
>> +	    {
>> +		xtype: 'textfield',
>> +		fieldLabel: gettext('Loopback IP'),
>> +		labelWidth: 120,
>> +		name: 'router_id',
>> +		allowBlank: false,
>> +	    },
>
>Here, the user-visible name is 'Loopback IP', internally it is named
>'Router ID'. In the documentation, 'Router-ID' is used too in the
>associated documentation.
>
>Perhaps just settle on Router-ID? As long as it is explained in the
>documentation (maybe also add a tooltip?), I wouldn't use a separate
>name IMHO.

We chose 'Loopback IP' as the user-facing term. Internally it's still
router-id.

>
>> +	    me.interface_selector,
>> +	];
>> +	me.callParent();
>> +    },
>> +});
>> +
>> +Ext.define('PVE.sdn.Fabric.OpenFabric.Node.Edit', {
>> +    extend: 'Proxmox.window.Edit',
>> +    xtype: 'pveSDNFabricAddNode',
>[..]
>> +
>> +    initComponent: function() {
>> +	let me = this;
>> +
>> +	me.node_not_accessible_warning = Ext.create('Proxmox.form.field.DisplayEdit', {
>> +	    userCls: 'pmx-hint',
>> +	    value: gettext('The node is not accessible.'),
>> +	    hidden: true,
>> +	});
>> +
>> +	let ipanel = Ext.create('PVE.sdn.Fabric.OpenFabric.Node.InputPanel', {
>> +	    node_not_accessible_warning: me.node_not_accessible_warning,
>> +	    isCreate: me.isCreate,
>> +	    loadClusterInterfaces: me.loadClusterInterfaces,
>> +	});
>> +
>> +	Ext.apply(me, {
>> +	    subject: gettext('Node'),
>
>Can be moved to a direct member assignment outside of initComponent().

Done.

>> +	    items: [ipanel],
>> +	});
>> +
>> +	me.callParent();
>> +
>> +	if (!me.isCreate) {
>> +	    me.loadAllAvailableNodes((allNodes) => {
>> +		if (allNodes.some(i => i.name === me.node)) {
>> +		    me.loadClusterInterfaces(me.node, (clusterResult) => {
>> +			me.loadFabricInterfaces(me.fabric, me.node, (fabricResult) => {
>> +			    fabricResult.interface = fabricResult.interface
>> +				.map(i => PVE.Parser.parsePropertyString(i));
>> +
>> +			    let data = {
>> +				node: fabricResult,
>> +				network_interfaces: clusterResult,
>> +			    };
>> +
>> +			    ipanel.setValues(data);
>> +			});
>> +		    });
>
>nit: perhaps move some of this into functions? This is pretty deeply
>nested at this point. Would make it definitely more readable.

Hmm debatable I guess—I find this quite ok readable, you get a sense of
what gets done after what.

Even if we split this up in function, the control flow will be harder to
see IMO.

>> +		} else {
>> +		    me.node_not_accessible_warning.setHidden(false);
>> +		    // If the node is not currently in the cluster and not available (we can't get it's interfaces).
>> +		    me.loadFabricInterfaces(me.fabric, me.node, (fabricResult) => {
>> +			fabricResult.interface = fabricResult.interface
>> +			    .map(i => PVE.Parser.parsePropertyString(i));
>> +
>> +			let data = {
>> +			    node: fabricResult,
>> +			};
>> +
>> +			ipanel.setValues(data);
>> +		    });
>> +		}
>> +	    });
>
>Seems oddly similar to the above, could this be simplified/abstracted a
>bit over both? If its feasible, of course.

I think that will make it more complicated, this clearly separates the
"connected" and the "unconnected" node.

>> +	}
>> +
>> +	if (me.isCreate) {
>> +	    me.method = 'POST';
>> +	} else {
>> +	    me.method = 'PUT';
>> +	}
>> +    },
>> +});
>> +
>> diff --git a/www/manager6/sdn/fabrics/ospf/NodeEdit.js b/www/manager6/sdn/fabrics/ospf/NodeEdit.js
>> new file mode 100644
>> index 000000000000..d022272b5428
>> --- /dev/null
>> +++ b/www/manager6/sdn/fabrics/ospf/NodeEdit.js
>> @@ -0,0 +1,207 @@
>> +Ext.define('PVE.sdn.Fabric.Ospf.Node.InputPanel', {
>> +    extend: 'Proxmox.panel.InputPanel',
>[..]
>> +
>> +Ext.define('PVE.sdn.Fabric.Ospf.Node.Edit', {
>
>Overall, seems this component is mostly the same as the respective
>OpenFabric counterpart - maybe a common parent ExtJS component can be
>created?

Yep, factored out the NodeEdit and FabricEdit for OpenFabric and OSPF so
we have to common components.

>[..]
>> +
>> +    initComponent: function() {
>> +	let me = this;
>> +
>> +	me.node_not_accessible_warning = Ext.create('Proxmox.form.field.DisplayEdit', {
>> +	    userCls: 'pmx-hint',
>> +	    value: gettext('The node is not accessible.'),
>> +	    hidden: true,
>> +	});
>> +
>> +
>> +	let ipanel = Ext.create('PVE.sdn.Fabric.Ospf.Node.InputPanel', {
>> +	    interface_selector: me.interface_selector,
>> +	    node_not_accessible_warning: me.node_not_accessible_warning,
>> +	    isCreate: me.isCreate,
>> +	    loadClusterInterfaces: me.loadClusterInterfaces,
>> +	});
>> +
>> +	Ext.apply(me, {
>> +	    subject: gettext('Node'),
>> +	    items: [ipanel],
>> +	});
>> +
>> +	me.callParent();
>> +
>> +	if (!me.isCreate) {
>> +	    me.loadAllAvailableNodes((allNodes) => {
>> +		if (allNodes.some(i => i.name === me.node)) {
>> +		    me.loadClusterInterfaces(me.node, (clusterResult) => {
>> +			me.loadFabricInterfaces(me.fabric, me.node, (fabricResult) => {
>> +			    fabricResult.interface = fabricResult.interface
>> +				.map(i => PVE.Parser.parsePropertyString(i));
>> +
>> +			    let data = {
>> +				node: fabricResult,
>> +				network_interfaces: clusterResult,
>> +			    };
>> +
>> +			    ipanel.setValues(data);
>> +			});
>> +		    });
>> +		} else {
>> +		    me.node_not_accessible_warning.setHidden(false);
>> +		    // If the node is not currently in the cluster and not available (we can't get it's interfaces).
>> +			me.loadFabricInterfaces(me.fabric, me.node, (fabricResult) => {
>
>nit: wrong indentation :^)

Fixed :)

Thanks for the review!


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