Add proxy support in kvm #80
Conversation
- Add proxy option to "kvm upgrade" - "kvm upgrade" uses global proxy (http_proxy env var) by default - Support authenticated proxies
| @@ -1,6 +1,7 @@ | |||
| param( | |||
| [parameter(Position=0)] | |||
| [string] $command, | |||
| [string] $proxy = "", | |||
There was a problem hiding this comment.
You don't need to specify a default value here. Also, when checking you want to do
if (!$proxy) {
$proxy = $env:http_proxy
}
|
@pranavkm , thank you for the feedbacks. I just revised based on your suggestions and it looks much cleaner now 😄 |
| @@ -135,6 +156,7 @@ param( | |||
|
|
|||
| $wc = New-Object System.Net.WebClient | |||
| $wc.Credentials = new-object System.Net.NetworkCredential("aspnetreadonly", "4d8a2d9c-7b80-4162-9978-47e918c9658c") | |||
| Add-Proxy-If-Specified([ref]$wc) | |||
There was a problem hiding this comment.
Not super sure if you need the [ref] here. If it works without it, might be cleaner to remove it. Also in theory you could do
Add-Proxy-If-Specified $wc (without the round brackets) and it should work. Not particularly important though.
There was a problem hiding this comment.
Just tested and seems I don't need [ref] here. They are removed in latest commit.
I keep the round brackets for consistency with all other invocations in this file. But thank you for the hint, it's good for people new to PowerShell script like me 😄
|
|
if 
|
This isn't where this change should be made. These changes should be done here: https://github.com/aspnet/KRuntime/blob/dev/setup/kvm.ps1 The file is checked in here after a build has taken the source from the kruntime and replaced things like the build number. It is put here as a temporary convenience until there is somewhere on the inter-webs that it can live. Does that make sense? |
|
Otherwise the change looks fine :) |
|
@glennc , thank you for the explanation. I created a new PR aspnet/dnx#352. This one is closed. |
See internal issue #80 about demands evaluation
Again, see internal issue #80
parent aspnet/dnx#215