PowerShellForGitHub: Improve and Standardise WhatIf/Confirm Processing

[Simon Heather (X-Guardian)]

This PR is superseded by PR #254

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.
• Moved the Write-InvocationLog line in each resource function to after the $PSCmdlet.ShouldProcess condition block. This will prevent the log being updated if ShouldProcess is $false.
• 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

[Simon Heather (X-Guardian)]

Ran the whole test suite for this PR, and the GitHubRepositoryForks test has deleted my fork of the PowerShellForGitHub repo, which includes all the branches for the pending PRs here including this one :(

[Howard Wolosky (HowardWolosky)]

Ran the whole test suite for this PR, and the GitHubRepositoryForks test has deleted my fork of the PowerShellForGitHub repo, which includes all the branches for the pending PRs here including this one :(

Oh no. I've immediately merged in #251 to avoid this in the future. So sorry about that.

[Simon Heather (X-Guardian)]

I've still got the branches locally, so no lost work, and I've pushed them back to my new fork of the repo, but the links to the open PRs are broken. PRs affected are:

• GitHubBranches: Add New/Remove-GitHubRepositoryBranch Functions #200 - Branch: https://github.com/X-Guardian/PowerShellForGitHub/tree/GitHubBranches-NewRemove
• GitHubTeams: Add New/Update/Remove Functions #222 - Branch: https://github.com/X-Guardian/PowerShellForGitHub/tree/GitHubTeams-Add-New-Remove
• New GitHubRepositoryBranchProtectionRule Functions #238 - Branch: https://github.com/X-Guardian/PowerShellForGitHub/tree/GitHubRepositoryBranchProtection
• PowerShellForGitHub: Improve and Standardise WhatIf/Confirm Processing #248 - Branch: https://github.com/X-Guardian/PowerShellForGitHub/tree/ShouldProcess-Update

Any idea how to fix the PRs?

[Howard Wolosky (HowardWolosky)]

Any idea how to fix the PRs?

Not sure yet. Doing some searching online. Will be able to look a bit more this afternoon.

[Howard Wolosky (HowardWolosky)]

I don't think you can.

https://stackoverflow.com/questions/36071272/fix-unknown-repository-of-an-opened-pr-after-deleted-the-fork
isaacs/github#168

I think you'll just need to create new pull requests for those outstanding ones, and then close out the ones that have now been orphaned.

[Howard Wolosky (HowardWolosky)]

Overall this looks good. Thanks for the effort.
A couple nits, and then a few that referenced the wrong value.

[Howard Wolosky (HowardWolosky)]

Actually, looking at this a bit more as full files as opposed to just the diffs, I think the one thing about the pattern that I'd change is that I'd move the check to the top of the functions as opposed to right before the call to Invoke-GHRestMethod*.

[Simon Heather (X-Guardian)]

Actually, looking at this a bit more as full files as opposed to just the diffs, I think the one thing about the pattern that I'd change is that I'd move the check to the top of the functions as opposed to right before the call to Invoke-GHRestMethod*.

If you are running a function with the WhatIf parameter, the main purpose is to validate that the set of parameters you have passed are valid. To achieve this, the $PSCmdlet.ShouldProcess statement needs to occur after all the parameter validation code.

[Howard Wolosky (HowardWolosky)]

If you are running a function with the WhatIf parameter, the main purpose is to validate that the set of parameters you have passed are valid. To achieve this, the $PSCmdlet.ShouldProcess statement needs to occur after all the parameter validation code.

Sufficiently compelling argument for me. Please keep as-is.

[Simon Heather (X-Guardian)]

This PR is superseded by PR #254