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

(#23) Handle all keyword for install #2541

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

TheCakeIsNaOH
Copy link
Member

@TheCakeIsNaOH TheCakeIsNaOH commented Jan 20, 2022

Description Of Changes

This adds the ability to install all packages from a source via the all
keyword. It runs a list against the current sources, and sets the
package names from that list of packages.

This functionality is only intended to be used with internal
repositories. Therefore a list of public repositories has been added,
and the current sources are checked against it to ensure only internal
sources are specified.

Renames the set_package_names_if_all_is_specified function to
set_local_package_names_if_all_is_specified. This is to increase
clarity with the set_remote_package_names_if_all_is_specified now being added.

Also this throws a not implemented exception if the all keyword is used with
non-nuget alternate sources. This is because the all keyword only has
handling built in for normal sources, and it is better to throw an
error than to have the source try to install a package called "all"

Motivation and Context

Adding the all keyword for install will bring C# choco up to feature parity with PowerShell choco. Or, at least all of the feature parity issues will be closed.

Testing

Run these to validate that choco will throw an error because public repositories are one of the sources:

choco install all --allow-unofficial
choco install all --allow-unofficial --source=https://community.chocolatey.org/api/v2/
choco install all --allow-unofficial --source=https://chocolatey.org/api/v2/
choco install all --allow-unofficial --source=https://www.nuget.org/api/v2/
choco install all --allow-unofficial --source=https://licensedpackages.chocolatey.org/api/v2/
choco install all --allow-unofficial --source=https://push.chocolatey.org/api/v2
choco install all --allow-unofficial --source="'https://community.chocolatey.org/api/v2/;"C:\packages"'"

Run these to validate that alternate sources fail with a notimplemented exception:

choco install all --allow-unofficial --source=WindowsFeatures
choco install all --allow-unofficial --source=ruby
choco install all --allow-unofficial --source=python
choco install all --allow-unofficial --source=webpi
choco install all --allow-unofficial --source=cygwin

Download these packages to a local folder:

  • curl 7.81.0
  • wget 1.21.2
  • iperf2 2.0.13
  • iperf2 2.0.14
  • iperf2 2.0.14.1001-alpha20201022

Run these commands pointing to that local folder to test the behavior of the all keyword:

Ensure that iperf 2.0.14 (i.e. not the prerelease version), wget and curl are installed:
choco install all --allow-unofficial --source=c:\packages

Ensure that the prerelease iperf is installed when prerelease is specified:
choco install all --allow-unofficial --source=c:\packages --pre

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)

Related Issue

Fixes #23

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.
  • N/A PowerShell v2 compatibility checked.

@TheCakeIsNaOH
Copy link
Member Author

For reference, here is the previous (PowerShell) implementation of handling the all keyword:
https://github.com/chocolatey-archive/chocolatey/blob/master/src/functions/Chocolatey-InstallAll.ps1

@TheCakeIsNaOH
Copy link
Member Author

TheCakeIsNaOH commented Feb 5, 2022

TODOs:

  • Commits reword
  • Fill in PR template
  • Local testing, then fill in manually testing section
  • Disable or implement "all" keyword for alternate sources
  • Check if other specific urls need to be disabled (e.g. other major public nuget repositories, alternate URLs for chocolatey.org/nuget.org?)
  • Maybe allow disabling other sources as well via config?
  • Documentation in install command help

@TheCakeIsNaOH TheCakeIsNaOH force-pushed the install-all branch 2 times, most recently from 827332a to ac8a735 Compare February 10, 2022 04:49
@TheCakeIsNaOH TheCakeIsNaOH marked this pull request as ready for review February 10, 2022 04:49
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the install-all branch 2 times, most recently from 359784c to 82ee402 Compare June 27, 2022 15:47
@coveralls
Copy link

coveralls commented Jun 27, 2022

Coverage Status

Coverage remained the same at 27.567% when pulling 9108aec on TheCakeIsNaOH:install-all into 3892cfb on chocolatey:develop.

@TheCakeIsNaOH TheCakeIsNaOH force-pushed the install-all branch 2 times, most recently from 9c52293 to a4557e9 Compare January 13, 2023 15:28
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the install-all branch 2 times, most recently from 001fad7 to 3cdfbd6 Compare June 6, 2023 22:59
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the install-all branch 2 times, most recently from e4044bb to 33c96d1 Compare July 1, 2023 03:17
@gep13
Copy link
Member

gep13 commented Dec 14, 2023

Some quick thoughts on this one...

We would want to limit usage of this to only custom sources, i.e. don't allow this on CCR, or Licensed Feed.

And, add a new property to source element, similar to selfService and adminOnly. So we would have installAll as an element.

@TheCakeIsNaOH
Copy link
Member Author

We would want to limit usage of this to only custom sources, i.e. don't allow this on CCR, or Licensed Feed.

This should already be the case, see the PublicNugetSources in ApplicationParameters.cs

And, add a new property to source element, similar to selfService and adminOnly. So we would have installAll as an element.

Would that be a breaking change for the choco source output?

@gep13
Copy link
Member

gep13 commented Apr 23, 2024

@TheCakeIsNaOH thank you for your work on this. I have taken this for a spin, and made a couple of changes based on a conversation that I have had with @pauby.

We are happy to move forward with this as-is, with some follow up work with regard to the introduction and usage of another property on the source element in the chocolatey.config file to allow setting installAll as enabled/disabled, in the same way that you can with selfService, and adminOnly

This renames the SetPackageNamesIfAllSpecified function to
SetLocalPackageNamesIfAllSpecified. This is to increase clarity for when
the all keyword is implemented, and a function will be added that sets
packages names from a remote source.
This adds the ability to install all packages from a source via the all
keyword. It runs a list against the current sources, and sets the
package names from that list of packages.

This functionality is only intended to be used with internal
repositories. Therefore a list of public repositories has been added,
and the current sources are checked against it to ensure only internal
sources are specified.
This throws a not implemented exception if the all keyword is used with
non-nuget alternate sources. This is because the all keyword only has
handling built in for normal sources, and it is better to throw an
error than to have the source try to install a package called "all"
One test to ensure that if the source is CCR that the install fails.
One test to ensure that all packages are install from a local source.
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@gep13 gep13 merged commit 43d9da4 into chocolatey:develop Apr 23, 2024
5 checks passed
@gep13
Copy link
Member

gep13 commented Apr 23, 2024

@TheCakeIsNaOH thanks again for getting this functionality added in!

@TheCakeIsNaOH TheCakeIsNaOH deleted the install-all branch April 26, 2024 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow all packages from a source to be installed
3 participants