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

["Christoph Heiss" <c.heiss@proxmox.com>]

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

> +
[..]
> +    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.

> +
> +	    },
> +	    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.

> +	    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().

> +	    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.

> +		} 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.

> +	}
> +
> +	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?

[..]
> +
> +    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 :^)


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