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

Impact Level #8157

Closed
kort3x opened this issue Oct 31, 2018 · 10 comments · Fixed by #8209

Comments

@kort3x
Copy link

commented Oct 31, 2018

I like the idea of impact levels for cmdlets.
What I dont like is that almost all stock cmdlets have "medium" as value for ConfirmImpact.

Get-Command -CommandType Cmdlet | 
  ForEach-Object { 
    $type = $_.ImplementingType
    if ($type -ne $null)
    {
      $type.GetCustomAttributes($true) | 
      Where-Object { $_.VerbName -ne $null } |
      Select-Object @{Name='Name';
      Expression={'{0}-{1}' -f $_.VerbName, $_.NounName}}, ConfirmImpact
    }
  } |
  Sort-Object ConfirmImpact -Descending

Remove-Item should behave differently from Get-Date.

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

Medium is the default when one doesn't spare the time to define a specific level. But I agree, the stock cmdlets should be more thorough in which has medium or low confirmImpact.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

If there are specific proposals on ConfirmImpact per cmdlet, we can review that.

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

these are the cmdlets that I'd personally expect to have a ConfirmImpact of Low, pilfered from the output of the command @kort3x wrote:

Name                              ConfirmImpact
----                              -------------
Compare-Object                           Medium
ConvertFrom-Csv                          Medium
ConvertFrom-Json                         Medium
ConvertFrom-Markdown                     Medium
ConvertFrom-StringData                   Medium
Convert-Path                             Medium
ConvertTo-Csv                            Medium
ConvertTo-Html                           Medium
ConvertTo-Json                           Medium
ConvertTo-Xml                            Medium
Export-ModuleMember                      Medium
ForEach-Object                           Medium
Format-Custom                            Medium
Format-Hex                               Medium
Format-List                              Medium
Format-Table                             Medium
Format-Wide                              Medium
Get-Alias                                Medium
Get-ChildItem                            Medium
Get-Command                              Medium
Get-ComputerInfo                         Medium
Get-Content                              Medium
Get-Culture                              Medium
Get-Date                                 Medium
Get-Event                                Medium
Get-EventSubscriber                      Medium
Get-ExperimentalFeature                  Medium
Get-FileHash                             Medium
Get-FormatData                           Medium
Get-Help                                 Medium
Get-History                              Medium
Get-Host                                 Medium
Get-Item                                 Medium
Get-ItemProperty                         Medium
Get-ItemPropertyValue                    Medium
Get-Job                                  Medium
Get-Location                             Medium
Get-MarkdownOption                       Medium
Get-Member                               Medium
Get-Module                               Medium
Get-Process                              Medium
Get-PSBreakpoint                         Medium
Get-PSCallStack                          Medium
Get-PSDrive                              Medium
Get-PSHostProcessInfo                    Medium
Get-PSProvider                           Medium
Get-PSReadLineKeyHandler                 Medium
Get-PSReadLineOption                     Medium
Get-PSSession                            Medium
Get-PSSessionCapability                  Medium
Get-PSSessionConfiguration               Medium
Get-Random                               Medium
Get-Runspace                             Medium
Get-RunspaceDebug                        Medium
Get-Service                              Medium
Get-TimeZone                             Medium
Get-TraceSource                          Medium
Get-TypeData                             Medium
Get-UICulture                            Medium
Get-Unique                               Medium
Get-Uptime                               Medium
Get-Variable                             Medium
Get-Verb                                 Medium
Group-Object                             Medium
Join-Path                                Medium
Measure-Command                          Medium
Measure-Object                           Medium
New-Alias                                Medium
New-Guid                                 Medium
New-Module                               Medium
New-ModuleManifest                       Medium
New-Object                               Medium
New-PSDrive                              Medium
New-PSRoleCapabilityFile                 Medium
New-PSSession                            Medium
New-PSSessionConfigurationFile           Medium
New-PSSessionOption                      Medium
New-PSTransportOption                    Medium
New-TemporaryFile                        Medium
New-TimeSpan                             Medium
New-Variable                             Medium
Out-Default                              Medium
Out-Host                                 Medium
Out-Null                                 Medium
Out-String                               Medium
Pop-Location                             Medium
Push-Location                            Medium
Read-Host                                Medium
Receive-Job                              Medium
Receive-PSSession                        Medium
Register-ArgumentCompleter               Medium
Resolve-Path                             Medium
Save-Help                                Medium
Select-Object                            Medium
Select-String                            Medium
Select-Xml                               Medium
Send-MailMessage                         Medium
Show-Markdown                            Medium
Sort-Object                              Medium
Split-Path                               Medium
Start-Sleep                              Medium
Tee-Object                               Medium
Test-Connection                          Medium
Test-Json                                Medium
Test-ModuleManifest                      Medium
Test-Path                                Medium
Test-PSSessionConfigurationFile          Medium
Trace-Command                            Medium
Update-FormatData                        Medium
Update-Help                              Medium
Update-TypeData                          Medium
Wait-Debugger                            Medium
Wait-Event                               Medium
Wait-Job                                 Medium
Wait-Process                             Medium
Where-Object                             Medium
Write-Debug                              Medium
Write-Error                              Medium
Write-Host                               Medium
Write-Information                        Medium
Write-Output                             Medium
Write-Progress                           Medium
Write-Verbose                            Medium
Write-Warning                            Medium

And the following command probably ought to be High by default:

Set-StrictMode                           Medium
@vexx32

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

I think part of the problem with having so few actual Low-ranked cmdlets in terms of ConfirmImpact is that having any set value for $ConfirmPreference other than High quickly becomes unusable, as almost every standard command used may suddenly start triggering prompts, but in fact they either a) do not properly implement ShouldProcess anyway, or b) their impact should be ranked significantly lower than it is set at.

I would contend that there is a good degree of value in having properly granular ConfirmImpact "recommendations" as it were, as a quick method of determining if I execute X cmdlet, am I risking system modifications I may not want?, and should I be cautious and make use of -WhatIf?

This would make setting a $ConfirmPreference = 'Medium' a viable method to ensure that any concrete changes to the system state is accompanied by a prompt, but simply retrieving and working with data in the console is not, as it should (hopefully) not cause any unexpected changes in system state.

Other Considerations

I am of two minds when it comes to creating new items and similar actions on the system; on the one hand, if a path is provided that is not occupied, there is very low risk of actually impacting system function. But, if an existing path is specified, it could overwrite the file and should prompt accordingly.

I almost feel as though we need a more dynamic way to alter the current ConfirmImpact of a cmdlet in certain cases; a cmdlet-level attribute doesn't seem sufficient when it make be capable of, say, two or three slightly different actions, with different levels of severity (e.g., New-Item can either create a new file in a blank space, provided the permissions in that location allow it — Low impact, in my opinion — OR it can overwrite a file with a blank one — requires use of -Force, from memory? — would be a Medium impact, in my opinion).

Perhaps we ought to modify the ShouldProcess() method to allow specifying the ConfirmImpact on a per-action basis, and it can take its default value from the cmdlet attribute property.

Ref: PowerShell/PowerShell-RFC#149

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@PowerShell/powershell-committee reviewed this, agree with list except: Update-Help should stay medium as it can affect system, Set-StrictMode should also stay medium since it only affects the session and not system state. The ConfirmImpact is documented as impact to system. Should also consider changing default for Get- cmdlets to low if not specified.

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

I almost mentioned that about Get- cmdlets myself, actually. Sounds good! If you guys don't mind, I'll be happy to pick up the menial labor of adding the appropriate levels to the cmdlets discussed.

Not sure how I'd go about changing the default just for Get-* verbed cmdlets (should that extend to advanced functions?) but it might be fun to give that a look as well.

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

@SteveL-MSFT from what I'm seeing on these commands, some most of them did not implement ShouldProcess at all.

So, really, the main problem here is that querying a command's ConfirmImpact incorrectly returns the default ConfirmImpact.Medium value instead of ConfirmImpact.None as might reasonably be expected for commands that don't set SupportsShouldProcess = true. I will look to make this change in as minimally-impacting way I can, and then check the other cmdlets you pointed out to see if there are other changes needed.

Definitely reduces the scope of changes necessary, though it's interesting.

@kort3x

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

@PowerShell/powershell-committee reviewed this, agree with list except: Update-Help should stay medium as it can affect system, Set-StrictMode should also stay medium since it only affects the session and not system state. The ConfirmImpact is documented as impact to system. Should also consider changing default for Get- cmdlets to low if not specified.

I agree that all get- cmdlets should return low

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

@PowerShell/powershell-committee re-reviewed this. The concern is that although we can change the default for C# cmdlets to be low for Get- cmdlets, it is much more difficult from an engineering perspective to support this for advanced functions. Our decision is that we should explicitly state the ConfirmImpact for cmdlets shipped with PSCore6, but leave the default to be medium if not specified (regardless if Get-) so that it is predictable and consistent.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jan 26, 2019

Our decision is that we should explicitly state the ConfirmImpact for cmdlets shipped with PSCore6,

Should we have a tracking issue for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.