-
Notifications
You must be signed in to change notification settings - Fork 926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added improvements to reduce errors and such #604
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @CamoCatX, nice improvements! ✨
Can you remove the google.com
replacement so that we can merge this PR? 😄
[nitpick] I would appreciate a more descriptive commit message, suggestion:
[installer] Improve install.ps1 code
- Remove progress bar for better performance
- Check for PowerShell x86 as it is unsupported
- Disable `KeepAlive` in connectivity check as no subsequent request are
expected
@@ -76,7 +76,16 @@ param ( | |||
[switch]$noReboots, | |||
[switch]$noChecks | |||
) | |||
$ErrorActionPreference = "Stop" | |||
$ErrorActionPreference = 'Stop' | |||
$ProgressPreference = 'SilentlyContinue' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the progress bar is a nice improvement! ✨
|
||
# https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.localaccounts/set-localuser | ||
if ( -not ([Environment]::Is64BitProcess)) { | ||
Write-Host "`t[!] It seems like you are using 32 bit powershell. Exiting because some commands do not work correctly in this mode." -ForegroundColor Red |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As powershell x86 is not working at the moment, I think adding this code is a good idea. But on the long-term, is there a reason why we would want to support powershell x86 (we would need to find alternative implementations for the used function(s))? 🤔 @mandiant/vms opinions?
@@ -94,7 +103,7 @@ function Test-WebConnection { | |||
|
|||
$response = $null | |||
try { | |||
$response = Invoke-WebRequest -Uri "https://$url" -UseBasicParsing | |||
$response = Invoke-WebRequest -Uri "https://$url" -UseBasicParsing -DisableKeepAlive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement, this function is only used to test the internet connection so we do not need a persistent connection to the server as there are not subsequent requests. 👍
Added Test-WebConnection 'google.com' to maintainers opinion
@Ana06 I have changed this back to google, even though I personally know many people who would prefer Microsoft if in a windows environment. But it should be able to be merged now. |
If I have free time in the future I can look into making this script compatible with pwsh x86. The problem I found when using x86 was with the Set-LocalUser cmdlet. Microsoft documentation lists this: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.localaccounts/set-localuser To resolve this issue we would have to find an alternative to the Microsoft.PowerShell.LocalAccounts module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the modification, but I think you accidentally changed the wrong domain. Please restore the domains in the internet connectivity check so that we can merge the PR.
I would prefer if you could squash your commits (to avoid fix commits in order to keep a clean commit history), but we can also merge the PR with squash an merge
if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements @CamoCatX!
If I have free time in the future I can look into making this script compatible with pwsh x86. The problem I found when using x86 was with the Set-LocalUser cmdlet. Microsoft documentation lists this: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.localaccounts/set-localuser To resolve this issue we would have to find an alternative to the Microsoft.PowerShell.LocalAccounts module
I have created #615 for this. It would be great if could work on it! 😄 If/When you have time to work on it, just write a comment in the issue to avoid someone else works on it at the same time. 😉
I added 4 different changes to this script.