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 E48811FF173 for ; Mon, 29 Jul 2024 12:25:41 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id ACCE31AED0; Mon, 29 Jul 2024 12:25:41 +0200 (CEST) Message-ID: Date: Mon, 29 Jul 2024 12:25:06 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Shannon Sterz References: <20240703080147.81154-1-a.lauterer@proxmox.com> <20240703080147.81154-5-a.lauterer@proxmox.com> Content-Language: en-US From: Aaron Lauterer In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL -0.038 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel