From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id F2EE472EA4 for ; Mon, 5 Jul 2021 08:52:01 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E1D271B34B for ; Mon, 5 Jul 2021 08:51:31 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 567CD1B33D for ; Mon, 5 Jul 2021 08:51:31 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 251F94086E for ; Mon, 5 Jul 2021 08:51:31 +0200 (CEST) Message-ID: <43eb1d71-4768-d8d6-805c-42ae83f7c9e6@proxmox.com> Date: Mon, 5 Jul 2021 08:51:14 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:90.0) Gecko/20100101 Thunderbird/90.0 Content-Language: en-US To: Proxmox VE development discussion , Lorenz Stechauner References: <20210701085007.2811779-1-l.stechauner@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210701085007.2811779-1-l.stechauner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.530 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% 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: [pve-devel] applied: [PATCH v11 manager 0/3] fix#1710: add download from url button 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: , X-List-Received-Date: Mon, 05 Jul 2021 06:52:02 -0000 On 01.07.21 10:50, Lorenz Stechauner wrote: > changes to v10: > * dropped already applied patches > * added "check" button - the gui now does not automatically send the metadata request anymore > * removed (visible) content type selector, because there was only one hard-coded option every time > * added loading mask while the metadata check is in progress > > Lorenz Stechauner (3): > api: nodes: add query_url_metadata method > ui: Utils: change download task format > fix #1710: ui: storage: add download from url button > > PVE/API2/Nodes.pm | 96 ++++++++++ > www/manager6/Utils.js | 2 +- > www/manager6/storage/Browser.js | 8 + > www/manager6/storage/ContentView.js | 262 +++++++++++++++++++++++++--- > 4 files changed, 343 insertions(+), 25 deletions(-) > applied series, thanks! Did some followups though: - I moved out the download window into its own file, was a bit out of place in the storage content view and adding 200+ lines code is just better down in a separate file, if not really general code to the existing content view components - I found the event logic a bit hard to grasp, reworked it with a view model - Renamed `Check` to `Query URL`, as when trying to think as user I had no idea what `Check` would actually do. - disabled the `Query URL` button when done so successfully, any change in URL or verify state re-enables it, it stays also enabled if the query itself fails, as that could have been a (network) hiccup, where allowing one to requery makes sense from UX POV. - do not invalidate size/mime on verify cert check toggle, makes no sense to me, the last OK queried info will still be valid. Above could have been noticed earlier on review, sorry for that, but as I was pretty swamped to take a closer look and there are others which can review such things I'm so free to not take the whole blame ;-) In general I wonder if we could do the metadata query also client side, we could just do a HEAD request from there which would be much nicer, but there are also cases where an admin may enter an internal URL or the DNS the browsers queries is another than the PVE hosts have, so while I think it would cover most of the actual cases in practice (as even with internal URLs, the admin often access PVE also from an internal network), it's still not always doable. On the other hand, it would drop any remaining concern about doing some http request from the PVE side. just noting for the record, we can always switch to that in the future and sunset or further-restrict the backend querier if we want...