[pve-devel] [PATCH v2 manager] set https_proxy to http_proxy for querying url metadata

[pve-devel] [PATCH v2 manager] set https_proxy to http_proxy for querying url metadata

[Oguz Bektas]

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v1->v2:
* thanks dominik for catching my error!

 PVE/API2/Nodes.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index d57a1937..96479848 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -1554,7 +1554,7 @@ __PACKAGE__->register_method({
 
 	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
 	if ($dccfg->{http_proxy}) {
-	    $ua->proxy('http', $dccfg->{http_proxy});
+	    $ua->proxy(['http', 'https'], $dccfg->{http_proxy});
 	}
 
 	my $verify = $param->{'verify-certificates'} // 1;
-- 
2.30.2

Re: [pve-devel] [PATCH v2 manager] set https_proxy to http_proxy for querying url metadata

[Thomas Lamprecht]

On 25.11.21 14:48, Oguz Bektas wrote:
> Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>

where is this tag coming from? Did not see it on the list, only the comment
that it wasn't working?

> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v1->v2:
> * thanks dominik for catching my error!
> 
>  PVE/API2/Nodes.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index d57a1937..96479848 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -1554,7 +1554,7 @@ __PACKAGE__->register_method({
>  
>  	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
>  	if ($dccfg->{http_proxy}) {
> -	    $ua->proxy('http', $dccfg->{http_proxy});
> +	    $ua->proxy(['http', 'https'], $dccfg->{http_proxy});
>  	}
>  
>  	my $verify = $param->{'verify-certificates'} // 1;
> 

Re: [pve-devel] [PATCH v2 manager] set https_proxy to http_proxy for querying url metadata

[Oguz Bektas]

On Thu, Nov 25, 2021 at 03:08:24PM +0100, Thomas Lamprecht wrote:
> On 25.11.21 14:48, Oguz Bektas wrote:
> > Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
> 
> where is this tag coming from? Did not see it on the list, only the comment
> that it wasn't working?

yes its fixed now... he did review it, and its only a single line, so i
thought i'll add it before it gets forgotten along the way ;)

> 
> > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> > ---
> > v1->v2:
> > * thanks dominik for catching my error!
> > 
> >  PVE/API2/Nodes.pm | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> > index d57a1937..96479848 100644
> > --- a/PVE/API2/Nodes.pm
> > +++ b/PVE/API2/Nodes.pm
> > @@ -1554,7 +1554,7 @@ __PACKAGE__->register_method({
> >  
> >  	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
> >  	if ($dccfg->{http_proxy}) {
> > -	    $ua->proxy('http', $dccfg->{http_proxy});
> > +	    $ua->proxy(['http', 'https'], $dccfg->{http_proxy});
> >  	}
> >  
> >  	my $verify = $param->{'verify-certificates'} // 1;
> > 
> 

Re: [pve-devel] [PATCH v2 manager] set https_proxy to http_proxy for querying url metadata

[Thomas Lamprecht]

On 25.11.21 15:11, Oguz Bektas wrote:
> On Thu, Nov 25, 2021 at 03:08:24PM +0100, Thomas Lamprecht wrote:
>> On 25.11.21 14:48, Oguz Bektas wrote:
>>> Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
>>
>> where is this tag coming from? Did not see it on the list, only the comment
>> that it wasn't working?
> 
> yes its fixed now... he did review it, and its only a single line, so i
> thought i'll add it before it gets forgotten along the way ;)
> 

yeah no, that's not how it works.

first, one line or hundreds, that does not matter, especially one-liners can be
pretty bad. Secondly, the patch was completely broken, which got mentioned, that's
not akin to a R-b tag at all.. Nothing was there to get forgotten along the way.