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

ConfirmImpact: Correctly Report impact level when SupportsShouldProcess = false #8209

Merged
merged 8 commits into from Jan 26, 2019

Conversation

@vexx32
Copy link
Contributor

commented Nov 8, 2018

PR Summary

Resolves #8157

This change reflects the cmdlets' actual defined attributes when querying a cmdlet's or function's defined ConfirmImpact value. Tests have been updated to reflect the true values now reported.

Prior to this PR they would report as having "Medium" impact when querying the attribute's set ConfirmImpact as that is the default value, despite not implementing ShouldProcess in the first place nor declaring support for it.

This is primarily a change in how this value reports when SupportsShouldProcess is not set to true for a given command, in the interest of visibility for the actual effective ConfirmImpact level of a cmdlet.

All changes from Medium to None are reflective of this. The only cmdlets that had an actual effective ConfirmImpact change are those that have been changed to Low per committee decision, and are all listed in the PR description.

It is my hope that by making plain the true effective implementations of ShouldProcess support in cmdlets and the effective ConfirmImpact values here that we can have a concrete reference point to look at and check which cmdlets should have ShouldProcess support and do not, or have an inappropriate ConfirmImpact value.

Per @PowerShell/powershell-committee's recommendation, the following cmdlets have additionally had their ConfirmImpact ratings set to Low:

  • New-PSDrive
  • New-Alias
  • New-TemporaryFile
  • Update-TypeData
  • Update-FormatData
  • New-Variable
  • ForEach-Object
  • New-ModuleManifest
  • Receive-PSSession

Note: The above change with the ConfirmImpact reporting highlighted that there were many discussed commands that simply do not currently implement ShouldProcess support.

The full list of commands loaded in a default session and their current ConfirmImpact ratings are detailed in these gists:

Following PR

  1. Further discussion may be warranted as to whether there is missing ShouldProcess support, or whether some commands may want to be increased in severity.
  2. As touched upon in the related issue, we may want to modify the default setting of ConfirmImpact according to the chosen verb of a cmdlet. This would only make a difference if the function or cmdlet explicitly sets SupportsShouldProcess = true without setting a ConfirmImpact. Some possible considerations that I think seem intuitive at the present moment:
    • Get -> ConfirmImpact.Low
    • Format -> ConfirmImpact.Low
    • Convert / ConvertTo / ConvertFrom -> ConfirmImpact.Low
    • Import / Export -> ConfirmImpact.Low
    • Join -> ConfirmImpact.Low
    • Measure -> ConfirmImpact.Low
    • Out -> ConfirmImpact.Low
    • Remove -> ConfirmImpact.High
    • Select -> ConfirmImpact.Low
    • Show -> ConfirmImpact.Low
    • Stop -> ConfirmImpact.High
    • Test -> ConfirmImpact.Low
    • Wait -> ConfirmImpact.Low
    • Write -> ConfirmImpact.Low

PR Checklist

vexx32 added some commits Nov 8, 2018

Make ConfirmImpact = None for cmdlets not implementing ShouldProcess
Have cmdlets with Get verb by default set ConfirmImpact to Low
Modify existing cmdlet confirmImpacts
per @PowerShell/powershell-committee recommendation

@vexx32 vexx32 changed the title ConfirmImpact: Correctly Report ConfirmImpact when SupportsShouldProcess = false ConfirmImpact: Correctly Report impact level when SupportsShouldProcess = false Nov 8, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 8, 2018

@vexx32 Please create new ConfirmImpact test for all cmdlets in the repo. I mean that the test should check only current values of ConfirmImpact. It should be another PR and we should merge it before the PR. Then in the PR we'll see changes we'll do.

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@iSazonov can you advise the most appropriate location for those tests to be placed? 🙂

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 8, 2018

Perhaps near DefaultCommands.Tests.ps1 would be good. DefaultConfirmImpacts.Tests.ps1 or something like.
And please use the same pattern for clarity.

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

Note: Currently failing test is due to test that expects ConfirmImpact to be reported for a cmdlet that is defined with ConfirmImpact = 'High' but does not set SupportsShouldProcess. I will fix this test also in the test-focused PR suggested by @iSazonov.

@vexx32 vexx32 referenced this pull request Nov 8, 2018
6 of 11 tasks complete
@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@iSazonov I'm having difficulty testing this effectively. It seems Get-Command is not registering the default modules as present at test time, it would seem. Tried a few different ways to go, but it seems the point where these tests get executed does not have the modules the default commands are packaged in.

Would this have to be moved to an xunit test, or do we have some other possible way to test for these? See #8214

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 9, 2018

@vexx32 See my answer in #8214.

@iSazonov iSazonov added the CL-General label Nov 16, 2018

@stale

This comment has been minimized.

Copy link

commented Dec 16, 2018

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Dec 16, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2018

Bump. Test for this are in #8214, ready to go. 😄

Once that gets merged in I'll pull in those changes and assess exactly which cmdlets register differently with this change.

@stale stale bot removed the Stale label Dec 16, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@iSazonov @SteveL-MSFT

I noted that while testing these changes the test I had previously constructed didn't provide particularly useful output, and changed it to also -PassThru the objects that it compares so that we can actually see the commands it's comparing and the differences properly.

I tested this locally and it passes, so hopefully the CI should pass as well. I noted as I went through there were still a few commands that do have a properly-implemented Medium ShouldProcess level declared. If you like, I can run through and point out the ones that probably should have a Low impact by my estimation and you guys can run thru and approve or deny any that shouldn't be changed?

There were also a couple as I ran through that I side-eyed and went "really, that didn't implement ShouldProcess?" so there may be a few of those kicking about too. Hopefully most of those ones should be pretty obvious in the diff display, though.

@@ -173,10 +173,10 @@ Describe "Verify approved aliases list" -Tags "CI" {
"Alias", "write", "Write-Output", $($FullCLR -or $CoreWindows ), "ReadOnly", "", ""
"Cmdlet", "Add-Computer", "", $($FullCLR ), "", "", ""
"Cmdlet", "Add-Content", "", $($FullCLR -or $CoreWindows -or $CoreUnix), "", "", "Medium"
"Cmdlet", "Add-History", "", $($FullCLR -or $CoreWindows -or $CoreUnix), "", "", "Medium"
"Cmdlet", "Add-Member", "", $($FullCLR -or $CoreWindows -or $CoreUnix), "", "", "Medium"
"Cmdlet", "Add-History", "", $($FullCLR -or $CoreWindows -or $CoreUnix), "", "", "None"

This comment has been minimized.

Copy link
@iSazonov

iSazonov Dec 21, 2018

Collaborator

Could you please clarify why does the change need?

This comment has been minimized.

Copy link
@vexx32

vexx32 Dec 21, 2018

Author Contributor

This change reflects the cmdlets actual defined attributes.

Prior to this PR they would report as having "Medium" impact when querying the attribute's set ConfirmImpact as that is the default value, despite not implementing ShouldProcess in the first place nor declaring support for it.

This is primarily a change in how this value reports when SupportsShouldProcess is not set to true for a given command, in the interest of visibility for the actual effective ConfirmImpact level of a cmdlet.

All changes from Medium to None are reflective of this. The only cmdlets that had an actual effective ConfirmImpact change are those that have been changed to Low per committee decision, and are all listed in the PR description.

It is my hope that by making plain the true effective implementations of ShouldProcess support in cmdlets and the effective ConfirmImpact values here that we can have a concrete reference point to look at and check which cmdlets should have ShouldProcess support and do not, or have an inappropriate ConfirmImpact value.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Dec 21, 2018

Collaborator

Thanks! Please add this to the PR description.

@daxian-dbw
Copy link
Member

left a comment

Looks good to me except one comment :)

if (VerbName == VerbsCommon.Get)
{
ConfirmImpact = ConfirmImpact.Low;
}

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jan 10, 2019

Member

This would cause inconsistency between cmdlet and advanced function. Say for the below function, I will see the ConfirmImpact value to be medium.

function Get-Something
{
    [CmdletBinding(SupportShouldProcess = true)]
    param()
    ...
}

From the committee review:

Should also consider changing default for Get- cmdlets to low if not specified.

I suggest we don't do this, so all Get command will return None for ConfirmImpact unless the command declares ConfirmImpact itself.

This comment has been minimized.

Copy link
@vexx32

vexx32 Jan 11, 2019

Author Contributor

Yeah, we should probably avoid disparity between cmdlets and advanced functions if we can.

I read the committee's comment there as "we should consider making Get- cmdlets return Low for ConfirmImpact if and only if SupportsShouldProcess = true and they do not specify a ConfirmImpact explicitly." -- maybe I was misinterpreting a bit?

And that's what this does. Technically it would always set Get cmdlets to Low internally, but nothing will ever register it unless SupportsShouldProcess = true thanks to the gated getter I added.

That said -- should I revert this to avoid disparity between cmdlets and advanced functions, or is there something I could do to mirror this effect for advanced functions named as Get-*?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jan 11, 2019

Member

The committee's comment is "we should consider ..." and we did consider it 😄 However, doing so will cause disparity between cmdlets and advanced functions, and there doesn't seem to be a simple way to make advanced functions do the same, as there is no way to know the verb within CmdletBindingAttribute.
I'm not sure what is the right trade-off in this case -- revert this change so Get-x cmdlets with SupportsShouldProcess = true shows Medium if ConfirmImpact is not specified? or accept the disparity between cmdlets and advanced functions?

@SteveL-MSFT @BrucePay @JamesWTruher @joeyaiello Can you please weigh in?

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jan 11, 2019

Collaborator

there doesn't seem to be a simple way to make advanced functions do the same, as there is no way to know the verb within CmdletBindingAttribute.

I think it is enough to indicate in the documentation that is the default for advanced functions and that it is necessary to explicitly indicate ConfirmImpact for Get verb. We could enhance ScriptAnalyzer too. Also we don't break advanced functions and user experience.

This comment has been minimized.

Copy link
@vexx32

vexx32 Jan 12, 2019

Author Contributor

Indeed. I think this change makes sense... but I'm not entirely sure this is the appropriate time to apply such a change. Of course, in an ideal world, cmdlet authors would just specify their own ConfirmImpact in every case, and I would generally think that should be the recommendation anyway.

I'd hope this is just a general catch-all for the occasional circumstance where it's omitted... but it may well have a wider impact than I'm thinking, so I would love to hear from the other team members as well. 🙂

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jan 16, 2019

Member

@PowerShell/powershell-committee re-reviewed the issue and agrees NOT to change the default

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jan 17, 2019

Member

@vexx32 given the new comment from committee review #8157 (comment), can you please revert the following changes?

if (VerbName == VerbsCommon.Get)
{
    ConfirmImpact = ConfirmImpact.Low;
}

This comment has been minimized.

Copy link
@vexx32

vexx32 Jan 17, 2019

Author Contributor

I'll take care of it tomorrow. 🙂

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jan 19, 2019

@vexx32 Please update the PR description (if needed).

Do we need documentation issue?

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2019

I don't think this change affects any documentation, unless the handful of cmdlets that did get changes in their ConfirmImpact mention the impact level specifically.

The rest of this change is likely to be largely invisible unless you're specifically looking for this data. PR description is OK, I think it's alright to leave the comments about possible follow up in case someone needs to refer to them. The description of the change remains accurate. 😄

@daxian-dbw
Copy link
Member

left a comment

LGTM. Thanks @vexx32 !

@daxian-dbw

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

Codacy's static analysis for this PR is incorrect and should be ignored.

@daxian-dbw daxian-dbw merged commit db1b309 into PowerShell:master Jan 26, 2019

7 of 8 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
CodeFactor 305 issues fixed. 12 issues found.
Details
PowerShell-CI-linux #PR-8209-20190126.01 succeeded
Details
PowerShell-CI-macos #PR-8209-20190126.01 succeeded
Details
PowerShell-CI-static-analysis #PR-8209-20190126.01 succeeded
Details
PowerShell-CI-windows #PR-8209-20190126.01 succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.