Skip to content

Add an implementation of Stop-Computer for Linux and macOS #11151

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.

Sign up for GitHub

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

Jump to bottom
Merged
TravisEz13 merged 1 commit into from
Dec 3, 2019
Merged

Add an implementation of Stop-Computer for Linux and macOS #11151

TravisEz13 merged 1 commit into from
Dec 3, 2019

Conversation

@brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Nov 22, 2019 •
edited by SteveL-MSFT
Loading

PR Summary

Adds an implementation of the Stop-Computer cmdlet for Linux and OS X

PR Context

Closes #5448

PR Checklist

@brendandburns brendandburns force-pushed the stop branch 4 times, most recently from 55c7dbb to db7bb30 Compare November 22, 2019 05:39
@brendandburns
Copy link
Contributor Author

brendandburns commented Nov 22, 2019

I'm pretty sure the Windows CI run is a flake.

TravisEz13
TravisEz13 reviewed Nov 22, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Management/Microsoft.PowerShell.Commands.Management.csproj Outdated
Copy link
Member

@TravisEz13 TravisEz13 Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't prevent cross compilation. Pass in the property from the build.psm1. You can then default the property if it is not set from the commandline.

Copy link
Member

@TravisEz13 TravisEz13 Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:

PowerShell/PowerShell.Common.props

Line 115 in c17d5a7

<IsWindows Condition="'$(IsWindows)' =='true' or ( '$(IsWindows)' == '' and '$(OS)' == 'Windows_NT')">true</IsWindows>

PowerShell/build.psm1

Line 347 in c17d5a7

$Arguments += "/property:IsWindows=true"

Copy link
Contributor Author

@brendandburns brendandburns Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to be a runtime thing.

TravisEz13
TravisEz13 requested changes Nov 22, 2019
View reviewed changes
Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 22, 2019
TravisEz13
TravisEz13 reviewed Nov 22, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs Outdated
Copy link
Member

@TravisEz13 TravisEz13 Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason start-process won't work?

Copy link
Member

@SteveL-MSFT SteveL-MSFT Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean the C# Process class

Copy link
Member

@TravisEz13 TravisEz13 Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, here is an example:

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

Line 19 in 9851b07

private static string StartProcess(

Copy link
Contributor Author

@brendandburns brendandburns Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

TravisEz13
TravisEz13 reviewed Nov 22, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs Outdated
Copy link
Member

@TravisEz13 TravisEz13 Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-P is not an option on the macOS shutdown

Copy link
Contributor Author

@brendandburns brendandburns Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

TravisEz13
TravisEz13 reviewed Nov 22, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs Outdated
Copy link
Member

@TravisEz13 TravisEz13 Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we handle it when the user doesn't have permissions to shutdown?

Copy link
Contributor Author

@brendandburns brendandburns Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command will error out and we (now) write the error code to the stream.

@TravisEz13 TravisEz13 self-assigned this Nov 22, 2019
@TravisEz13
Copy link
Member

TravisEz13 commented Nov 22, 2019

@brendandburns I had the CI system rerun the flaky tests and filed: #11168

PaulHigin
PaulHigin suggested changes Nov 22, 2019
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. I would like to see Restart-Computer also be enabled for non-Windows platforms.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 25, 2019
@brendandburns brendandburns force-pushed the stop branch 5 times, most recently from 8afdfc1 to 1be3fb8 Compare November 25, 2019 21:35
@brendandburns
Copy link
Contributor Author

brendandburns commented Nov 25, 2019

Comments addressed, please re-check.

@brendandburns brendandburns force-pushed the stop branch 2 times, most recently from 90c6695 to 919fc84 Compare November 25, 2019 22:02
@brendandburns
Copy link
Contributor Author

brendandburns commented Nov 26, 2019

comments addressed, please take another look.

PaulHigin
PaulHigin approved these changes Nov 26, 2019
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

SteveL-MSFT
SteveL-MSFT requested changes Nov 26, 2019
View reviewed changes
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 26, 2019
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 27, 2019
@brendandburns
Copy link
Contributor Author

brendandburns commented Nov 27, 2019

@SteveL-MSFT comments addressed, please take another look.

Thanks!

SteveL-MSFT
SteveL-MSFT approved these changes Nov 28, 2019
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brendandburns
Copy link
Contributor Author

brendandburns commented Dec 3, 2019

@TravisEz13 any chance you can take another look here, I'd love to see this merged since I have a follow up PR to share once it is merged.

@TravisEz13
Copy link
Member

TravisEz13 commented Dec 3, 2019

@brendandburns Sorry, I've been out of the office.

@TravisEz13 TravisEz13 added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 3, 2019
@TravisEz13 TravisEz13 added this to the 7.1.0-preview.1 milestone Dec 3, 2019
@TravisEz13 TravisEz13 changed the title Add an implementation of Stop-Computer for Linux and OS X. Add an implementation of Stop-Computer for Linux and macOS Dec 3, 2019
@TravisEz13 TravisEz13 merged commit d38541f into PowerShell:master Dec 3, 2019
@brendandburns brendandburns mentioned this pull request Dec 11, 2019
Merged
7 tasks
@ghost
Copy link

ghost commented Mar 26, 2020

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Mar 26, 2020
Closed
@vexx32 vexx32 mentioned this pull request Apr 24, 2020
Closed
@chasewilson chasewilson mentioned this pull request May 20, 2020
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

+1 more reviewer

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TravisEz13 TravisEz13

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.1

Development

Successfully merging this pull request may close these issues.

Missing Stop-Computer cmdlet

4 participants

@brendandburns @TravisEz13 @SteveL-MSFT @PaulHigin