Skip to content
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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

TheCakeIsNaOH
Copy link
Member

@TheCakeIsNaOH TheCakeIsNaOH commented Jan 28, 2023

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 has
the config passed in directly from the nuget service.

Testing

Chocolatey CLI validate remembered args functionality:

Chocolatey CLI test overriding args:

  • Uninstall curl if installed
  • Run .\choco.exe install curl --package-parameters="'/Curlv1Param'" --install-arguments="'/CurlV1Arg'" --version=7.81.0 --debug
  • Run .\choco.exe upgrade curl --package-parameters="'/Curlv2Param'" --install-arguments="'/CurlV2Arg'" --version=7.81.0 --debug
  • Validate that on the upgrade of curl that it uses the v2 param and arg

ChocolateyGUI:

  • Build ChocolateyGUI with a Chocolatey CLI assembly build from this PR branch
  • Run ChocolateyGUI and (if needed) uninstall curl
  • Ensure useRememberedArgumentsForUpgrades is enabled in ChocolateyGUI settings
  • Advanced install curl 7.81.0 with /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.
  • Go to this PC, view curl details, then view package arguments to validate that the params/args were remembered
  • Update curl
  • Wait for it to update, and then view details and click view package arguments and validate that the params/args were still remembered

Operating Systems Testing

  • Windows 10 22H2

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Part of #508
Fixes #2886
Partially fixes #2761

@TheCakeIsNaOH
Copy link
Member Author

@vexx32 when you have some time, would you be able to a look at this? Since you investigated #2886

@fabiorzfreitas
Copy link

Excuse me, I came from #2503 . Could someone please tell me what's stopping this PR from being merged? Thank you!

@TheCakeIsNaOH
Copy link
Member Author

TheCakeIsNaOH commented Aug 9, 2023

@fabiorzfreitas there is no guarantee or guideline on timelines for merging PRs:
https://github.com/chocolatey/choco/blob/develop/CONTRIBUTING.md#submit-pull-request-pr

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.

@TheCakeIsNaOH TheCakeIsNaOH force-pushed the remembered-new-parser branch 3 times, most recently from 468e15a to 1a012ce Compare January 10, 2024 00:00
@TheCakeIsNaOH TheCakeIsNaOH mentioned this pull request Jan 10, 2024
7 tasks
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the remembered-new-parser branch 3 times, most recently from 6ec0586 to d7d493a Compare January 10, 2024 01:24
@pauby
Copy link
Member

pauby commented Jan 24, 2024

Any update on this?

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.
@pauby
Copy link
Member

pauby commented Aug 23, 2024

Please don't tag people in PR's like this. The PR will be reviewed in due course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants