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

vexx32 · 2018-11-08T05:30:12Z

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

Further discussion may be warranted as to whether there is missing ShouldProcess support, or whether some commands may want to be increased in severity.
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

Have cmdlets with Get verb by default set ConfirmImpact to Low

per @PowerShell/powershell-committee recommendation

iSazonov · 2018-11-08T05:45:09Z

@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 · 2018-11-08T13:14:55Z

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

iSazonov · 2018-11-08T14:47:23Z

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

vexx32 · 2018-11-08T17:45:25Z

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 · 2018-11-08T21:49:57Z

@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 · 2018-11-09T03:28:16Z

@vexx32 See my answer in #8214.

stale · 2018-12-16T09:17:18Z

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.

vexx32 · 2018-12-16T12:16:49Z

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.

vexx32 · 2018-12-21T03:03:08Z

@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.

iSazonov · 2018-12-21T12:20:23Z

test/powershell/engine/Basic/DefaultCommands.Tests.ps1

@@ -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"


Could you please clarify why does the change need?

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.

Thanks! Please add this to the PR description.

daxian-dbw

Looks good to me except one comment :)

daxian-dbw · 2019-01-10T23:06:32Z

src/System.Management.Automation/engine/Attributes.cs

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


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.

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-*?

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?

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.

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. 🙂

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

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

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

I'll take care of it tomorrow. 🙂

iSazonov · 2019-01-19T06:09:02Z

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

Do we need documentation issue?

vexx32 · 2019-01-19T11:58:09Z

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

LGTM. Thanks @vexx32 !

daxian-dbw · 2019-01-26T11:04:59Z

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

vexx32 added 2 commits November 7, 2018 23:46

Make ConfirmImpact = None for cmdlets not implementing ShouldProcess

f08c417

Have cmdlets with Get verb by default set ConfirmImpact to Low

Modify existing cmdlet confirmImpacts

f89a1e1

per @PowerShell/powershell-committee recommendation

vexx32 requested review from adityapatwardhan, BrucePay, daxian-dbw, JamesWTruher, mirichmo and PaulHigin as code owners November 8, 2018 05:30

Address CodeFactor gripe with formatting

55ac75f

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

vexx32 mentioned this pull request Nov 8, 2018

Add Tests for ConfirmImpact Ratings #8214

Merged

11 tasks

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 16, 2018

stale bot added the Stale label Dec 16, 2018

stale bot removed the Stale label Dec 16, 2018

vexx32 added 3 commits December 20, 2018 20:57

Merge branch 'master' into Fix-ShouldProcess-ConfirmImpact

d861f95

Fix test condition and provide meaningful output from differing entries

3f547df

Update DefaultCommands with current state of ConfirmImpact ratings

44c5856

iSazonov reviewed Dec 21, 2018

View reviewed changes

iSazonov assigned daxian-dbw Dec 21, 2018

daxian-dbw reviewed Jan 10, 2019

View reviewed changes

📝 Address review comments

8dca78c

Fix indentation

d8beede

daxian-dbw approved these changes Jan 26, 2019

View reviewed changes

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

vexx32 deleted the Fix-ShouldProcess-ConfirmImpact branch June 13, 2020 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vexx32 commented Nov 8, 2018 •

edited

Loading

iSazonov commented Nov 8, 2018

vexx32 commented Nov 8, 2018

iSazonov commented Nov 8, 2018 •

edited

Loading

vexx32 commented Nov 8, 2018

vexx32 commented Nov 8, 2018

iSazonov commented Nov 9, 2018

stale bot commented Dec 16, 2018

vexx32 commented Dec 16, 2018 •

edited

Loading

vexx32 commented Dec 21, 2018

iSazonov Dec 21, 2018

vexx32 Dec 21, 2018 •

edited

Loading

iSazonov Dec 21, 2018

daxian-dbw left a comment

daxian-dbw Jan 10, 2019

vexx32 Jan 11, 2019 •

edited

Loading

daxian-dbw Jan 11, 2019 •

edited

Loading

iSazonov Jan 11, 2019 •

edited

Loading

vexx32 Jan 12, 2019

SteveL-MSFT Jan 16, 2019

daxian-dbw Jan 17, 2019

vexx32 Jan 17, 2019

iSazonov commented Jan 19, 2019

vexx32 commented Jan 19, 2019

daxian-dbw left a comment

daxian-dbw commented Jan 26, 2019

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

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

Conversation

vexx32 commented Nov 8, 2018 • edited Loading

PR Summary

Following PR

PR Checklist

iSazonov commented Nov 8, 2018

vexx32 commented Nov 8, 2018

iSazonov commented Nov 8, 2018 • edited Loading

vexx32 commented Nov 8, 2018

vexx32 commented Nov 8, 2018

iSazonov commented Nov 9, 2018

stale bot commented Dec 16, 2018

vexx32 commented Dec 16, 2018 • edited Loading

vexx32 commented Dec 21, 2018

iSazonov Dec 21, 2018

Choose a reason for hiding this comment

vexx32 Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

iSazonov Dec 21, 2018

Choose a reason for hiding this comment

daxian-dbw left a comment

Choose a reason for hiding this comment

daxian-dbw Jan 10, 2019

Choose a reason for hiding this comment

vexx32 Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

daxian-dbw Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

iSazonov Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

vexx32 Jan 12, 2019

Choose a reason for hiding this comment

SteveL-MSFT Jan 16, 2019

Choose a reason for hiding this comment

daxian-dbw Jan 17, 2019

Choose a reason for hiding this comment

vexx32 Jan 17, 2019

Choose a reason for hiding this comment

iSazonov commented Jan 19, 2019

vexx32 commented Jan 19, 2019

daxian-dbw left a comment

Choose a reason for hiding this comment

daxian-dbw commented Jan 26, 2019

vexx32 commented Nov 8, 2018 •

edited

Loading

iSazonov commented Nov 8, 2018 •

edited

Loading

vexx32 commented Dec 16, 2018 •

edited

Loading

vexx32 Dec 21, 2018 •

edited

Loading

vexx32 Jan 11, 2019 •

edited

Loading

daxian-dbw Jan 11, 2019 •

edited

Loading

iSazonov Jan 11, 2019 •

edited

Loading