Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
adityapatwardhan merged 25 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
zhenggu:masterzhenggu/PowerShell:masterCopy head branch name to clipboard
Aug 28, 2018
Merged

Fix Set-Service -Status Stopped runs failed on the service with dependencies #5525
adityapatwardhan merged 25 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
zhenggu:masterzhenggu/PowerShell:masterCopy head branch name to clipboard

Conversation

@zhenggu

@zhenggu zhenggu commented Nov 22, 2017

Copy link
Copy Markdown
Contributor

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

…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)
@msftclas

msftclas commented Nov 22, 2017

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@iSazonov iSazonov left a comment

Copy link
Copy Markdown
Collaborator

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);

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator

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

@markekraus

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Collaborator

We have a similar question in #5471

@zhenggu

zhenggu commented Nov 23, 2017

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Collaborator

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

@mklement0

mklement0 commented Nov 23, 2017

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Collaborator

@mklement0 Thanks for useful comments!

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

Design review needed.

@markekraus

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown
Contributor

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

markekraus commented Nov 23, 2017

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

@TravisEz13 TravisEz13 added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Dec 7, 2017
@SteveL-MSFT

Copy link
Copy Markdown
Member

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

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 4, 2018
@daxian-dbw daxian-dbw requested a review from anmenaga February 1, 2018 17:51
@adityapatwardhan adityapatwardhan changed the title Fix the Set-Service -Status Stopped issue #5517 Fix the Set-Service -Status Stopped issue Feb 1, 2018
@adityapatwardhan

Copy link
Copy Markdown
Member

@markekraus

markekraus commented Feb 9, 2018

Copy link
Copy Markdown
Contributor

@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 SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee and removed Committee-Reviewed PS-Committee has reviewed this and made a decision labels Feb 10, 2018
@SteveL-MSFT

Copy link
Copy Markdown
Member

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

@SteveL-MSFT SteveL-MSFT added the Committee-Reviewed PS-Committee has reviewed this and made a decision label Feb 21, 2018
@TravisEz13

Copy link
Copy Markdown
Member

test/tools/TestService/Service1.Designer.cs, line 1 at r12 (raw file):

namespace TestService

missing copyright headers

@TravisEz13 TravisEz13 left a comment

Copy link
Copy Markdown
Member

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: :shipit: complete! all files reviewed

@zhenggu

zhenggu commented Aug 7, 2018

Copy link
Copy Markdown
Contributor Author

@TravisEz13 Please help to review again. Thanks.

@TravisEz13 TravisEz13 left a comment

Copy link
Copy Markdown
Member

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: :shipit: complete! all files reviewed

@zhenggu

zhenggu commented Aug 16, 2018

Copy link
Copy Markdown
Contributor Author

Does this PR need another review?

@anmenaga

Copy link
Copy Markdown

@adityapatwardhan This looks ready for merge.

@adityapatwardhan

Copy link
Copy Markdown
Member

@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

@zhenggu

zhenggu commented Aug 22, 2018

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Collaborator

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

@zhenggu

zhenggu commented Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

@iSazonov Thanks, will add it

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

zhenggu commented Aug 27, 2018

Copy link
Copy Markdown
Contributor Author

@adityapatwardhan have added the document, and all the actions are ready, please help to review.

sdwheeler pushed a commit to MicrosoftDocs/PowerShell-Docs that referenced this pull request Aug 27, 2018
@adityapatwardhan

adityapatwardhan commented Aug 27, 2018

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

@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

zhenggu commented Aug 28, 2018

Copy link
Copy Markdown
Contributor Author

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

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

Copy link
Copy Markdown
Member

@zhenggu Thank you for your contribution!

@zhenggu

zhenggu commented Aug 28, 2018

Copy link
Copy Markdown
Contributor Author

Thanks everyone's help to my first PR

@iSazonov

Copy link
Copy Markdown
Collaborator

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

joeyaiello pushed 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

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set-Service -Status Stopped should not prevent stopping of services without dependents only because they depend on others

9 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.