-
Notifications
You must be signed in to change notification settings - Fork 1k
Add gui preinstall checks #707
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
Conversation
7dbc832
to
5c11551
Compare
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.
@sara-rn thanks for working on this enhacements!
As already mentioned in your previous PRs: when making UI changes, add a screenshot to make the review of the PR easier.
This code is duplicating a lot of code and logic in both the UI and command line. This makes it difficult to maintain the code and keep the logic consistent. As said in , we have to ensure we do not duplicate code to make it easy to keep both views consistent. One way to implement this is to have a checks array with an entry per check with the following information:
(name, description, function, allow_continue)
For example:
("PowerShell version >= 5", Check-PowerShell-Version, False)
The function should return an info message that we can display in the command line (as we do at the moment) and in the UI (extra info when a check fails, as the text below the check title). I think we can summarize some of the info messages if needed. For example:
function Check-PowerShell-Version {
$psVersion = $PSVersionTable.PSVersion
if ($psVersion -lt [System.Version]"5.0.0") {
return "Your PowerShell version ($psVersion) is not supported"
}
}
The PSAvoidUsingWMICmdlet
linter complain should be easy to fix by replacing the used functions as explained in https://learn.microsoft.com/en-us/powershell/utility-modules/psscriptanalyzer/rules/avoidusingwmicmdlet?view=ps-modules In any case, focus on implementing the code using a checks array, as I think this may fix the other offense as well.
$checksPassed = $true | ||
$mandatoryChecksPassed = $true | ||
|
||
################################# Functions that conduct Pre-Install Checks ################################# |
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.
Keep new headers consistent with existent ones:
################################# Functions that conduct Pre-Install Checks ################################# | |
################################################################################ | |
Functions that conduct Pre-Install Checks | |
################################################################################ |
install.ps1
Outdated
if ($psVersion -lt [System.Version]"5.0.0") { | ||
Write-Host "`t[!] You are using PowerShell version $psVersion. This is an old version and it is not supported" -ForegroundColor Red | ||
Read-Host "Press any key to exit..." | ||
exit 1 | ||
return $false | ||
} else { | ||
Write-Host "`t[+] Installing with PowerShell version $psVersion" -ForegroundColor Green | ||
return $true |
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.
Replace all code blocks like:
if (condition) {
return $false
} else {
return $true
}
by:
return (inverse_condition)
In this case:
return ($psVersion -lt [System.Version]"5.0.0")
Write-Host "[+] Checking if PowerShell version is compatible..." | ||
if (-not (Test-PSVersion)){ | ||
Write-Host "`t[!] You are using PowerShell version $psVersion. This is an old version and it is not supported" -ForegroundColor Red | ||
$mandatoryChecksPassed = $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.
[nitpick] I would prefer we exist immediately in the command line if a mandatory check fails. This is very easy to do if you implement it with an array of checks as I proposed above.
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.
ok but we cannot exit in the GUI, correct?
I wanted to implement the checks the same way in both cli and the GUI so that's why it doesn't exit when a mandatory check fails
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.
That's a good argument. Let's keep it consistent then by performing all the checks in both. Note the order of the checks at the moment is not consistent. The use of an array of checks would remove current code duplication and ensure consistency in order.
in the proposed function you would return a message,then how would you check if a |
@Ana06 just to clarify, you still want to display messages via |
An empty string is the same as False for PowerShell: $error_info = Test-PowerShell-Version
if ($error_info) {
// The check has failed
}
No. My proposal was to display the same message in the console and the GUI (only in one of them), but to display the exact same message by returning it by the Test function if the check fails. Regarding the internet check, you can perform the check before displaying the UI. This is similar to preloading the packages for next UI view. I think we do need to check the three domains to avoid issues, but we do not need to log them, just a message for failure is enough. |
@sara-rn thanks for the last screenshot, can you make the Installation Checks box (and the text inside) and the text in the check boxes a bit longer so that there is not that much empty space to the right? I would also be nice to have the text in every of the check boxes in a single line (by making the box a bit bigger) and to add some space above the buttons. |
![]() |
@Ana06 how about the Hint messages we display? Shall we only display them in the console right?
|
No, my proposal was to display the exact same messages in both the console and the UI (just a single message), but shortening them a bit if needed. For example: |
Thanks @Ana06 for the clarification, then all these links to articles would disappear as you would only summarize it in a single line:
or these knowledge articles:
|
Yes, what about moving all this information to the README and linking it if any of them fail? We have already some of this information in the README and keeping the information in sync is tricky. So I think having shorter messages in the installer (consistent between the UI and the console) and linking extra documentation that we can maintain in a single place is an enhancement. |
5c11551
to
46959a8
Compare
@Ana06 where in the GUI you want to add the link to the README? below the two checkboxes? |
I implemented all the Check functions as you suggested, however I didn't implement the checks_array because it requires a lot of duplication in any case as there are three components in the GUI that are created per check and this cannot be done dynamically. If a check is not successful we have to manually show the $error_info in the label and change the color from grey to red, so it not only shows False but the error message gets updated. |
2c9168b
to
58fc51b
Compare
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.
Please use an array of checks as proposed to remove current code duplication, making the code easy to maintain and ensuring consistency between both modes. Note that the order of the checks is important and the following must be performed first: powershell version, admin rights, execution policy unrestricted. This is not the case at the moment in the GUI.
The GUI views fails with the following error:
PS C:\Users\flare\Desktop> .\install.ps1 -password password
[+] Starting GUI to allow user to edit configuration file...
[+] Checking for Internet connectivity (google.com)... (mandatory)
[+] Checking for Internet connectivity (github.com)... (mandatory)
[+] Checking for Internet connectivity (raw.githubusercontent.com)... (mandatory)
The property 'Visible' cannot be found on this object. Verify that the property exists and can be set.
At C:\Users\flare\Desktop\install.ps1:838 char:8
+ $BreakMyInstallLabel.Visible = $false
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : InvalidOperation: (:) [], ParentContainsErrorRecordException
+ FullyQualifiedErrorId : PropertyNotFound
Write-Host "[+] Checking if PowerShell version is compatible..." | ||
if (-not (Test-PSVersion)){ | ||
Write-Host "`t[!] You are using PowerShell version $psVersion. This is an old version and it is not supported" -ForegroundColor Red | ||
$mandatoryChecksPassed = $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.
That's a good argument. Let's keep it consistent then by performing all the checks in both. Note the order of the checks at the moment is not consistent. The use of an array of checks would remove current code duplication and ensure consistency in order.
|
1dd4b04
to
f6728b3
Compare
@Ana06 It should work on Windows 11 but there seems to be a problem with this Windows Build. The specific line is using WMI
Option 2
See screenshot from the old installer running in my Windows 10: |
@sara-rn I am not sure why you have changed the implementation of the Windows Defender Check and I do not understand why you say that reverting to the previous implementation is not an option because it raises an exception. Can you please provide more details? What exception exactly do you get? The previous version of the installer works for me. @sara-rn can you also please explain in detail what every of the lines of the following code does? The code is not clear to me. $defender = Get-CimInstance -Namespace "root\Microsoft\Windows\Defender" -Class MSFT_MpPreference
if ($defender.DisableRealtimeMonitoring) {
if (Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Windows Defender\Features" -Name "TamperProtection" -ea 0) {
if ((Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Windows Defender\Features" -Name "TamperProtection").TamperProtection -eq 5){
return "Windows Defender and Tamper Protection are enabled"
}
}
} |
I have done some more testing, there is not a problem with the build, it is caused by disabling Windows Defender and is expected. In Windows 11 In general (and independently of fixing this check which needs to be done), adding a try-catch to every of the checks is a good idea as it would ensure we display that we are not sure if the check is verified or not without failing the installer. Please as said in my previous comment, explain why it was needed to change the implementation of the Windows Defender check and provide the error you get in Windows 10. |
This is the error I get with the old implementation when I ran it in my Windows 10 VM after removing the try/catch:
|
I have confirmed with @sara-rn that that VM has Windows Defender enabled in the VM she gets the error and consequently the current code works as expected (as the check fails because Windows Defender may be enabled). I propose the following in order to merge this PR:
I have added the error @sara-rn got to #722 (comment) where we are discussing improving the Windows Defender check. This is completely independent from this PR if we do not modify the check logic here. |
813e586
to
a89be79
Compare
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.
Some minor changes requests including fixing typos introduced in the last change and keeping the messages consitent.
4e63578
to
b1ed672
Compare
e128bb4
to
30db070
Compare
A new window will automatically check for preinstall checks, which will allow the user to continue if the checks are not mandatory. Two new global variables `$mandatoryChecksPassed` and `$checksPassed` are created to determine if any of the mandatory or optional checks are passed or not. If the mandatory checks are not passed the installation will display a message that cannot continue.. New functions have been created for each of the preinstall checks. Each function contains a try/catch to capture exceptions Both in cli or gui mode the same functions must be invoked. Install gui window is displayed inmediatelly before Boxstarter is installed Preload gui controls of the custom configuration window Add a checkbox in case some non-mandatory checks are not successful Add a second checkbox to confirm a snapshot has been taken Only display the first checkbox if some checks were not succesful
2327521
to
19bac92
Compare
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.
Tested locally, everything looks good. Thanks for all the work @sara-rn to add the checks to the UI 🚀
Based on the code from Commando-VM, created a new Window that performs pre-installation checks. The same pre-checks that are done if
-noGui
is provided.closes #509