PowerShellForGitHub: Improve and Standardise WhatIf/Confirm Processing

[Simon Heather (X-Guardian)]

Description

This PR improves and standardises the WhatIf/Confirm ('ShouldProcess') processing across all the functions.

Summary of changes

• Moved the $PSCmdlet.ShouldProcess check out of Invoke-GHRestMethod and added it to the calling functions with the correct operation and target.
• Added WhatIf:$false and Confirm:$false to the Out-File Cmdlet call in the Write-Log function in the Helpers module. This will prevent the Write-Log function displaying WhatIf and Confirm prompts.
• Removed ShouldProcess from non-state changing functions.
• Modified the current ShouldProcess conditions to use an early return.
• Modified the Set-GitHubIssueLabel function to remove ConfirmImpact='High' and set ConfirmPreference to 'Low' if no labels have been specified instead.
• Modified the Update-GitHubRepository function to remove ConfirmImpact='High' and set ConfirmPreference to 'Low' if the repo is being renamed instead.

Issues Fixed

• FixesPowerShellForGitHub: Suggested WhatIf/Confirm Processing Improvements #225

References

N/A

Checklist

• You actually ran the code that you just wrote, especially if you did just "one last quick change".
• Comment-based help added/updated, including examples.
• Static analysis is reporting back clean.
• New/changed code adheres to our coding guidelines.
• New/changed code continues to support the pipeline.
• Changes to the manifest file follow the manifest guidance.
• Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.
• Relevant usage examples have been added/updated in USAGE.md.
• If desired, ensure your name is added to our Contributors list

[Howard Wolosky (HowardWolosky)]

A few lingering questions/nits, and then there's the need to handle the merge conflict in GitHubCore.ps1 and Telemetry.ps1 introduced by the recent change that removed Status.

[Howard Wolosky (HowardWolosky)]

Also -- please let me know in what order you'd prefer your changes to go in...if you're wanting this one to go in before your other changes or after.

[Howard Wolosky (HowardWolosky)]

After looking through this one more time, I think there is a flaw in the pattern of putting Write-InvocationLogafter the $PSCmdlet.ShouldProcess() check...there are a number of functions that do validation work and will write error messages to the console/log file in the event that there was a problem. Those lines will all print without an logged invocation entry, which could both confusing as well as detrimental (no way to review the passed-in parameters).

I think that the $PSCmdlet.ShouldProcess() checks should remain where they are, but that the Write-InvocationLog entries should go back to their original spots.

[Simon Heather (X-Guardian)]

Howard Wolosky (@HowardWolosky), this is ready for the next review.

[Howard Wolosky (HowardWolosky)]

Thanks for your patience here and all the hard work. Ready to get this merged in now.

[Howard Wolosky (HowardWolosky)]

/azp run PowerShellForGitHub-CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Howard Wolosky (HowardWolosky)]

This change is causing the tests to fail right now, because we still have the check at line 157 that calls $PSCmdlet.ShouldProcess():

Invoke-SendTelemetryEvent' calls ShouldProcess/ShouldContinue but does not have the ShouldProcess attribute.
Stack trace
at line: 157 in /home/vsts/work/1/s/Telemetry.ps1
ShouldProcess
Severity: Warning

[Simon Heather (X-Guardian)]

Removed ShouldProcessBlock

[Howard Wolosky (HowardWolosky)]

/azp run PowerShellForGitHub-CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Howard Wolosky (HowardWolosky)]

There are 7 UT failures with this change (all 7 are consistently failing across all 4 targets, 2 additionals are one-off issues I believe).

All 7 are in the GitHubBranches.tests.ps1, most likely around usage of -WhatIf in Get-* functions...

Expected an exception, with message 'Origin branch Missing-Branch not found' to be thrown, but the message was 'A parameter cannot be found that matches parameter name 'Whatif'.'. from /home/vsts/work/1/s/GitHubBranches.ps1:300 char:53
at <ScriptBlock>, /home/vsts/work/1/s/Tests/GitHubBranches.tests.ps1: line 221
at <ScriptBlock>, /home/vsts/work/1/s/Tests/GitHubBranches.tests.ps1: line 235

[Simon Heather (X-Guardian)]

Howard Wolosky (@HowardWolosky), I've remove the redundant WhatIf/Confirm parameters. Can you re-run the CI?

[Howard Wolosky (HowardWolosky)]

/azp run PowerShellForGitHub-CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Howard Wolosky (HowardWolosky)]

Nice to get this in. Thanks so much for spearheading this effort, Simon Heather (@X-Guardian)!