From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 07F161FF16B
for <inbox@lore.proxmox.com>; Thu, 3 Apr 2025 11:17:34 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id C974A3ADB9;
Thu, 3 Apr 2025 11:17:19 +0200 (CEST)
Date: Thu, 03 Apr 2025 11:16:45 +0200
Message-Id: <D8WW2HAVN0E1.1SEMKA8CUSXRQ@proxmox.com>
From: "Christoph Heiss" <c.heiss@proxmox.com>
To: "Gabriel Goller" <g.goller@proxmox.com>
Mime-Version: 1.0
X-Mailer: aerc 0.20.1
References: <20250328171340.885413-1-g.goller@proxmox.com>
<20250328171340.885413-50-g.goller@proxmox.com>
In-Reply-To: <20250328171340.885413-50-g.goller@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.030 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH pve-manager 5/7] fabrics: add NodeEdit
components
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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