* [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs @ 2023-09-21 13:09 Philipp Hufnagl 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression Philipp Hufnagl ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Philipp Hufnagl @ 2023-09-21 13:09 UTC (permalink / raw) To: pve-devel Many web pages offer the download of the disk images compressed. This patch allows the download of archives (like .gz), automatically detects the format and decompresses it Changes since v7: * added detect-compression API parameter again Changes since v6: * remove detect-compression API parameter * add compression field with cbind Changes since v5: * split manager patch into frontend/backend * remove not needed regex group Changes since v4: * add commit messages * fix nit Changes since v3: * generate compression regex from compression list * fix logic errors Changes since v2: * move compression code to the download function in common * minor code improvements Changes since v1: * Improve code quality as suggested by feedbackfix #4849: allow download of compressed ISOs Many web pages offer the download of the disk images compressed. This patch allows the download of archives (like .gz), automatically detects the format and decompresses it Changes since v7: * added detect-compression API parameter again Changes since v6: * remove detect-compression API parameter * add compression field with cbind Changes since v5: * split manager patch into frontend/backend * remove not needed regex group Changes since v4: * add commit messages * fix nit Changes since v3: * generate compression regex from compression list * fix logic errors Changes since v2: * move compression code to the download function in common * minor code improvements Changes since v1: * Improve code quality as suggested by feedback Philipp Hufnagl (2): fix #4849: api: download to storage: automatically dectect and configure compression fix #4849: ui: download to storage: automatically dectect and configure compression PVE/API2/Nodes.pm | 22 ++++++++++++++++++++- www/manager6/Makefile | 1 + www/manager6/form/DecompressionSelector.js | 13 ++++++++++++ www/manager6/window/DownloadUrlToStorage.js | 14 ++++++++++++- 4 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 www/manager6/form/DecompressionSelector.js -- 2.39.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-21 13:09 [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs Philipp Hufnagl @ 2023-09-21 13:09 ` Philipp Hufnagl 2023-09-26 10:56 ` Thomas Lamprecht 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 2/2] fix #4849: ui: " Philipp Hufnagl ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Philipp Hufnagl @ 2023-09-21 13:09 UTC (permalink / raw) To: pve-devel extends the query_url_metadata callback with the functionallity to detect used compressions. If a compression is used it tells the ui which one Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com> --- PVE/API2/Nodes.pm | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm index 5a148d1d..1e8ed09e 100644 --- a/PVE/API2/Nodes.pm +++ b/PVE/API2/Nodes.pm @@ -34,6 +34,7 @@ use PVE::RRD; use PVE::Report; use PVE::SafeSyslog; use PVE::Storage; +use PVE::Storage::Plugin; use PVE::Tools; use PVE::pvecfg; @@ -1564,7 +1565,13 @@ __PACKAGE__->register_method({ type => 'boolean', optional => 1, default => 1, - } + }, + 'detect-compression' => { + description => "If true an auto detection of used compression will be attempted", + type => 'boolean', + optional => 1, + default => 0, + }, }, }, returns => { @@ -1583,6 +1590,11 @@ __PACKAGE__->register_method({ type => 'string', optional => 1, }, + compression => { + type => 'string', + enum => $PVE::Storage::Plugin::KNOWN_COMPRESSION_FORMATS, + optional => 1, + }, }, }, code => sub { @@ -1605,6 +1617,7 @@ __PACKAGE__->register_method({ SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE, ); } + my $detect_compression = $param->{'detect-compression'} // 0; my $req = HTTP::Request->new(HEAD => $url); my $res = $ua->request($req); @@ -1615,6 +1628,7 @@ __PACKAGE__->register_method({ my $disposition = $res->header("Content-Disposition"); my $type = $res->header("Content-Type"); + my $compression; my $filename; if ($disposition && ($disposition =~ m/filename="([^"]*)"/ || $disposition =~ m/filename=([^;]*)/)) { @@ -1628,10 +1642,16 @@ __PACKAGE__->register_method({ $type = $1; } + if ($detect_compression && $filename =~ m!^(.+)\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})$!) { + $filename = $1; + $compression = $2; + } + my $ret = {}; $ret->{filename} = $filename if $filename; $ret->{size} = $size + 0 if $size; $ret->{mimetype} = $type if $type; + $ret->{compression} = $compression if $compression; return $ret; }}); -- 2.39.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression Philipp Hufnagl @ 2023-09-26 10:56 ` Thomas Lamprecht 2023-09-26 12:25 ` Philipp Hufnagl 2023-09-27 8:03 ` Philipp Hufnagl 0 siblings, 2 replies; 16+ messages in thread From: Thomas Lamprecht @ 2023-09-26 10:56 UTC (permalink / raw) To: Proxmox VE development discussion, Philipp Hufnagl, Fabian Grünbichler while this is already applied, some comments inline, for a possible next time, and also the big question if this is even required, after all I can just check the few compression algorithms easily in the frontend, i.e., offloading a simple string regex match to the backend seems rather odd to me.. Am 21/09/2023 um 15:09 schrieb Philipp Hufnagl: > extends the query_url_metadata callback with the functionallity to s/functionallity/functionality/ > detect used compressions. If a compression is used it tells the ui which > one > > Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com> > --- > PVE/API2/Nodes.pm | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm > index 5a148d1d..1e8ed09e 100644 > --- a/PVE/API2/Nodes.pm > +++ b/PVE/API2/Nodes.pm > @@ -34,6 +34,7 @@ use PVE::RRD; > use PVE::Report; > use PVE::SafeSyslog; > use PVE::Storage; > +use PVE::Storage::Plugin; > use PVE::Tools; > use PVE::pvecfg; > > @@ -1564,7 +1565,13 @@ __PACKAGE__->register_method({ > type => 'boolean', > optional => 1, > default => 1, > - } > + }, > + 'detect-compression' => { > + description => "If true an auto detection of used compression will be attempted", Grammatically and semantically slightly better would be something like: "If true, the queried filename is checked for a compression extension." > + type => 'boolean', > + optional => 1, > + default => 0, > + }, > }, > }, > returns => { > @@ -1583,6 +1590,11 @@ __PACKAGE__->register_method({ > type => 'string', > optional => 1, > }, > + compression => { > + type => 'string', > + enum => $PVE::Storage::Plugin::KNOWN_COMPRESSION_FORMATS,> + optional => 1, > + }, > }, > }, > code => sub { > @@ -1605,6 +1617,7 @@ __PACKAGE__->register_method({ > SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE, > ); > } > + my $detect_compression = $param->{'detect-compression'} // 0; It's often best to avoid such intermediate variables if there's just a single use case and on doesn't needs to "jump through hoops" to get to the value – e.g., a simple hash access like here if 100% fine, if the value would to be pulled out of some deeply nested structure, or assembled or the like, then it might have its merit to use an intermediate variable. > > my $req = HTTP::Request->new(HEAD => $url); > my $res = $ua->request($req); > @@ -1615,6 +1628,7 @@ __PACKAGE__->register_method({ > my $disposition = $res->header("Content-Disposition"); > my $type = $res->header("Content-Type"); > > + my $compression; Keep definition and use closer together (I'd moved this down directly above the if t that sets it) > my $filename; > > if ($disposition && ($disposition =~ m/filename="([^"]*)"/ || $disposition =~ m/filename=([^;]*)/)) { > @@ -1628,10 +1642,16 @@ __PACKAGE__->register_method({ > $type = $1; > } > > + if ($detect_compression && $filename =~ m!^(.+)\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})$!) { There are code paths where $filename is not yet defined here, resulting in a rather ugly warning – so this needs upfront checking too – always check where the value code path is coming in (yeah, Rust would do that for you, but most API endpoints are small enough to be able to do so quickly also manually) > + $filename = $1; > + $compression = $2; > + } > + > my $ret = {}; > $ret->{filename} = $filename if $filename; > $ret->{size} = $size + 0 if $size; > $ret->{mimetype} = $type if $type; > + $ret->{compression} = $compression if $compression; > > return $ret; > }}); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-26 10:56 ` Thomas Lamprecht @ 2023-09-26 12:25 ` Philipp Hufnagl 2023-09-26 14:23 ` Thomas Lamprecht 2023-09-27 8:03 ` Philipp Hufnagl 1 sibling, 1 reply; 16+ messages in thread From: Philipp Hufnagl @ 2023-09-26 12:25 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion, Fabian Grünbichler On 9/26/23 12:56, Thomas Lamprecht wrote: > while this is already applied, some comments inline, for a possible next > time, and also the big > question if this is even required, after all I can just check the few > compression algorithms easily in the frontend, i.e., offloading a simple > string regex match to the backend seems rather odd to me.. The problem with that is that the point where the iso is stored might not be accessible for the client. If it is done by the PVE, it might resolve the url differently. > > Am 21/09/2023 um 15:09 schrieb Philipp Hufnagl: >> extends the query_url_metadata callback with the functionallity to > > s/functionallity/functionality/ > >> detect used compressions. If a compression is used it tells the ui which >> one >> >> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com> >> --- >> PVE/API2/Nodes.pm | 22 +++++++++++++++++++++- >> 1 file changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm >> index 5a148d1d..1e8ed09e 100644 >> --- a/PVE/API2/Nodes.pm >> +++ b/PVE/API2/Nodes.pm >> @@ -34,6 +34,7 @@ use PVE::RRD; >> use PVE::Report; >> use PVE::SafeSyslog; >> use PVE::Storage; >> +use PVE::Storage::Plugin; >> use PVE::Tools; >> use PVE::pvecfg; >> >> @@ -1564,7 +1565,13 @@ __PACKAGE__->register_method({ >> type => 'boolean', >> optional => 1, >> default => 1, >> - } >> + }, >> + 'detect-compression' => { >> + description => "If true an auto detection of used compression will be attempted", > > Grammatically and semantically slightly better would be something like: > "If true, the queried filename is checked for a compression extension." > > > >> + type => 'boolean', >> + optional => 1, >> + default => 0, >> + }, >> }, >> }, >> returns => { >> @@ -1583,6 +1590,11 @@ __PACKAGE__->register_method({ >> type => 'string', >> optional => 1, >> }, >> + compression => { >> + type => 'string', >> + enum => $PVE::Storage::Plugin::KNOWN_COMPRESSION_FORMATS,> + optional => 1, >> + }, >> }, >> }, >> code => sub { >> @@ -1605,6 +1617,7 @@ __PACKAGE__->register_method({ >> SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE, >> ); >> } >> + my $detect_compression = $param->{'detect-compression'} // 0; > > It's often best to avoid such intermediate variables if there's just a > single use case and on doesn't needs to "jump through hoops" to get > to the value – e.g., a simple hash access like here if 100% fine, if > the value would to be pulled out of some deeply nested structure, or > assembled or the like, then it might have its merit to use an > intermediate variable. > >> >> my $req = HTTP::Request->new(HEAD => $url); >> my $res = $ua->request($req); >> @@ -1615,6 +1628,7 @@ __PACKAGE__->register_method({ >> my $disposition = $res->header("Content-Disposition"); >> my $type = $res->header("Content-Type"); >> >> + my $compression; > > Keep definition and use closer together (I'd moved this down directly above the if t that sets it) > >> my $filename; >> >> if ($disposition && ($disposition =~ m/filename="([^"]*)"/ || $disposition =~ m/filename=([^;]*)/)) { >> @@ -1628,10 +1642,16 @@ __PACKAGE__->register_method({ >> $type = $1; >> } >> >> + if ($detect_compression && $filename =~ m!^(.+)\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})$!) { > > There are code paths where $filename is not yet defined here, resulting > in a rather ugly warning – so this needs upfront checking too – always > check where the value code path is coming in (yeah, Rust would do that for > you, but most API endpoints are small enough to be able to do so quickly also manually) > >> + $filename = $1; >> + $compression = $2; >> + } >> + >> my $ret = {}; >> $ret->{filename} = $filename if $filename; >> $ret->{size} = $size + 0 if $size; >> $ret->{mimetype} = $type if $type; >> + $ret->{compression} = $compression if $compression; >> >> return $ret; >> }}); > Sorry for the small issues. This was my first larger feature. I will try to improve next time! ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-26 12:25 ` Philipp Hufnagl @ 2023-09-26 14:23 ` Thomas Lamprecht 2023-09-26 14:54 ` Philipp Hufnagl 0 siblings, 1 reply; 16+ messages in thread From: Thomas Lamprecht @ 2023-09-26 14:23 UTC (permalink / raw) To: Philipp Hufnagl, Proxmox VE development discussion, Fabian Grünbichler Am 26/09/2023 um 14:25 schrieb Philipp Hufnagl: > On 9/26/23 12:56, Thomas Lamprecht wrote: >> while this is already applied, some comments inline, for a possible next >> time, and also the big >> question if this is even required, after all I can just check the few >> compression algorithms easily in the frontend, i.e., offloading a simple >> string regex match to the backend seems rather odd to me.. > The problem with that is that the point where the iso is stored might > not be accessible for the client. If it is done by the PVE, it might > resolve the url differently. I'm not sure if I understand, I thought that's why we made the link metadata- query API in the first place (which I obv. do not want to drop in general)? As we got the correct (from the PVE node's POV) resolved filename returned by the metadata query API, so we can just do the regex string match for detecting a possible compression file extension on that in the frontend after that API call returns. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-26 14:23 ` Thomas Lamprecht @ 2023-09-26 14:54 ` Philipp Hufnagl 2023-09-26 14:57 ` Thomas Lamprecht 0 siblings, 1 reply; 16+ messages in thread From: Philipp Hufnagl @ 2023-09-26 14:54 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion, Fabian Grünbichler On 9/26/23 16:23, Thomas Lamprecht wrote: > Am 26/09/2023 um 14:25 schrieb Philipp Hufnagl: >> On 9/26/23 12:56, Thomas Lamprecht wrote: >>> while this is already applied, some comments inline, for a possible next >>> time, and also the big >>> question if this is even required, after all I can just check the few >>> compression algorithms easily in the frontend, i.e., offloading a simple >>> string regex match to the backend seems rather odd to me.. >> The problem with that is that the point where the iso is stored might >> not be accessible for the client. If it is done by the PVE, it might >> resolve the url differently. > > I'm not sure if I understand, I thought that's why we made the link > metadata- query API in the first place (which I obv. do not want to drop > in general)? > > As we got the correct (from the PVE node's POV) resolved filename > returned by the metadata query API, so we can just do the regex string > match for detecting a possible compression file extension on that in the > frontend after that API call returns. > Yes that would have been possible, however it would not have saved an API call since the call is needed anyway. I did it there because I considered it a cleaner solution to do all handling of metadata in one place rather then returning a "filename" that has to be further processed in "filename" and "compression". ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-26 14:54 ` Philipp Hufnagl @ 2023-09-26 14:57 ` Thomas Lamprecht 0 siblings, 0 replies; 16+ messages in thread From: Thomas Lamprecht @ 2023-09-26 14:57 UTC (permalink / raw) To: Philipp Hufnagl, Proxmox VE development discussion, Fabian Grünbichler Am 26/09/2023 um 16:54 schrieb Philipp Hufnagl: > On 9/26/23 16:23, Thomas Lamprecht wrote: >> Am 26/09/2023 um 14:25 schrieb Philipp Hufnagl: >>> On 9/26/23 12:56, Thomas Lamprecht wrote: >>>> while this is already applied, some comments inline, for a possible next >>>> time, and also the big >>>> question if this is even required, after all I can just check the few >>>> compression algorithms easily in the frontend, i.e., offloading a simple >>>> string regex match to the backend seems rather odd to me.. >>> The problem with that is that the point where the iso is stored might >>> not be accessible for the client. If it is done by the PVE, it might >>> resolve the url differently. >> >> I'm not sure if I understand, I thought that's why we made the link >> metadata- query API in the first place (which I obv. do not want to drop >> in general)? >> >> As we got the correct (from the PVE node's POV) resolved filename >> returned by the metadata query API, so we can just do the regex string >> match for detecting a possible compression file extension on that in the >> frontend after that API call returns. >> > > Yes that would have been possible, however it would not have saved an > API call since the call is needed anyway. I did it there because I > considered it a cleaner solution to do all handling of metadata in one > place rather then returning a "filename" that has to be further > processed in "filename" and "compression". I never implied that it would save an API call, just that it would avoid adding unecesarry parameters and return values, bloating up our API schema for a trivial string match that any client can do themselves.. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-26 10:56 ` Thomas Lamprecht 2023-09-26 12:25 ` Philipp Hufnagl @ 2023-09-27 8:03 ` Philipp Hufnagl 2023-09-27 8:33 ` Thomas Lamprecht 1 sibling, 1 reply; 16+ messages in thread From: Philipp Hufnagl @ 2023-09-27 8:03 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion, Fabian Grünbichler > There are code paths where $filename is not yet defined here, resulting > in a rather ugly warning – so this needs upfront checking too – always > check where the value code path is coming in (yeah, Rust would do that for > you, but most API endpoints are small enough to be able to do so quickly also manually) I will make a new patch resolving this issue. While I am on it, I see what I can resolve on the other issues. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-27 8:03 ` Philipp Hufnagl @ 2023-09-27 8:33 ` Thomas Lamprecht 2023-09-27 8:57 ` Philipp Hufnagl 0 siblings, 1 reply; 16+ messages in thread From: Thomas Lamprecht @ 2023-09-27 8:33 UTC (permalink / raw) To: Philipp Hufnagl, Proxmox VE development discussion, Fabian Grünbichler Am 27/09/2023 um 10:03 schrieb Philipp Hufnagl: >> There are code paths where $filename is not yet defined here, resulting >> in a rather ugly warning – so this needs upfront checking too – always >> check where the value code path is coming in (yeah, Rust would do that for >> you, but most API endpoints are small enough to be able to do so quickly also manually) > > I will make a new patch resolving this issue. While I am on it, I see > what I can resolve on the other issues. FYI, I have a pretty much done patch for this already here, so if you did not have anything (close to) already finished, I could push that out – just mentioning to avoid potential duplicate work. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-27 8:33 ` Thomas Lamprecht @ 2023-09-27 8:57 ` Philipp Hufnagl 2023-09-27 17:19 ` Thomas Lamprecht 0 siblings, 1 reply; 16+ messages in thread From: Philipp Hufnagl @ 2023-09-27 8:57 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion, Fabian Grünbichler On 9/27/23 10:33, Thomas Lamprecht wrote: > Am 27/09/2023 um 10:03 schrieb Philipp Hufnagl: >>> There are code paths where $filename is not yet defined here, resulting >>> in a rather ugly warning – so this needs upfront checking too – always >>> check where the value code path is coming in (yeah, Rust would do that for >>> you, but most API endpoints are small enough to be able to do so quickly also manually) >> >> I will make a new patch resolving this issue. While I am on it, I see >> what I can resolve on the other issues. > > FYI, I have a pretty much done patch for this already here, so if > you did not have anything (close to) already finished, I could push > that out – just mentioning to avoid potential duplicate work. I have nothing close. Sorry for the inconvenience and thank you for fixing. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 2023-09-27 8:57 ` Philipp Hufnagl @ 2023-09-27 17:19 ` Thomas Lamprecht 0 siblings, 0 replies; 16+ messages in thread From: Thomas Lamprecht @ 2023-09-27 17:19 UTC (permalink / raw) To: Proxmox VE development discussion, Philipp Hufnagl, Fabian Grünbichler Am 27/09/2023 um 10:57 schrieb Philipp Hufnagl: > On 9/27/23 10:33, Thomas Lamprecht wrote: >> FYI, I have a pretty much done patch for this already here, so if >> you did not have anything (close to) already finished, I could push >> that out – just mentioning to avoid potential duplicate work. > > > I have nothing close. Sorry for the inconvenience and thank you for > fixing. OK, I now applied below change for doing this in the frontend and reverted the API change. FWIW, this is not 100% semantically the same, as I now try to match the file-extension with the case-insensitive flag to allow matching a, e.g., ISO.GZ too. ----8<---- diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js index 481cb2ed..335d6aa6 100644 --- a/www/manager6/window/DownloadUrlToStorage.js +++ b/www/manager6/window/DownloadUrlToStorage.js @@ -66,7 +66,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', { params: { url: queryParam.url, 'verify-certificates': queryParam['verify-certificates'], - 'detect-compression': view.content === 'iso' ? 1 : 0, }, waitMsgTarget: view, failure: res => { @@ -81,11 +80,22 @@ Ext.define('PVE.window.DownloadUrlToStorage', { urlField.validate(); let data = res.result.data; + + let filename = data.filename || ""; + let compression = '__default__'; + if (view.content === 'iso') { + const matches = filename.match(/^(.+)\.(gz|lzo|zst)$/i); + if (matches) { + filename = matches[1]; + compression = matches[2]; + } + } + view.setValues({ - filename: data.filename || "", + filename, + compression, size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"), mimetype: data.mimetype || gettext("Unknown"), - compression: data.compression || '__default__', }); }, }); ^ permalink raw reply [flat|nested] 16+ messages in thread
* [pve-devel] [PATCH manager v8 2/2] fix #4849: ui: download to storage: automatically dectect and configure compression 2023-09-21 13:09 [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs Philipp Hufnagl 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression Philipp Hufnagl @ 2023-09-21 13:09 ` Philipp Hufnagl 2023-09-26 10:59 ` Thomas Lamprecht 2023-09-22 14:02 ` [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs Dominik Csapak 2023-09-26 7:39 ` [pve-devel] applied-series: [PATCH manager v9 " Fabian Grünbichler 3 siblings, 1 reply; 16+ messages in thread From: Philipp Hufnagl @ 2023-09-21 13:09 UTC (permalink / raw) To: pve-devel extends the download iso prompt with a "compression algorithm" drop down under advanced. User can configure there if a decompression algorithm should be used from the storage backend. The compression algorithm will be automatically guessed when calling query_url_metadata Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com> --- www/manager6/Makefile | 1 + www/manager6/form/DecompressionSelector.js | 13 +++++++++++++ www/manager6/window/DownloadUrlToStorage.js | 14 +++++++++++++- 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 www/manager6/form/DecompressionSelector.js diff --git a/www/manager6/Makefile b/www/manager6/Makefile index 59a5d8a7..87e66ece 100644 --- a/www/manager6/Makefile +++ b/www/manager6/Makefile @@ -34,6 +34,7 @@ JSSRC= \ form/ContentTypeSelector.js \ form/ControllerSelector.js \ form/DayOfWeekSelector.js \ + form/DecompressionSelector.js \ form/DiskFormatSelector.js \ form/DiskStorageSelector.js \ form/FileSelector.js \ diff --git a/www/manager6/form/DecompressionSelector.js b/www/manager6/form/DecompressionSelector.js new file mode 100644 index 00000000..abd19316 --- /dev/null +++ b/www/manager6/form/DecompressionSelector.js @@ -0,0 +1,13 @@ +Ext.define('PVE.form.DecompressionSelector', { + extend: 'Proxmox.form.KVComboBox', + alias: ['widget.pveDecompressionSelector'], + config: { + deleteEmpty: false, + }, + comboItems: [ + ['__default__', Proxmox.Utils.NoneText], + ['lzo', 'LZO'], + ['gz', 'GZIP'], + ['zst', 'ZSTD'], + ], +}); diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js index 90320da4..36ad13fa 100644 --- a/www/manager6/window/DownloadUrlToStorage.js +++ b/www/manager6/window/DownloadUrlToStorage.js @@ -66,6 +66,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', { params: { url: queryParam.url, 'verify-certificates': queryParam['verify-certificates'], + 'detect-compression': view.content === 'iso' ? 1 : 0, }, waitMsgTarget: view, failure: res => { @@ -84,6 +85,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', { filename: data.filename || "", size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"), mimetype: data.mimetype || gettext("Unknown"), + compression: data.compression || '__default__', }); }, }); @@ -203,6 +205,17 @@ Ext.define('PVE.window.DownloadUrlToStorage', { change: 'setQueryEnabled', }, }, + { + xtype: 'pveDecompressionSelector', + name: 'compression', + fieldLabel: gettext('Decompression algorithm'), + allowBlank: true, + hasNoneOption: true, + value: '__default__', + cbind: { + hidden: get => get('content') !== 'iso', + }, + }, ], }, { @@ -223,7 +236,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', { if (!me.storage) { throw "no storage ID specified"; } - me.callParent(); }, }); -- 2.39.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 2/2] fix #4849: ui: download to storage: automatically dectect and configure compression 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 2/2] fix #4849: ui: " Philipp Hufnagl @ 2023-09-26 10:59 ` Thomas Lamprecht 2023-09-26 12:27 ` Philipp Hufnagl 0 siblings, 1 reply; 16+ messages in thread From: Thomas Lamprecht @ 2023-09-26 10:59 UTC (permalink / raw) To: Proxmox VE development discussion, Philipp Hufnagl Am 21/09/2023 um 15:09 schrieb Philipp Hufnagl: > extends the download iso prompt with a "compression algorithm" drop down > under advanced. User can configure there if a decompression algorithm > should be used from the storage backend. The compression algorithm will > be automatically guessed when calling query_url_metadata > > Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com> > --- > www/manager6/Makefile | 1 + > www/manager6/form/DecompressionSelector.js | 13 +++++++++++++ > www/manager6/window/DownloadUrlToStorage.js | 14 +++++++++++++- > 3 files changed, 27 insertions(+), 1 deletion(-) > create mode 100644 www/manager6/form/DecompressionSelector.js > > diff --git a/www/manager6/Makefile b/www/manager6/Makefile > index 59a5d8a7..87e66ece 100644 > --- a/www/manager6/Makefile > +++ b/www/manager6/Makefile > @@ -34,6 +34,7 @@ JSSRC= \ > form/ContentTypeSelector.js \ > form/ControllerSelector.js \ > form/DayOfWeekSelector.js \ > + form/DecompressionSelector.js \ > form/DiskFormatSelector.js \ > form/DiskStorageSelector.js \ > form/FileSelector.js \ > diff --git a/www/manager6/form/DecompressionSelector.js b/www/manager6/form/DecompressionSelector.js > new file mode 100644 > index 00000000..abd19316 > --- /dev/null > +++ b/www/manager6/form/DecompressionSelector.js > @@ -0,0 +1,13 @@ > +Ext.define('PVE.form.DecompressionSelector', { > + extend: 'Proxmox.form.KVComboBox', > + alias: ['widget.pveDecompressionSelector'], > + config: { > + deleteEmpty: false, > + }, > + comboItems: [ > + ['__default__', Proxmox.Utils.NoneText], > + ['lzo', 'LZO'], > + ['gz', 'GZIP'], > + ['zst', 'ZSTD'], > + ], > +}); For such small things, that have no other use sites, I normally prefer to just define the values inline.. And finding other use sites for such things is not trivial, as that would mean we will couple the values that both sites can understand, which for compression might not be the case (e.g., in the future we might want to support downloading bz2 here, but not want to support it for backups). > diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js > index 90320da4..36ad13fa 100644 > --- a/www/manager6/window/DownloadUrlToStorage.js > +++ b/www/manager6/window/DownloadUrlToStorage.js > @@ -66,6 +66,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', { > params: { > url: queryParam.url, > 'verify-certificates': queryParam['verify-certificates'], > + 'detect-compression': view.content === 'iso' ? 1 : 0, > }, > waitMsgTarget: view, > failure: res => { > @@ -84,6 +85,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', { > filename: data.filename || "", > size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"), > mimetype: data.mimetype || gettext("Unknown"), > + compression: data.compression || '__default__', > }); > }, > }); > @@ -203,6 +205,17 @@ Ext.define('PVE.window.DownloadUrlToStorage', { > change: 'setQueryEnabled', > }, > }, > + { > + xtype: 'pveDecompressionSelector', > + name: 'compression', > + fieldLabel: gettext('Decompression algorithm'), > + allowBlank: true, > + hasNoneOption: true, > + value: '__default__', > + cbind: { > + hidden: get => get('content') !== 'iso', > + }, > + }, > ], > }, > { > @@ -223,7 +236,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', { > if (!me.storage) { > throw "no storage ID specified"; > } > - please avoid unrelated change in the future. > me.callParent(); > }, > }); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 2/2] fix #4849: ui: download to storage: automatically dectect and configure compression 2023-09-26 10:59 ` Thomas Lamprecht @ 2023-09-26 12:27 ` Philipp Hufnagl 0 siblings, 0 replies; 16+ messages in thread From: Philipp Hufnagl @ 2023-09-26 12:27 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion On 9/26/23 12:59, Thomas Lamprecht wrote: > Am 21/09/2023 um 15:09 schrieb Philipp Hufnagl: >> extends the download iso prompt with a "compression algorithm" drop down >> under advanced. User can configure there if a decompression algorithm >> should be used from the storage backend. The compression algorithm will >> be automatically guessed when calling query_url_metadata >> >> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com> >> --- >> www/manager6/Makefile | 1 + >> www/manager6/form/DecompressionSelector.js | 13 +++++++++++++ >> www/manager6/window/DownloadUrlToStorage.js | 14 +++++++++++++- >> 3 files changed, 27 insertions(+), 1 deletion(-) >> create mode 100644 www/manager6/form/DecompressionSelector.js >> >> diff --git a/www/manager6/Makefile b/www/manager6/Makefile >> index 59a5d8a7..87e66ece 100644 >> --- a/www/manager6/Makefile >> +++ b/www/manager6/Makefile >> @@ -34,6 +34,7 @@ JSSRC= \ >> form/ContentTypeSelector.js \ >> form/ControllerSelector.js \ >> form/DayOfWeekSelector.js \ >> + form/DecompressionSelector.js \ >> form/DiskFormatSelector.js \ >> form/DiskStorageSelector.js \ >> form/FileSelector.js \ >> diff --git a/www/manager6/form/DecompressionSelector.js b/www/manager6/form/DecompressionSelector.js >> new file mode 100644 >> index 00000000..abd19316 >> --- /dev/null >> +++ b/www/manager6/form/DecompressionSelector.js >> @@ -0,0 +1,13 @@ >> +Ext.define('PVE.form.DecompressionSelector', { >> + extend: 'Proxmox.form.KVComboBox', >> + alias: ['widget.pveDecompressionSelector'], >> + config: { >> + deleteEmpty: false, >> + }, >> + comboItems: [ >> + ['__default__', Proxmox.Utils.NoneText], >> + ['lzo', 'LZO'], >> + ['gz', 'GZIP'], >> + ['zst', 'ZSTD'], >> + ], >> +}); > > For such small things, that have no other use sites, I normally > prefer to just define the values inline.. > > And finding other use sites for such things is not trivial, as that > would mean we will couple the values that both sites can understand, > which for compression might not be the case (e.g., in the future we > might want to support downloading bz2 here, but not want to support > it for backups). > >> diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js >> index 90320da4..36ad13fa 100644 >> --- a/www/manager6/window/DownloadUrlToStorage.js >> +++ b/www/manager6/window/DownloadUrlToStorage.js >> @@ -66,6 +66,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', { >> params: { >> url: queryParam.url, >> 'verify-certificates': queryParam['verify-certificates'], >> + 'detect-compression': view.content === 'iso' ? 1 : 0, >> }, >> waitMsgTarget: view, >> failure: res => { >> @@ -84,6 +85,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', { >> filename: data.filename || "", >> size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"), >> mimetype: data.mimetype || gettext("Unknown"), >> + compression: data.compression || '__default__', >> }); >> }, >> }); >> @@ -203,6 +205,17 @@ Ext.define('PVE.window.DownloadUrlToStorage', { >> change: 'setQueryEnabled', >> }, >> }, >> + { >> + xtype: 'pveDecompressionSelector', >> + name: 'compression', >> + fieldLabel: gettext('Decompression algorithm'), >> + allowBlank: true, >> + hasNoneOption: true, >> + value: '__default__', >> + cbind: { >> + hidden: get => get('content') !== 'iso', >> + }, >> + }, >> ], >> }, >> { >> @@ -223,7 +236,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', { >> if (!me.storage) { >> throw "no storage ID specified"; >> } >> - > > please avoid unrelated change in the future. > >> me.callParent(); >> }, >> }); > Sorry for the issues. This was my first larger feature. I will try to improve next time! ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs 2023-09-21 13:09 [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs Philipp Hufnagl 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression Philipp Hufnagl 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 2/2] fix #4849: ui: " Philipp Hufnagl @ 2023-09-22 14:02 ` Dominik Csapak 2023-09-26 7:39 ` [pve-devel] applied-series: [PATCH manager v9 " Fabian Grünbichler 3 siblings, 0 replies; 16+ messages in thread From: Dominik Csapak @ 2023-09-22 14:02 UTC (permalink / raw) To: Proxmox VE development discussion, Philipp Hufnagl LGTM now, and tested fine (this time with ISOs *and* CT templates ;) ) two tiny nits: the commit message of the first message could mention that the parameter is necessary to differentiate between different needs for different content types, but imho not super important now (could be fixed up e.g.) the last hunk in the second patch seems leftover, but it only removes an empty line, so it should do any harm. Reviewed-by: Dominik Csapak <d.csapak@proxmox.com> Tested-by: Dominik Csapak <d.csapak@proxmox.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [pve-devel] applied-series: [PATCH manager v9 0/2] fix #4849: allow download of compressed ISOs 2023-09-21 13:09 [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs Philipp Hufnagl ` (2 preceding siblings ...) 2023-09-22 14:02 ` [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs Dominik Csapak @ 2023-09-26 7:39 ` Fabian Grünbichler 3 siblings, 0 replies; 16+ messages in thread From: Fabian Grünbichler @ 2023-09-26 7:39 UTC (permalink / raw) To: Proxmox VE development discussion with some commit message fixups, and d/control update folded in (and Dominik's T+R-B). On September 21, 2023 3:09 pm, Philipp Hufnagl wrote: > Many web pages offer the download of the disk images compressed. This > patch allows the download of archives (like .gz), automatically detects > the format and decompresses it > > Changes since v7: > * added detect-compression API parameter again > > Changes since v6: > * remove detect-compression API parameter > * add compression field with cbind > > Changes since v5: > * split manager patch into frontend/backend > * remove not needed regex group > > Changes since v4: > * add commit messages > * fix nit > > Changes since v3: > * generate compression regex from compression list > * fix logic errors > > Changes since v2: > * move compression code to the download function in common > * minor code improvements > > > Changes since v1: > * Improve code quality as suggested by feedbackfix #4849: allow download of compressed ISOs > > Many web pages offer the download of the disk images compressed. This > patch allows the download of archives (like .gz), automatically detects > the format and decompresses it > > Changes since v7: > * added detect-compression API parameter again > > Changes since v6: > * remove detect-compression API parameter > * add compression field with cbind > > Changes since v5: > * split manager patch into frontend/backend > * remove not needed regex group > > Changes since v4: > * add commit messages > * fix nit > > Changes since v3: > * generate compression regex from compression list > * fix logic errors > > Changes since v2: > * move compression code to the download function in common > * minor code improvements > > > Changes since v1: > * Improve code quality as suggested by feedback > Philipp Hufnagl (2): > fix #4849: api: download to storage: automatically dectect and > configure compression > fix #4849: ui: download to storage: automatically dectect and > configure compression > > PVE/API2/Nodes.pm | 22 ++++++++++++++++++++- > www/manager6/Makefile | 1 + > www/manager6/form/DecompressionSelector.js | 13 ++++++++++++ > www/manager6/window/DownloadUrlToStorage.js | 14 ++++++++++++- > 4 files changed, 48 insertions(+), 2 deletions(-) > create mode 100644 www/manager6/form/DecompressionSelector.js > > -- > 2.39.2 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-09-27 17:19 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-21 13:09 [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs Philipp Hufnagl 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression Philipp Hufnagl 2023-09-26 10:56 ` Thomas Lamprecht 2023-09-26 12:25 ` Philipp Hufnagl 2023-09-26 14:23 ` Thomas Lamprecht 2023-09-26 14:54 ` Philipp Hufnagl 2023-09-26 14:57 ` Thomas Lamprecht 2023-09-27 8:03 ` Philipp Hufnagl 2023-09-27 8:33 ` Thomas Lamprecht 2023-09-27 8:57 ` Philipp Hufnagl 2023-09-27 17:19 ` Thomas Lamprecht 2023-09-21 13:09 ` [pve-devel] [PATCH manager v8 2/2] fix #4849: ui: " Philipp Hufnagl 2023-09-26 10:59 ` Thomas Lamprecht 2023-09-26 12:27 ` Philipp Hufnagl 2023-09-22 14:02 ` [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs Dominik Csapak 2023-09-26 7:39 ` [pve-devel] applied-series: [PATCH manager v9 " Fabian Grünbichler
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox