Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upFix Set-Service -Status Stopped runs failed on the service with dependencies #5525
Conversation
zhenggu
requested review from
adityapatwardhan and
daxian-dbw
as
code owners
Nov 22, 2017
This comment has been minimized.
This comment has been minimized.
msftclas
commented
Nov 22, 2017
•
iSazonov
reviewed
Nov 22, 2017
|
@mklement0 Could you please review the PR? |
|
|
||
| if ((servicedependedon != null) && (servicedependedon.Length > 0)) | ||
| { | ||
| WriteNonTerminatingError(service, null, "ServiceIsDependentOnNoForce", ServiceResources.ServiceIsDependentOnNoForce, ErrorCategory.InvalidOperation); |
This comment has been minimized.
This comment has been minimized.
iSazonov
Nov 22, 2017
Collaborator
We should remove the resource string from RESX file it is no longer used.
| @@ -1784,24 +1793,17 @@ protected override void ProcessRecord() | |||
| { | |||
| if (!service.Status.Equals(ServiceControllerStatus.Stopped)) | |||
| { | |||
| //check for the dependent services as set-service dont have force parameter | |||
| //check for the dependent services | |||
This comment has been minimized.
This comment has been minimized.
zhenggu
added some commits
Nov 22, 2017
This comment has been minimized.
This comment has been minimized.
|
I'm curious if this is even the right direction. Doesn't |
This comment has been minimized.
This comment has been minimized.
|
We have a similar question in #5471 |
This comment has been minimized.
This comment has been minimized.
|
I think it is necessary to remove the ServicesDependedOn check for it will block the normal function. and for parameter "Force", we can discuss it in other thread, if the parameter may be not acceptable. |
This comment has been minimized.
This comment has been minimized.
|
@SteveL-MSFT Could you please review the PR with #5471 on PowerShell committee? |
This comment has been minimized.
This comment has been minimized.
|
Yes,
On the flip side, it currently doesn't support As for the issue at hand: Preventing a service from stopping because it depends on other services - as opposed to having other services depend on it (having dependents) - is an arbitrary and nonsensical restriction. I presume it was introduced based on a misconception and should therefore be removed, which will make |
This comment has been minimized.
This comment has been minimized.
|
@mklement0 Thanks for useful comments!
Design review needed. |
This comment has been minimized.
This comment has been minimized.
Yea, but stopping a service is NOT setting a service. We have specific verbs for that kind of action. if there is a current bug in how it operates, that should be fixed, but adding On your second point about I'm just trying to keep things simple here and it seems like this PR is going in the wrong direction from simple. There is nothing wrong with doing something like |
This comment has been minimized.
This comment has been minimized.
|
I don't why it was decided to provide duplicate functionality in
Adding a Yes, you can argue that the
And, finally, another aspect in which |
This comment has been minimized.
This comment has been minimized.
No, it doesn't at all. Tell me, absent Edit: as a compromise, if everyone really thinks that having
I disagree, setting it to manual, when you are "manually" forcing the service to start certainly serves as a sensible setting. But this is a discussion for another time, lets not continue it in this thread. |
This comment has been minimized.
This comment has been minimized.
|
@iSazonov committee is on taking time off this week for Thanksgiving (US holiday), will resume next week |
TravisEz13
added
the
Review - Committee
label
Dec 7, 2017
This comment has been minimized.
This comment has been minimized.
|
@PowerShell/powershell-committee reviewed this. Given that |
SteveL-MSFT
added
Committee-Reviewed
and removed
Review - Committee
labels
Jan 4, 2018
daxian-dbw
assigned
adityapatwardhan
Feb 1, 2018
daxian-dbw
requested a review
from
anmenaga
Feb 1, 2018
adityapatwardhan
changed the title
Fix the Set-Service -Status Stopped issue #5517
Fix the Set-Service -Status Stopped issue
Feb 1, 2018
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@SteveL-MSFT I would like the comittee to reconsider the approval of |
SteveL-MSFT
added
Review - Committee
and removed
Committee-Reviewed
labels
Feb 10, 2018
This comment has been minimized.
This comment has been minimized.
|
@PowerShell/powershell-committee reviewed this. The only applicable parameter to |
SteveL-MSFT
added
the
Committee-Reviewed
label
Feb 21, 2018
TravisEz13
requested changes
Aug 6, 2018
|
This comment has been minimized.
This comment has been minimized.
|
@TravisEz13 Please help to review again. Thanks. |
TravisEz13
reviewed
Aug 9, 2018
|
TravisEz13
approved these changes
Aug 10, 2018
This comment has been minimized.
This comment has been minimized.
|
Does this PR need another review? |
This comment has been minimized.
This comment has been minimized.
|
@adityapatwardhan This looks ready for merge. |
This comment has been minimized.
This comment has been minimized.
|
@zhenggu Thanks for your contribution. Please add the PR submission template and update the appropriate fields. https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission The template is here: https://github.com/PowerShell/PowerShell/blob/master/.github/PULL_REQUEST_TEMPLATE.md |
This comment has been minimized.
This comment has been minimized.
|
@adityapatwardhan for this PR adds a "-Force" parameter for Set-Service, but I am not sure should I modify https://github.com/PowerShell/PowerShell-Docs/blob/staging/reference/6/Microsoft.PowerShell.Management/Set-Service.md before this PR merged or it should be changed after it this PR is merged? |
This comment has been minimized.
This comment has been minimized.
|
@zhenggu Please open an issue or PR in PowerShell-Docs repo and add the link here. |
This comment has been minimized.
This comment has been minimized.
|
@iSazonov Thanks, will add it |
zhenggu
added a commit
to zhenggu/PowerShell-Docs
that referenced
this pull request
Aug 27, 2018
zhenggu
referenced this pull request
Aug 27, 2018
Merged
Add "-Force" to Set-Service for PowerShell/PowerShell#5525 #2858
This comment has been minimized.
This comment has been minimized.
|
@adityapatwardhan have added the document, and all the actions are ready, please help to review. |
sdwheeler
added a commit
to MicrosoftDocs/PowerShell-Docs
that referenced
this pull request
Aug 27, 2018
adityapatwardhan
approved these changes
Aug 27, 2018
This comment has been minimized.
This comment has been minimized.
|
@zhenggu The tests are classified as |
This comment has been minimized.
This comment has been minimized.
zhenggu
added some commits
Aug 28, 2018
This comment has been minimized.
This comment has been minimized.
|
@adityapatwardhan, all test cases passed, please help to merge. |
adityapatwardhan
merged commit 358e8ab
into
PowerShell:master
Aug 28, 2018
7 of 8 checks passed
This comment has been minimized.
This comment has been minimized.
|
@zhenggu Thank you for your contribution! |
TravisEz13
added a commit
that referenced
this pull request
Aug 28, 2018
TravisEz13
added this to the v6.1.0 milestone
Aug 28, 2018
This comment has been minimized.
This comment has been minimized.
|
Thanks everyone's help to my first PR |
This comment has been minimized.
This comment has been minimized.
|
@zhenggu Thanks for your contribution and patience! I hope you will continue. |
zhenggu commentedNov 22, 2017
•
edited
PR Summary
Fixes #5517
In the previous version, if one service has dependency services, it cannot be stopped by "Set-Service -Stop", this PR fixes this issue.
And also add the Parameter "-Force" to Set-Service, when it is enabled, the function will stop it's dependent services, and then stop the target's service itself.
Without the parameter "-Force" it will raise a exception, if the service has running dependent services.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature testsThis change is