PowerShellForGitHub: Suggested WhatIf/Confirm Processing Improvements

[X-Guardian]

Issue Details

Since PR #213, the ShouldProcess (WhatIf/Confirm) processing in the module is better, but it still could be improved.
The PowerShellForGuthub functions are designed to be consumed by other scripts/functions,
so the norm in this case is for each function to only ever output a single ShouldProcess prompt.
Currently, most functions output at least two prompts, for example:

PS> New-GitHubRepository -OrganizationName test -RepositoryName test -WhatIf
What if: Performing the operation "Output to File" on target "\\nas01\data\users\simon\documents\PowerShellForGitHub.log".
What if: Performing the operation "Invoke-WebRequest" on target "https://github.com/api/orgs/test/repos".

The WhatIf for the 'Output to File' should not be shown, and the second WhatIf has a non-useful operation and target. It should be: What if: Performing the operation "Create Repository" on target "test".

Also, if a function is given an invalid set of parameters, for example:

New-GitHubRepository -RepositoryName test -TeamId 1 -WhatIf
What if: Performing the operation "Output to File" on target "\\nas01\data\users\simon\documents\PowerShellForGitHub.log".
Exception: Z:\Users\Simon\Documents\GitHub\X-Guardian\PowerShellForGitHub\GitHubRepositories.ps1:144:9
Line |
 144 |          throw $message
     |          ~~~~~~~~~~~~~~
     | TeamId may only be specified when creating a repository under an organization.

The WhatIf for the 'Output to File' should not be shown.

Suggested solution to the issue

• Move the $PSCmdlet.ShouldProcess check out of Invoke-GHRestMethod and add it to the calling functions with the correct operation and target (as has been done for the ConfirmHighImpact PR Add confirmation prompts and examples for Remove- functions #174).
• Move the Write-InvocationLog -Invocation $MyInvocation line in each resource function to within the $PSCmdlet.ShouldProcess condition block. This will prevent the log being updated if ShouldProcess is $false.
• Add -WhatIf:$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.

This would also have the additional benefit of being able to remove the SuppressMessageAttribute PSShouldProcess decorator from all the functions, as the ShouldProcess logic will now be happening within the function.

Requested Assignment

• If possible, I would like to fix this.

Operating System

OsName               : Microsoft Windows 10 Pro
OsOperatingSystemSKU : 48
OsArchitecture       : 64-bit
WindowsVersion       : 1903
WindowsBuildLabEx    : 18362.1.amd64fre.19h1_release.190318-1202
OsLanguage           : en-GB
OsMuiLanguages       : {en-GB, en-US}

PowerShell Version

Name                           Value
----                           -----
PSVersion                      7.0.1
PSEdition                      Core
GitCommitId                    7.0.1
OS                             Microsoft Windows 10.0.18362
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Module Version

Running: 0.14.0
Installed: 0.14.0

[HowardWolosky]

Awesome write-up, @X-Guardian. The only place in the module with a call to Out-File is in Write-Log (Write-InvocationLog is just a wrapper on top of Write-Log). So, your -WhatIt:$false change would only need to happen there.

Beyond that, I'm ok with you proposal. My main request here would still to perform the check similar to how it's done in Invoke-GHRestMethod as opposed to the way that it's done right now in the Remove-* methods. I think code is more readable with early returns as opposed to further nested expressions due to the added if check.

Marking this as In Progress given its assignment to you.
Thanks again!

[X-Guardian]

Hi @HowardWolosky, yes you are right about Out-File, I have modified the description.

[X-Guardian]

I've just been looking through the functions and most if not all of the non-state changing functions (Get-*, Group-* etc) have SupportsShouldProcess enabled. This is not a standard pattern and SupportsShouldProcess is normally only set on state-changing functions (New-*, Set-*, Remove-* etc).

What do you think about also removing SupportsShouldProcess from the non-state changing functions?

[HowardWolosky]

What do you think about also removing SupportsShouldProcess from the non-state changing functions?

I think your instinct here is probably right. I checked a number of the built-in Get-* cmdlets, and none of them support WhatIf. On the one hand, even the Get-* methods are state changing because they result in a change to the log file, but that's probably an acceptable compromise to make.

Go for it.