-
Notifications
You must be signed in to change notification settings - Fork 905
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
(#2886)(#2761) Improve remembered arguments handling #3003
base: develop
Are you sure you want to change the base?
(#2886)(#2761) Improve remembered arguments handling #3003
Conversation
8bcb5ad
to
5dba563
Compare
5dba563
to
fc678a7
Compare
fc678a7
to
8b3a36d
Compare
Excuse me, I came from #2503 . Could someone please tell me what's stopping this PR from being merged? Thank you! |
8b3a36d
to
1e549ef
Compare
@fabiorzfreitas there is no guarantee or guideline on timelines for merging PRs: The Chocolatey development team decided to not put this PR on any of the milestones for the releases yet. This PR likely requires changes to other products, and also would require validation and testing to ensure this change does not break other products. Therefore, it is not just as simple as checking that the tests pass and then clicking merge, it requires more work on the closed source products. So it is a business decision as to where time is spent. If you are a licensed customer, you could reach out to support, as licensed customers get some level of prioritization for bugs and features. Unfortunately, there is not really a documented way for non-licensed users to signal interest. Maybe the Github thumbs up reaction can be documented/used that way? https://github.com/chocolatey/choco/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc In terms of what I expect for a timeline, I do not expect this to be merged soon, as per the recent livestream where it was indicated that other products will be worked on for a time now that Chocolatey CLI v2.x has been released and has had bug fixe releases. |
468e15a
to
1a012ce
Compare
6ec0586
to
d7d493a
Compare
You can see the current status of the PR by looking at the comments above. |
This helps ensure that package IDs are handled in a case insensitive manner.
This allows overriding of remembered package parameters, install arguments, cache location and execution timeout during upgrade. So a user can pass in different package parameters or arguments without having to completely reinstall the package or turn off remembered arguments. Switch arguments cannot be checked, because the lack of a switch normally would mean that they are just relying on the remembered args to remember it, so there is no way to determine if an override of the remembered args is appropriate.
…ration This adds a new method, GetPackageConfigFromRememberedArguments which is similar to SetConfigFromRememberedArguments, but operates using a different method. First, a OptionSet is set up, so only the config that is passed in is modified, instead of the config that the global options parser was set with with. Second, it returns the modified configuration instead of the original configuration, because it is the local configuration being modified. Third, it has a more general name and changes log messages to be more general, so it later can more easily be reused for uninstall and export. This change fixes remembered arguments when Chocolatey is used as a library, like in ChocolateyGUI, where the config that is passed to the install_run method is not necessarily the same config object that was used to set up the global argument parser. The downside of using a new commandline parser is that it opens up the possibility of drift between the upgrade/global arguments and this added parser. However, this is not an issue because the format of the saved arguments is known, and any added arguments there would not work without being added here as well, which would be picked up during development.
d7d493a
to
85ab916
Compare
Please don't tag people in PR's like this. The PR will be reviewed in due course. |
Description Of Changes
First commit:
Makes some package id comparisons case insensitive. This helps ensure that package IDs are handled in a case
insensitive manner.
Second commit:
Allow overriding remembered params and args
This allows overriding of remembered package parameters, install
arguments, cache location and execution timeout during upgrade.
So a user can pass in different package parameters or arguments
without having to completely reinstall the package or turn off
remembered arguments.
Switch arguments cannot be checked, because the lack of a switch
normally would mean that they are just relying on the remembered args
to remember it, so there is no way to determine if an override of the
remembered args is appropriate.
Third Commit
Switch remembered args to only change local configuration
This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.
This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.
The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
Motivation and Context
See linked issues.
The possibility of setting the local config was opened up with the changes made in #508, because now the
handle_package_result
hasthe config passed in directly from the nuget service.
Testing
Chocolatey CLI validate remembered args functionality:
Chocolatey CLI test overriding args:
.\choco.exe install curl --package-parameters="'/Curlv1Param'" --install-arguments="'/CurlV1Arg'" --version=7.81.0 --debug
.\choco.exe upgrade curl --package-parameters="'/Curlv2Param'" --install-arguments="'/CurlV2Arg'" --version=7.81.0 --debug
ChocolateyGUI:
useRememberedArgumentsForUpgrades
is enabled in ChocolateyGUI settings/CurlOnlyParam
package parameter and/CurlOnlyArg
install argument. The current only way to do this is to either use the CLI or add a source with the older version, since version selection is broken.Operating Systems Testing
Change Types Made
Change Checklist
Related Issue
Part of #508
Fixes #2886
Partially fixes #2761