★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/5525Nouvelle recherche | Portail wanayoo
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

Fix Set-Service -Status Stopped runs failed on the service with dependencies #5525

Merged
merged 25 commits into from Aug 28, 2018

Conversation

@zhenggu
Copy link
Contributor

commented Nov 22, 2017

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


This change is Reviewable

Fix the Set-Service -Status Stopped issue when the service has depend…
…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. (#5517)
@msftclas

This comment has been minimized.

Copy link

commented Nov 22, 2017

CLA assistant check
All CLA requirements met.

@iSazonov
Copy link
Collaborator

left a comment

@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.

Copy link
@iSazonov

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.

Copy link
@iSazonov

iSazonov Nov 22, 2017

Collaborator

Please correct the comment "// Check ...."

zhenggu added some commits Nov 22, 2017

@markekraus

This comment has been minimized.

Copy link
Collaborator

commented Nov 22, 2017

I'm curious if this is even the right direction. Doesn't Stop-Service -Force do this already? Shouldn't Set-Service only support a a very basic stop? The -Status on Set-Service is redundant to stop-service which should be the de facto way to stop services.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 22, 2017

We have a similar question in #5471

@zhenggu

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2017

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.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 23, 2017

@SteveL-MSFT Could you please review the PR with #5471 on PowerShell committee?

@mklement0

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2017

Yes, Set-Service -Status duplicates Start-Service / Resume-Service / Stop-Service / Suspend-Service functionality, but:

  • it is a one-stop solution with more of a desired-state feel.
  • it offers functionality that Start-Service doesn't have: if the target service's startup mode is currently Disabled, Start-Service invariably fails. By contrast, you can use Set-Service -StartupType Manual -State Running in such a case, for instance.

On the flip side, it currently doesn't support -Force in order to stop a service that have dependents - which I think should be fixed too.

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 -Status Stopped work as expected, as long as the target service doesn't have dependents.

@iSazonov: How is #5471 related to this?

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 23, 2017

@mklement0 Thanks for useful comments!

How is #5471 Start-Process: add parameter 'ExitTimeout' related to this?

Design review needed.

@markekraus

This comment has been minimized.

Copy link
Collaborator

commented Nov 23, 2017

@mklement0

it is a one-stop solution with more of a desired-state feel.

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 -force for set- to only affect the stop is ridiculous. We have the stop verb, use it for advanced stop functionality. If you can get away with basic stop on set, so be it, that's what stop is for. If I could go back in time I would say the set verb should not have had this functionality to begin with. if it did, then the stop verb should not have been included.

On your second point about start-service, I will say that set is also not start and including it originally is silly, IMO. The correction there is to add a -force to start if it is not already there that enables the service if it is not disabled and starts any services it depends on.

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 set-service @params -PassThru | stop-service or set-service @params -PassThru | start-service. That keeps to the do one thing philosophy. increasing complexity in a command to do another thing when there is another command that does that one thing is an odd design choice. (setting aside any perceived convenience in doing so).

@mklement0

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2017

I don't why it was decided to provide duplicate functionality in Set-Service, but given that it was:

  • fixing the bug is a must, as we agree.

  • however, not duplicating all the functionality seems silly, if adding -Force is all that it takes, especially given that, as demonstrated, in other aspects it can do more than one of its counterparts.
    (On a side note: status value suspended should probably be added as an alias for paused).

Adding a -Force switch to Start-Service is not a solution, because there is no sensible default for the startup type that invariably has to change in that case - unless you add -StartupType to Start-Service too, but that would then duplicate what Set-Service already does.

Yes, you can argue that the -Status parameter should never have been implemented, but, looking at the bigger picture, the Set verb has always had desired-state aspects in certain cmdlets, resulting in duplication as well; two examples:

  • You can use Set-Variable without ever having to touch New-Variable - the variable is created on demand.

  • You can use Set-Content to create a file on demand, without needing to use New-Item.

And, finally, another aspect in which Set-Service -Status provides extra (again, desired-state) functionality is that -Status Running either starts or resumes (continues) a service, as appropriate, whereas applying Start-Service to a suspended (paused) service results in an error.

@markekraus

This comment has been minimized.

Copy link
Collaborator

commented Nov 23, 2017

however, not duplicating all the functionality seems silly,

No, it doesn't at all. Set- is for setting, Stop- is for stopping. if the ability to stop on set- goes beyond anything basic, you should use stop-. The ability to stop on set- is a convenience that arguably should not be there (and I'm not advocating for its removal, for clarity).

Tell me, absent -Status Stopped, what purposed does -Force have? If I see -Force on Set-Service is expect it to allow me to force service settings. In this case, the only thing force is good for is stopping the service. We already have Stop-Service -Force for that. For desired state, you are not likely going to be forcing the service to stop. Again, this adds complexity and redundancy, and I'm arguing for simplicity.

Edit: as a compromise, if everyone really thinks that having set- force stop a service is not as crazy as I think it is, how about -Status StoppedForced or something instead of -Force switch that only works for a specific single value on a single parameter?

Adding a -Force switch to Start-Service is not a solution, because there is no sensible default for the startup type that invariably has to change in that case

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.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Nov 23, 2017

@iSazonov committee is on taking time off this week for Thanksgiving (US holiday), will resume next week

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

@PowerShell/powershell-committee reviewed this. Given that -Status Stopped is already there, it would make sense to support -Force for Set-Service

@daxian-dbw daxian-dbw requested a review from anmenaga Feb 1, 2018

@adityapatwardhan adityapatwardhan changed the title Fix the Set-Service -Status Stopped issue #5517 Fix the Set-Service -Status Stopped issue Feb 1, 2018

@markekraus

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2018

@SteveL-MSFT I would like the comittee to reconsider the approval of -Force for this PR. RE: #6113 (comment) and a few other discussions we have had here. The -Force in this PR not only applies to a single parameter, it also only applies to a single option for that parameter. I still believe including a ForceStopped or StoppedForced option for -Status is the better design choice and that we should avoid -Force abuse.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

@PowerShell/powershell-committee reviewed this. The only applicable parameter to -Force is -Status (when value is Stopped) and there's no applicability to other parameters for this cmdlet. The usage is consistent with other cmdlets like Stop-Service, so committee continues to recommend using -Force for this case.

@TravisEz13
Copy link
Member

left a comment

Reviewed 1 of 1 files at r8, 6 of 7 files at r11, 1 of 1 files at r12.
Reviewable status: :shipit: complete! all files reviewed

@zhenggu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

@TravisEz13 Please help to review again. Thanks.

@TravisEz13
Copy link
Member

left a comment

Reviewed 4 of 4 files at r13.
Reviewable status: :shipit: complete! all files reviewed

@zhenggu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

Does this PR need another review?

@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

@adityapatwardhan This looks ready for merge.

@adityapatwardhan

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

@zhenggu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

@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?

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Aug 23, 2018

@zhenggu Please open an issue or PR in PowerShell-Docs repo and add the link here.

@zhenggu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2018

@iSazonov Thanks, will add it

zhenggu added a commit to zhenggu/PowerShell-Docs that referenced this pull request Aug 27, 2018

@zhenggu zhenggu referenced this pull request Aug 27, 2018

Merged

Add "-Force" to Set-Service for PowerShell/PowerShell#5525 #2858

2 of 9 tasks complete
@zhenggu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

@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

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@zhenggu The tests are classified as Feature so I pushed an empty commit with [Feature] to run all tests. If all tests pass, I will go ahead and merge.

@adityapatwardhan

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@zhenggu Please have a look at the failures and for the next commit have [Feature] in your commit message to ensure all tests are executed. See this for more details.

zhenggu added some commits Aug 28, 2018

@zhenggu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2018

@adityapatwardhan, all test cases passed, please help to merge.

@adityapatwardhan adityapatwardhan merged commit 358e8ab into PowerShell:master Aug 28, 2018

7 of 8 checks passed

CodeFactor 8 issues fixed. 533 issues found.
Details
PowerShell-CI-linux PR-5525-20180828.02 succeeded with issues
Details
PowerShell-CI-macos PR-5525-20180828.02 succeeded with issues
Details
PowerShell-CI-windows PR-5525-20180828.02 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@adityapatwardhan

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@zhenggu Thank you for your contribution!

TravisEz13 added a commit that referenced this pull request Aug 28, 2018

@TravisEz13 TravisEz13 added this to the v6.1.0 milestone Aug 28, 2018

@zhenggu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2018

Thanks everyone's help to my first PR

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Aug 29, 2018

@zhenggu Thanks for your contribution and patience! I hope you will continue.

joeyaiello added a commit to MicrosoftDocs/PowerShell-Docs that referenced this pull request Sep 13, 2018

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