-
Notifications
You must be signed in to change notification settings - Fork 979
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
Update installer to organize packages by categories #672
base: main
Are you sure you want to change the base?
Conversation
65d91a5
to
f316eb6
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 the installer here is broken because #671 broke the installer. Please rebase after #675 is merged so that the code works.
The fist UI window (install customization) is missing a next button, so I couldn't test the categories page:
Please address this issue so that I can review the new window as well.
Remember adding labels to PRs (in this case enhacement
is appropriate) and to add the issue the PR closes to the PR description (this PR should address #578).
install.ps1
Outdated
try { | ||
(New-Object System.Net.WebClient).DownloadFile($categoriesUrl, $categoriesFile) | ||
$categories = Get-Content -Path $categoriesFile | ||
} catch { |
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.
If you catch the error, how can you continue to execute the rest of the code without categories?
install.ps1
Outdated
$form.StartPosition = 'CenterScreen' | ||
|
||
#needed to use Find-Package | ||
install-packageprovider nuget -force |
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.
Make the installation of nuget consistency (same code structure) with the installation of other dependencies like boxstarter and chocolatey. Concretely: check if it is installed if possible, and print a message that is being installed and installed successfully instead of the output:
Name Version Source Summary
---- ------- ------ -------
nuget 2.8.5.208 https://onege... NuGet provider for the OneGet meta-package manager
f7c4e94
to
3100f2b
Compare
After the initial form to set up the environmental variables a new window will display all the packages grouped by category. The packages from `config.xml` will be selected by default.
6caf0ca
to
8aa429c
Compare
@Ana06 there is no need to use |
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 I was not able to review the installer UI as the installer does not run correctly:
Get-Packages-Categories : The term 'Get-Packages-Categories' is not recognized as the name of a cmdlet, function,
script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is
correct and try again.
At C:\Users\flare\Desktop\install.ps1:491 char:27
+ $packagesByCategory = Get-Packages-Categories
+ ~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : ObjectNotFound: (Get-Packages-Categories:String) [], ParentContainsErrorRecordException
+ FullyQualifiedErrorId : CommandNotFoundException
I think is because you have placed the code of Get-Packages-Categories
after the use of the function (it needs to be defined before). Please ensure the code works as it otherwise make it the review difficult.
I also recommend you to add some screenshots when making UI changes.
install.ps1
Outdated
## PACKAGE SELECTION BY CATEGORY | ||
################################################################################ | ||
|
||
function Get-Packages-Categories { |
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.
@Ana06 there is no need to use
Find-Package
, therefore the installation of nuget or downloadcategories.txt
is not necessary. At the end of the feed from https://www.myget.org/F/vm-packages/Packages there is a<link rel="next"
with the URL for the next packages.
The packages are selected and the Continue button adds the selected items to theconfig.xml
Seeing how complicated the code to use https://www.myget.org/F/vm-packages/api/v2/Packages?$filter=IsLatestVersion%20eq%20true
is, I think the solution with Find-Package
was better: much simpler code and easier to maintain.
I executed it without any issues, but when I switched to a different snapshot with Powershell version 5.1 I had the same error |
84595c3
to
8d8d15e
Compare
c9902a5
to
2ad68c8
Compare
Retrieve packages from the APIurl `https://www.myget.org/F/vm-packages` instead of using `Find-Package` that required the installation of nuget
2ad68c8
to
94db0a1
Compare
Update installer so that after the initial form to set up the environmental variables a new window displays all the packages grouped by category.
config.xml
need to be selected by default.Closes #578