-
Notifications
You must be signed in to change notification settings - Fork 915
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 check for Internet connectivity #504
Conversation
install.ps1
Outdated
@@ -241,6 +242,21 @@ if (-not $noChecks.IsPresent) { | |||
} | |||
} | |||
|
|||
# Check Internet connectivity and return boolean value, exit if value is 'false' | |||
Write-Host "[+] Checking for Internet connectivity..." | |||
$connectionStatus = Test-Connection 1.1.1.1 -Quiet |
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.
what are your thoughts on adding a second check like to 8.8.8.8
here as a backup?
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.
[nitpick] even better would be to check that the computer can reach myget or github as we rely on them in the installer
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.
Added check for github.com
, tried with myget.org
& www.myget.org
however they both seem to be blocking the ICMP packets.
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.
@wand3rlust this is a great addition! thanks for the PR and the detailed description with screenshots 👍
We are planing to add some other checks and improvements to the installer in the next few days (Powershell version check, update to use the new debloat and installer packages, etc.), that will cause merge conflicts with this PR. It would be good if you could address the changes requested quick, so that we can merge this PR before the rest (to avoid you have to rebase with the new changes) 😉
@@ -239,6 +240,22 @@ if (-not $noChecks.IsPresent) { | |||
} else { | |||
Write-Host "`t[+] Username '$extractedUsername' does not contain any spaces." -ForegroundColor Green | |||
} | |||
|
|||
# Check Internet connectivity and return boolean value, exit if value is 'false' |
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.
This should be indented. But I think it is better to add a linter for this: #507
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.
@wand3rlust thanks for the contribution and for addressing the changes that fast!
Added check to detect Internet connectivity (via ICMP) using 'Test-Connection' cmdlet. The response is a boolean value and if said value is 'false', installer exits.
[Tested on]:
[Case1]: Internet connectivity detected
[Case 2]: Internet connectivity not detected
Closes #486