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

(#3477) Replace Get-ChecksumValid with Assert-ValidChecksum cmdlet. #3525

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

Conversation

vexx32
Copy link
Member

@vexx32 vexx32 commented Oct 3, 2024

Description Of Changes

  • Remove Get-ChecksumValid
  • Add Assert-ValidChecksum
  • Add tests for command to verify behaviour matches the old command.
  • Add alias for backwards compatibility with packages.
  • Minor fixes for the doc-gen script to address issues I ran into.
  • Added a framework we can use in future for deprecating old aliases or to-be-removed commands in a simple and standardised way.

This PR does not contain the compiled XML help information for the command. In discussion with @gep13 we agreed that it would be better to compile that in the final release. I will have a docs PR linked here in a moment, watch this space.

Note: As this PR is part of #3477 and is thus a breaking change to a degree, we will need to cut a support/2.x branch once this is merged, so that none of the breaking changes associated with #3477 inadvertently make their way into the 2.x versions.

Motivation and Context

See #3477

Documentation PR: chocolatey/docs#1075

Testing

Tested locally with the accompanying Pester tests:

  1. From an administrative PowerShell session
  2. Run .\build.ps1
  3. Run .\Invoke-Tests.ps1 -Tag AssertValidChecksum
  4. From VS, run the integration tests and verify there are no unexpected issues.

Proving out the deprecated commands framework

After running the above, close and reopen the admin PowerShell session, and then follow these steps:

  1. Apply the following patch: Get-ValidCheckSum-deprecation-verification.patch
  2. Rerun the prior steps to run the build and tests again, including the code and test change.
  3. Monitor the output from the tests; near the start of the test run, you should see a warning issued about the test calling Get-ChecksumValid and that the alias is deprecated and should be replaced with Assert-ValidChecksum

Operating Systems Testing

Windows 10

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 v3 compatibility checked?

Related Issue

Part of #3477

@vexx32 vexx32 requested review from gep13 and corbob October 3, 2024 20:38
@vexx32 vexx32 marked this pull request as draft October 3, 2024 20:40
@vexx32
Copy link
Member Author

vexx32 commented Oct 3, 2024

There is one final matter to resolve before this PR is fully ready; help documentation for the cmdlet. I will have a corresponding PR opened on the docs repository by tomorrow, and then update the Chocolatey.PowerShell.dll-help.xml file for this PR accordingly.

@vexx32 vexx32 marked this pull request as ready for review October 4, 2024 17:53
@vexx32 vexx32 force-pushed the assert-checksum branch 2 times, most recently from 1627d8f to 7574df1 Compare October 4, 2024 18:28
@vexx32
Copy link
Member Author

vexx32 commented Oct 7, 2024

There is one final matter to resolve before this PR is fully ready; help documentation for the cmdlet. I will have a corresponding PR opened on the docs repository by tomorrow, and then update the Chocolatey.PowerShell.dll-help.xml file for this PR accordingly.

After talking with @gep13 we have agreed it is best not to update the XML here at present, as it would require increasingly complicated messing around with the associated docs PRs as we rewrite more commands, and will instead opt to update the help XML file before a release artifact is created.

@vexx32 vexx32 force-pushed the assert-checksum branch 3 times, most recently from a1983e4 to f9be806 Compare October 15, 2024 14:44
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.

Can you get this PR rebased onto the head of develop?

// Use the following format to provide a deprecation notice. If the new command name is an empty string,
// the warning will inform the user it is to be removed instead of renamed.
//
// { "Deprecated-CommandName", "New-CommandName" },
Copy link
Member

Choose a reason for hiding this comment

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

In the linked patch file in the PR description, there is an additional line to add to this _deprecatedCommandNames dictionary. What is the reason that this is not included in this PR? Just so that we can see it in action, before and after?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, just to demonstrate that it works, as we don't have any fully deprecated command names just yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could arguably mark anything we rename immediately as a deprecated name and just have it be aliased until next major version, but wasn't sure if we wanted to do that yet.

As a small convenience, to allow us to run tests for an invididual
cmdlet when doing development testing.
This command replaces the Get-ChecksumValid helper function, adding an
alias for compatibility purposes.
Add tests to ensure the behaviour of rewritten command matches the
original behaviour.
When working on this, some irregularities were noted with the generation
of the documentation, so these changes avoid the issues that arose.
This allows us to maintain a simple list of deprecated aliases and the
new command name, or commands that are being removed, and automatically
warn when that command is called.
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.

2 participants