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

(#310, #3318) Add Uninstall-ChocolateyPath cmdlet & rewrite associated functions as cmdlets #3440

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

Conversation

vexx32
Copy link
Member

@vexx32 vexx32 commented May 10, 2024

Description Of Changes

  • Added Uninstall-ChocolateyPath helper cmdlet
  • Rewrote the following associated functions into C# cmdlets:
    • Get-EnvironmentVariable
    • Get-EnvironmentVariableNames
    • Install-ChocolateyPath
    • Set-EnvironmentVariable
    • Test-ProcessAdminRights
    • Update-SessionEnvironment
  • Tweaked the PowerShell command lookup behaviour when loading the chocolateyInstaller.psm1 module to ensure that licensed cmdlets can be preferentially used instead of open-source cmdlets or functions, regardless of any name collisions that may occur.
  • Tweaked the assembly resolver to ensure that if we have the licensed extension depend on the new Chocolatey.PowerShell assembly, it will not cause any issues when trying to load the licensed extension outside the PowerShell module context.
  • Added an update-cmdlet-documentation.ps1 script which will use PlatyPS to read the cmdlet metadata and keep documentation up to date.
    • Provides a -NewCommand switch to allow easier additions of documentation pages for any new cmdlets (PlatyPS does not add newly added commands by default).
    • This script directly adds or updates documentation in a local copy of the chocolatey/docs repository, and compiles an XML help file from the resulting content.

Motivation and Context

Partially, we just want to finally add Uninstall-ChocolateyPath. However, given overall contribution rates to the powershell functions are quite low, the decision has been made to rewrite our helper functions into C# cmdlets.

This PR serves as the initial set of cmdlets, a handful of useful example cases to prove out the viability of this approach, as well as ensuring that we have a better path forward for both OSS commands and any licensed commands we want to use in the future. It also opens up the possibility of more easily overriding the OSS cmdlets with a licensed command, and there are provisions in the chocolateyInstaller.psm1 module script to ensure that can work seamlessly.

Testing

  1. Using the add-path.zip package (renamed to .nupkg), attempt to install and uninstall the package running the chocolatey.console project in Visual Studio, specifying package parameters as /Path=C:\test /Scope=Machine (or other paths, or with /Scope=User).
  2. Build the project to completion and install the resulting package on a VM. Run the same test on this VM.
  3. Copy the installation of Chocolatey here to a folder that does not require administrative access, set $env:ChocolateyInstall to this folder, and attempt to run the same tests as before with the choco.exe in this folder. You should notice that any attempts to write to the Machine scope path fail with an error.
  4. In a PowerShell session, import the chocolatey installer module Import-Module $env:ChocolateyInstall/helpers/chocolateyInstaller.psm1 and check the Get-Help results for Install-ChocolateyPath, Uninstall-ChocolateyPath, and the other provided cmdlets. None of the cmdlets added/rewritten here should be missing help.

There will be a related PR to the licensed extension repository with some additional instructions to confirm the command overriding behaviour works and inheritance from this new assembly can function correctly.

For documentation generation:

  1. Ensure you have the chocolatey/docs repository cloned locally. (use the branch from this draft docs PR: (doc) Add/update docs for Chocolatey.PowerShell module docs#993)
  2. Run the provided update-cmdlet-documentation.ps1 script, optionally providing the path to the docs repo folder with -DocsRepositoryPath if it is cloned in a folder that is not ../docs relative to the choco repository.

For the present I have included the XML rendition of the help in the repository, and I think it makes sufficient sense to just update this file whenever we need to; the markdown help can remain in the docs repository, and updating the XML help for this PowerShell module can be done as-needed and just committed to this repository.

The Invoke-Tests.ps1 file should also run the included Pester tests for these commands.

Operating Systems Testing

Win10

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?

Related Issue

Fixes #310

Fixes #3318

@vexx32 vexx32 requested a review from gep13 May 10, 2024 16:12
@vexx32 vexx32 force-pushed the uninstall-chocolateypath-cmdlets branch 3 times, most recently from 4dc4c82 to 1b4485c Compare May 13, 2024 14:59
@vexx32 vexx32 changed the title (#310) Add Uninstall-ChocolateyPath cmdlet & rewrite associated functions as cmdlets (#310, #3318) Add Uninstall-ChocolateyPath cmdlet & rewrite associated functions as cmdlets May 20, 2024
recipe.cake Outdated Show resolved Hide resolved
recipe.cake Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add a test for installation of the add-path package, mentioned in the manual tests of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look at doing this bit tomorrow, yes :3

Copy link
Member

Choose a reason for hiding this comment

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

@vexx32 am I right in saying that this still needs to be done?

@vexx32 vexx32 force-pushed the uninstall-chocolateypath-cmdlets branch from 40934f0 to 50e5994 Compare May 20, 2024 20:11
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

Great work on this @vexx32. @gep13 asked me to have an additional look over the changes, and comment if I noticed anything.

recipe.cake Outdated Show resolved Hide resolved
recipe.cake Show resolved Hide resolved
src/Chocolatey.PowerShell/Shared/EnvironmentVariable.cs Outdated Show resolved Hide resolved
src/chocolatey.console/chocolatey.console.csproj Outdated Show resolved Hide resolved
src/chocolatey.resources/helpers/chocolateyProfile.psm1 Outdated Show resolved Hide resolved
src/chocolatey.sln Show resolved Hide resolved
@gep13
Copy link
Member

gep13 commented May 21, 2024

@vexx32 one other thing that I thought about yesterday, but didn't mention...

Can you update your commit messages to provide more information? If/when reviewing the commits directly, we won't have the same context that exists in this PR. I am thinking that all the information in the PR description should make its way into the commit messages.

Thoughts?

@vexx32
Copy link
Member Author

vexx32 commented May 21, 2024

That sounds like rather a lot, but I don't disagree, I'll look to do that too.

EDIT: Done.

@vexx32 vexx32 force-pushed the uninstall-chocolateypath-cmdlets branch 3 times, most recently from 7aea2c1 to c985b57 Compare May 22, 2024 13:45
Finally added Uninstall-ChocolateyPath. However, given overall contribution rates to the powershell functions are quite low, the decision has been made to rewrite our helper functions into C# cmdlets.

This serves as the initial set of cmdlets, a handful of useful example cases to prove out the viability of this approach, as well as ensuring that we have a better path forward for both OSS commands and any licensed commands we want to use in the future. It also opens up the possibility of more easily overriding the OSS cmdlets with a licensed command, and there are provisions in the `chocolateyInstaller.psm1` module script to ensure that can work seamlessly.

Summary of changes:

- Added Uninstall-ChocolateyPath helper cmdlet
- Rewrote the following associated functions into C# cmdlets:
  - Get-EnvironmentVariable
  - Get-EnvironmentVariableNames
  - Install-ChocolateyPath
  - Set-EnvironmentVariable
  - Test-ProcessAdminRights
  - Update-SessionEnvironment
- Tweaked the PowerShell command lookup behaviour when loading the `chocolateyInstaller.psm1` module to ensure that licensed cmdlets can be preferentially used instead of open-source cmdlets or functions, regardless of any name collisions that may occur.
- Tweaked the assembly resolver to ensure that if we have the licensed extension depend on the new Chocolatey.PowerShell assembly, it will not cause any issues when trying to load the licensed extension outside the PowerShell module context.
- Added an `update-cmdlet-documentation.ps1` script which will use PlatyPS to read the cmdlet metadata and keep documentation up to date.
  - Provides a `-NewCommand` switch to allow easier additions of documentation pages for any new cmdlets (PlatyPS does not add newly added commands by default).
  - This script directly adds or updates documentation in a local copy of the chocolatey/docs repository, and compiles an XML help file from the resulting content.
@gep13 gep13 force-pushed the uninstall-chocolateypath-cmdlets branch from c985b57 to c7149f5 Compare May 23, 2024 06:43
@gep13
Copy link
Member

gep13 commented May 23, 2024

@vexx32 can you let me know when the last remaining unresolved comments have been worked on? I would like to get this merged, so that we can continue with testing. Thanks

NOTE: I will mention here again, I forced pushed this PR on the head of develop, as I thought that it was ready for merging. You will need to reset you clone before continuing.

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