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.

Sign up for GitHub

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

Jump to bottom

(#3318) Add ShouldProcess support to rewritten helper cmdlets #3532

Merged
merged 2 commits into from
Nov 4, 2024
Merged

(#3318) Add ShouldProcess support to rewritten helper cmdlets #3532

merged 2 commits into from
Nov 4, 2024

Conversation

vexx32
Copy link
Member

@vexx32 vexx32 commented Oct 21, 2024 •
edited
Loading

Description Of Changes

  • Add ShouldProcess support to Set-EnvironmentVariable, Install-ChocolateyPath, Uninstall-ChocolateyPath, and Update-SessionEnvironment
  • Add unit tests verifying just the base logic of the commands, that the ShouldProcess lines get hit when you use -WhatIf in the appropriate circumstances.
  • Tag potentially destructive / system-state-modifying tests with VMOnly so they do not run by default in typical dev environments

Motivation and Context

In discussion with @gep13 we realised there is value in having the ability to non-destructively verify the logic paths taken in the code here. As such, I have augmented the existing commands and associated helper classes with ShouldProcess semantics to make this workable.

Testing

  1. Run build.ps1
  2. Run ./Invoke-Tests.ps1 -Tags WhatIf

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 -- will submit a docs PR to update these cmdlet docs as well, ShouldProcess will add -Confirm and -WhatIf to the documentation list iirc
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v3 compatibility checked?

Related Issue

Part of #3318

Docs PR: chocolatey/docs#1086

@vexx32 vexx32 requested review from gep13 and corbob October 21, 2024 20:31
@vexx32 vexx32 mentioned this pull request Oct 22, 2024
Merged
7 tasks
gep13
gep13 previously requested changes Oct 25, 2024
View reviewed changes
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.

This is a great addition, and I don't see any reason that we can't pull this into 2.4.0. I have left one minor comment, but other than that, this looks great!

tests/pester-tests/powershell-commands/Uninstall-ChocolateyPath.Tests.ps1 Outdated Show resolved Hide resolved
@gep13
Copy link
Member

gep13 commented Oct 25, 2024

Also, this is tagged against #3477 but should it not actually be tagged against #3318? I realise that this has already shipped, but this change is related to those cmdlets, and not the new ones that will be converted as part of #3477.

@vexx32 vexx32 force-pushed the ShouldProcess-cmdlets branch from 061a60e to 707db73 Compare October 25, 2024 20:34
@vexx32
Copy link
Member Author

vexx32 commented Oct 25, 2024

@gep13 I think I've addressed both of those, thanks! Also rebased on develop.

@vexx32 vexx32 added this to the 2.4.0 milestone Oct 31, 2024
@vexx32 vexx32 changed the title (#3477) Add ShouldProcess support to rewritten helper cmdlets (#3318) Add ShouldProcess support to rewritten helper cmdlets Oct 31, 2024
corbob
corbob requested changes Nov 1, 2024
View reviewed changes
Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

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

Just some standardization of the ShouldProcess casing. Will need to make equivalent changes in the Pester tests.

src/Chocolatey.PowerShell/Helpers/EnvironmentHelper.cs Show resolved Hide resolved
src/Chocolatey.PowerShell/Helpers/EnvironmentHelper.cs Outdated Show resolved Hide resolved
src/Chocolatey.PowerShell/Helpers/EnvironmentHelper.cs Outdated Show resolved Hide resolved
@vexx32 vexx32 force-pushed the ShouldProcess-cmdlets branch from 707db73 to a523d82 Compare November 1, 2024 20:04
@gep13 gep13 added the NO RELEASE NOTES Should not be included in the release notes - not enhancing or fixing end product. label Nov 4, 2024
vexx32 added 2 commits November 4, 2024 14:38
@vexx32 @corbob
(chocolatey#3318) Add ShouldProcess support to commands
a65a51f 
Some cmdlets should support ShouldProcess, this change ensures that they
do correctly support it, along with adding checks to the underlying
methods to make it work as users will expect.

As Chocolatey CLI already supports --dry-run itself, this is primarily
going to be useful for effectively unit testing the commands.
@vexx32 @corbob
(chocolatey#3318) Add -WhatIf unit tests for helper commands
c209065 
Update-SessionEnvironment, Set-EnvironmentVariable, and
(Un)Install-ChocolateyPath implement ShouldProcess, so we can use
-WhatIf to verify their behaviour with unit tests.
@corbob corbob force-pushed the ShouldProcess-cmdlets branch from a523d82 to c209065 Compare November 4, 2024 22:39
@corbob corbob dismissed gep13’s stale review November 4, 2024 22:40

These changes have been made.

corbob
corbob approved these changes Nov 4, 2024
View reviewed changes
Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

@corbob corbob merged commit 5d0e42d into chocolatey:develop Nov 4, 2024
5 checks passed
@corbob
Copy link
Member

corbob commented Nov 4, 2024

Thanks for getting this fixed up @vexx32 👍

@corbob corbob added 4 - Done and removed 3 - Review labels Nov 4, 2024
@vexx32 vexx32 deleted the ShouldProcess-cmdlets branch November 5, 2024 14:25
@vexx32
Copy link
Member Author

vexx32 commented Nov 12, 2024

🎉 This issue has been resolved in version 2.4.0 🎉

The release is available on:

@vexx32 vexx32 removed the 4 - Done label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corbob corbob corbob approved these changes

@gep13 gep13 gep13 left review comments

Assignees
No one assigned
Labels
5 - Released Improvement NO RELEASE NOTES Should not be included in the release notes - not enhancing or fixing end product.
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@vexx32 @gep13 @corbob