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
