-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix Set-Service -Status Stopped runs failed on the service with dependencies #5525
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
Conversation
…ents services And 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. (PowerShell#5517)
iSazonov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mklement0 Could you please review the PR?
|
|
||
| if ((servicedependedon != null) && (servicedependedon.Length > 0)) | ||
| { | ||
| WriteNonTerminatingError(service, null, "ServiceIsDependentOnNoForce", ServiceResources.ServiceIsDependentOnNoForce, ErrorCategory.InvalidOperation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the resource string from RESX file it is no longer used.
| if (!service.Status.Equals(ServiceControllerStatus.Stopped)) | ||
| { | ||
| //check for the dependent services as set-service dont have force parameter | ||
| //check for the dependent services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct the comment "// Check ...."
|
I'm curious if this is even the right direction. Doesn't |
|
We have a similar question in #5471 |
|
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. |
|
@SteveL-MSFT Could you please review the PR with #5471 on PowerShell committee? |
|
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 |
|
@mklement0 Thanks for useful comments!
Design review needed. |
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 |
|
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 |
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. |
|
@iSazonov committee is on taking time off this week for Thanksgiving (US holiday), will resume next week |
|
@PowerShell/powershell-committee reviewed this. Given that |
|
@SteveL-MSFT I would like the comittee to reconsider the approval of |
|
@PowerShell/powershell-committee reviewed this. The only applicable parameter to |
|
test/tools/TestService/Service1.Designer.cs, line 1 at r12 (raw file):
missing copyright headers |
TravisEz13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r8, 6 of 7 files at r11, 1 of 1 files at r12.
Reviewable status:complete! all files reviewed
|
@TravisEz13 Please help to review again. Thanks. |
TravisEz13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r13.
Reviewable status:complete! all files reviewed
|
Does this PR need another review? |
|
@adityapatwardhan This looks ready for merge. |
|
@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 |
|
@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? |
|
@zhenggu Please open an issue or PR in PowerShell-Docs repo and add the link here. |
|
@iSazonov Thanks, will add it |
|
@adityapatwardhan have added the document, and all the actions are ready, please help to review. |
|
@zhenggu The tests are classified as |
|
@adityapatwardhan, all test cases passed, please help to merge. |
|
@zhenggu Thank you for your contribution! |
|
Thanks everyone's help to my first PR |
|
@zhenggu Thanks for your contribution and patience! I hope you will continue. |
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