Computer cmdlets should fail with error when not run via sudo on Unix by SteveL-MSFT · Pull Request #19824 · PowerShell/PowerShell

SteveL-MSFT · 2023-06-19T22:01:32Z

PR Summary

Computer cmdlets rely on shutdown command to work and doesn't have error handling when it fails so when run without root permissions it silently fails. Change is to check the exit code and emit the stderr message. Also changed to use CommandDiscovery to find shutdown instead of a hardcoded path.

PR Context

Fix #19802

PR Checklist

daxian-dbw · 2023-06-19T22:32:03Z

@SteveL-MSFT Did you take a look at #19780 and see if it tries to fix the same?

SteveL-MSFT · 2023-06-26T19:31:09Z

@daxian-dbw the two PRs are complimentary and addressing different issues

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

test/powershell/Modules/Microsoft.PowerShell.Management/Restart-Computer.Tests.ps1

…es to resx

src/Microsoft.PowerShell.Commands.Management/resources/ComputerResources.resx

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

src/Microsoft.PowerShell.Commands.Management/resources/ComputerResources.resx

test/powershell/Modules/Microsoft.PowerShell.Management/Restart-Computer.Tests.ps1

ghost · 2023-07-10T14:01:01Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

…nt/ComputerUnix.cs Co-authored-by: Ilya <darpa@yandex.ru>

…rResources.resx Co-authored-by: Ilya <darpa@yandex.ru>

JamesWTruher

there's a potential collision between this and #19780

test/powershell/Modules/Microsoft.PowerShell.Management/Restart-Computer.Tests.ps1

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

src/Microsoft.PowerShell.Commands.Management/resources/ComputerResources.resx

…nt/ComputerUnix.cs Co-authored-by: Dongbo Wang <dongbow@microsoft.com>

…rResources.resx Co-authored-by: Dongbo Wang <dongbow@microsoft.com>

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

…nt/ComputerUnix.cs

pull-request-quantifier-deprecated · 2023-07-17T19:06:08Z

This PR has 52 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Small
Size       : +42 -10
Percentile : 20.8%

Total files changed: 5

Change summary by file extension:
.cs : +27 -7
.resx : +3 -3
.ps1 : +12 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

daxian-dbw

LGTM

Computer cmdlets should fail with error when not run via sudo on Unix

3e3a51c

SteveL-MSFT requested review from adityapatwardhan and daxian-dbw as code owners June 19, 2023 22:01

pull-request-quantifier-deprecated bot added the Extra Small label Jun 19, 2023

SteveL-MSFT assigned adityapatwardhan Jun 19, 2023

StevenBucher98 added the PowerShell-Docs not needed The PR was reviewed and doesn't appear to require a PowerShell Docs update label Jun 26, 2023

SteveL-MSFT added 6 commits June 26, 2023 14:01

add tracing for test failure

59aa58b

fix test to skip if running as root

97210bc

add test tracing to see if running as root

c34bbe0

change to use group id and add more traces

9a6c5a6

add back tracing of error

df0337f

clear the error var

488877e

iSazonov reviewed Jun 27, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs Outdated Show resolved Hide resolved

explicitly use out-string for error

2553820

iSazonov reviewed Jun 27, 2023

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Management/Restart-Computer.Tests.ps1 Outdated Show resolved Hide resolved

SteveL-MSFT added 3 commits June 27, 2023 13:26

add additional path to search for shutdown command, move error messag…

f9b68e4

…es to resx

trace if shutdown is found

c4c8870

add path to output

f78c1ee

iSazonov reviewed Jun 28, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/resources/ComputerResources.resx Outdated Show resolved Hide resolved

update RunCommand for shutdown, remove unneeded string

cd1937c

SteveL-MSFT requested a review from anmenaga as a code owner July 1, 2023 19:34

pull-request-quantifier-deprecated bot added Small and removed Extra Small labels Jul 1, 2023

iSazonov reviewed Jul 3, 2023

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label Jul 10, 2023

SteveL-MSFT and others added 2 commits July 10, 2023 09:46

Update src/Microsoft.PowerShell.Commands.Management/commands/manageme…

72e4b99

…nt/ComputerUnix.cs Co-authored-by: Ilya <darpa@yandex.ru>

Update src/Microsoft.PowerShell.Commands.Management/commands/manageme…

2426a18

…nt/ComputerUnix.cs Co-authored-by: Ilya <darpa@yandex.ru>

SteveL-MSFT and others added 5 commits July 10, 2023 10:10

Update src/Microsoft.PowerShell.Commands.Management/resources/Compute…

c8fe455

…rResources.resx Co-authored-by: Ilya <darpa@yandex.ru>

address Ilya's feedback

63daba6

change to using command discovery

f9a04ba

skip test if shutdown command is not available

f347ffb

fix test

6fd2b60

JamesWTruher reviewed Jul 17, 2023

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Management/Restart-Computer.Tests.ps1 Show resolved Hide resolved

daxian-dbw reviewed Jul 17, 2023

View reviewed changes

SteveL-MSFT and others added 2 commits July 17, 2023 10:17

Update src/Microsoft.PowerShell.Commands.Management/commands/manageme…

69f4c07

…nt/ComputerUnix.cs Co-authored-by: Dongbo Wang <dongbow@microsoft.com>

Update src/Microsoft.PowerShell.Commands.Management/resources/Compute…

702093c

…rResources.resx Co-authored-by: Dongbo Wang <dongbow@microsoft.com>

daxian-dbw added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Jul 17, 2023

microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 17, 2023

address Dongbo's feedback

95594d7

daxian-dbw reviewed Jul 17, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs Outdated Show resolved Hide resolved

Update src/Microsoft.PowerShell.Commands.Management/commands/manageme…

57808f5

…nt/ComputerUnix.cs

daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jul 17, 2023

daxian-dbw approved these changes Jul 17, 2023

View reviewed changes

daxian-dbw merged commit 5a6c66b into PowerShell:master Jul 17, 2023

SteveL-MSFT deleted the unix-computer-cmdlet branch July 17, 2023 21:38

Conversation

SteveL-MSFT commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

daxian-dbw commented Jun 19, 2023

Uh oh!

SteveL-MSFT commented Jun 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Jul 10, 2023

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pull-request-quantifier-deprecated bot commented Jul 17, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SteveL-MSFT commented Jun 19, 2023 •

edited

Loading