From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 6F2881FF16E for ; Mon, 29 Jul 2024 12:43:10 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 78A681B2D5; Mon, 29 Jul 2024 12:43:10 +0200 (CEST) Mime-Version: 1.0 Date: Mon, 29 Jul 2024 12:42:36 +0200 Message-Id: From: "Shannon Sterz" To: "Aaron Lauterer" , "Proxmox VE development discussion" X-Mailer: aerc 0.17.0-69-g65571b67d7d3-dirty References: <20240703080147.81154-1-a.lauterer@proxmox.com> <20240703080147.81154-5-a.lauterer@proxmox.com> In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL -0.048 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 widget-toolkit v3] fix #3892: Network: add bridge vids field for bridge_vids X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Mon Jul 29, 2024 at 12:25 PM CEST, Aaron Lauterer wrote: > > > On 2024-07-26 14:22, Shannon Sterz wrote: > > On Wed Jul 3, 2024 at 10:01 AM CEST, Aaron Lauterer wrote: > >> The new optional bridge_vids field allows to set that property via the > >> GUI. Since the backend needs to support it, the field needs to be > >> explicitly enabled. > >> > >> For now, Proxmox VE (PVE) is the use case. > >> > >> Signed-off-by: Aaron Lauterer > >> --- > >> changes since v2: > >> * added validation code following how it is implemented in the API > >> > >> src/node/NetworkEdit.js | 62 +++++++++++++++++++++++++++++++++++++++++ > >> src/node/NetworkView.js | 5 ++++ > >> 2 files changed, 67 insertions(+) > >> > >> diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js > >> index 27c1baf..8c1b135 100644 > >> --- a/src/node/NetworkEdit.js > >> +++ b/src/node/NetworkEdit.js > >> @@ -2,6 +2,9 @@ Ext.define('Proxmox.node.NetworkEdit', { > >> extend: 'Proxmox.window.Edit', > >> alias: ['widget.proxmoxNodeNetworkEdit'], > >> > >> + // Enable to show the VLAN ID field > >> + bridge_set_vids: false, > >> + > >> initComponent: function() { > >> let me = this; > >> > >> @@ -57,11 +60,67 @@ Ext.define('Proxmox.node.NetworkEdit', { > >> } > >> > >> if (me.iftype === 'bridge') { > >> + let vids = Ext.create('Ext.form.field.Text', { > >> + fieldLabel: gettext('Bridge VIDS'), > >> + name: 'bridge_vids', > >> + emptyText: '2-4094', > >> + disabled: true, > >> + autoEl: { > >> + tag: 'div', > >> + 'data-qtip': gettext('Space-separated list of VLANs and ranges, for example: 2 4 100-200'), > >> + }, > >> + validator: function(value) { > >> + if (!value) { return true; } // Empty > > > > nit: our js style guide state that single line ifs should be avoided > > > >> + let result = true; > >> + > >> + let vid_list = value.split(' '); > >> + > >> + let checkVid = function(tag) { > >> + if (tag < 2 || tag > 4094) { > >> + return `not a valid VLAN ID '${tag}'`; > >> + } > >> + return true; > >> + }; > >> + > >> + for (const vid of vid_list) { > >> + if (!vid) { > >> + continue; > >> + } > >> + let res = vid.match(/^(\d+)([-](\d+))?$/i); > > > > nit: see my comment for patch 2. > > > >> + if (!res) { > >> + return `not a valid VLAN configuration '${vid}'`; > >> + } > >> + let start = Number(res[1]); > >> + let end = Number(res[3]); > >> + > >> + if (start) { > >> + result = checkVid(start); > >> + if (result !== true) { return result; } > > > > same here... > > > >> + } > >> + if (end) { > >> + result = checkVid(end); > >> + if (result !== true) { return result; } > > > > and here. it might be more elegant to do this: > > > > let invalidVid = function(tag) { > > if (!isNaN(tag) && (tag < 2 || tag > 4094)) { > > return `not a valid VLAN ID '${tag}'`; > > } > > > > return false; > > }; > > > > // [..] > > > > if (res = invalidVid(start)) { > > return res; > > } > > > > if (res = invalidVid(end)) { > > return res; > > } > > > > `Number(res[n])` should return `NaN` if `res[n]` is undefined, this > > shouldn't be the case for start ever but is relevant for `end`. > > > > Thanks for the overall review and these suggestions! > > The above example won't work exactly like that, as eslint will throw an > error due to assignments inside the if-clause. > > I opted for > res = invalidVid(start); > if (res) { > return res; > } > > Not quite as succinct, but it does the same job and adheres to our > eslint rules. a fair enough, yeah didn't check with eslint, sorry my bad _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel