[Feature]Added Remove-service to Management module #4858

joandrsn · 2017-09-18T15:56:19Z

I have added the function Remove-Service to the Management module.
The functions New-Service and Set-Service were already present, I thought it would be a good idea to have a Remove-Service

Issue #4717

msftclas · 2017-09-18T15:56:23Z

@joandrsn,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

adityapatwardhan · 2017-09-18T18:06:47Z

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

+                    {
+                        hScManager = NativeMethods.OpenSCManagerW(
+                            ServiceComputerName,
+                            null,


Please use named parameters.

I don't quite follow you. Did you want to use the variable ComputerName ? Since the method supports both a Name on the local computer and a servicecontroller on a remote machine, I believe I have to use the ServiceComputerName variable.

see https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/named-and-optional-arguments

It makes the code more readable, particularly where null is passed.

adityapatwardhan · 2017-09-18T18:08:52Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

@@ -1,4 +1,5 @@
-Describe "Set/New-Service cmdlet tests" -Tags "Feature", "RequireAdminOnWindows" {
+Import-Module ".\Microsoft.PowerShell.Commands.Management.dll"


This should not be required. Can you remove this?

adityapatwardhan · 2017-09-18T18:10:02Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+        finally {
+            Get-CimInstance Win32_Service -Filter "name='$servicename'" | Remove-CimInstance -ErrorAction SilentlyContinue
+        }
+    }


Can you also add tests which validates the error cases?

Also add tests for pipeline input.

adityapatwardhan · 2017-09-18T18:15:37Z

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

+    /// This class implements the set-service command
+    /// </summary>
+    [Cmdlet(VerbsCommon.Remove, "Service", SupportsShouldProcess = true, DefaultParameterSetName = "Name")]
+    [OutputType(typeof(ServiceController))]


Typically Remove* cmdlets do not return anything.

adityapatwardhan · 2017-09-18T18:17:50Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+        finally {
+            Get-CimInstance Win32_Service -Filter "name='$servicename'" | Remove-CimInstance -ErrorAction SilentlyContinue
+        }
+    }


Also add tests for pipeline input.

markekraus

Minor formatting and comment/summary rewording suggestions.

markekraus · 2017-09-18T18:48:43Z

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

+
+        #region Overrides
+        /// <summary>
+        /// Create the service


Should be Remove the service?

markekraus · 2017-09-18T18:59:06Z

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


+    #region RemoveServiceCommand
+    /// <summary>
+    /// This class implements the set-service command


should be Remove-Service?

markekraus · 2017-09-18T19:06:56Z

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

+        #region Parameters
+
+        /// <summary>
+        /// Name of the service to create


Should be Name of the service to remove?

markekraus · 2017-09-18T19:07:43Z

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

+
+        /// <summary>
+        /// The following is the definition of the input parameter "InputObject".
+        /// Specifies ServiceController object representing the services to be stopped.


Should be the services to be removed.?

markekraus · 2017-09-18T19:09:50Z

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

+                            continue;
+                        }
+
+


Remove extra line.

…new function

markekraus · 2017-09-18T19:31:29Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

@@ -1,4 +1,5 @@
-Describe "Set/New-Service cmdlet tests" -Tags "Feature", "RequireAdminOnWindows" {
+Import-Module "C:\Users\jsa\code\PowerShell\src\powershell-win-core\bin\Debug\netcoreapp2.0\win7-x64\Microsoft.PowerShell.Commands.Management.dll"


Please Remove

markekraus · 2017-09-18T19:31:43Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+        { Remove-Service -Name "testremoveservice" -ErrorAction 'Stop' } | Should Throw
+    }
+
+


Remove extra line

…alid servicename

markekraus · 2017-09-18T21:39:55Z

@joandrsn it looks like the cmdlet doesn't export. I can repro on win10 x64. No clue why, though.

joandrsn · 2017-09-18T21:48:53Z

@markekraus If I import the cmdlet with my import statement that I previously committed (and removed again), I can get it to work. Somehow the travis build gets the module imported (it doesn't fail).
I'm not sure why that is.
If I try a Get-Command Get-Service I get a version 3.1.0.0 and after importing the .dll i get 6.0.0.0

adityapatwardhan · 2017-09-18T21:53:59Z

@joandrsn I guess you need to add them to the exports here:

https://github.com/PowerShell/PowerShell/blob/master/src/Modules/Windows-Core/Microsoft.PowerShell.Management/Microsoft.PowerShell.Management.psd1

https://github.com/PowerShell/PowerShell/blob/master/src/Modules/Unix/Microsoft.PowerShell.Management/Microsoft.PowerShell.Management.psd1

SteveL-MSFT · 2017-09-18T21:46:15Z

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

+        /// <summary>
+        /// Name of the service to remove
+        /// </summary>
+        /// <value></value>


should probably remove this

SteveL-MSFT · 2017-09-18T21:47:58Z

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

+        protected override void ProcessRecord()
+        {
+            ServiceController service = null;
+            string ServiceComputerName = null;


for consistency, should use camelCasing

SteveL-MSFT · 2017-09-18T21:51:46Z

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

+                    {
+                        hScManager = NativeMethods.OpenSCManagerW(
+                            ServiceComputerName,
+                            null,


see https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/named-and-optional-arguments

It makes the code more readable, particularly where null is passed.

SteveL-MSFT · 2017-09-18T21:54:58Z

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

+                                WriteNonTerminatingError(
+                                    service,
+                                    exception,
+                                    "CouldNotRemoveService",


This doesn't seem to the right error. According to the docs, CloseServiceHandle only fails if it's an invalid handle which is probably never going to the be case here. Anyways, even if this fails, it would seem that the service was still deleted. Maybe this should just be a diagnostic assert?

SteveL-MSFT · 2017-09-18T21:55:16Z

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

+                            bool succeeded = NativeMethods.CloseServiceHandle(hScManager);
+                            if (!succeeded)
+                            {
+                                int lastError = Marshal.GetLastWin32Error();


same as above, perhaps just an assert?

…ion on cleanup

SteveL-MSFT · 2017-09-18T23:18:17Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

+            }
+            $service = New-Service @parameters
+            $service | Should Not BeNullOrEmpty
+            Get-Service -Name $servicename | Remove-Service -ErrorAction SilentlyContinue


I don't think you want -ErrorAction SilentlyContinue there in case something fails

iSazonov · 2017-09-19T14:09:10Z

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

+                    Diagnostics.Assert(!String.IsNullOrEmpty(Name), "null ServiceName");
+                    // "new ServiceController" will succeed even if
+                    // there is no such service.  This checks whether
+                    // the service actually exists.


Should we put the comment before Diagnostics.Assert?
And please reformat the comment - we can use wider lines.

iSazonov · 2017-09-19T14:19:39Z

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


+    #region RemoveServiceCommand
+    /// <summary>
+    /// This class implements the remove-service command


Case - Remove-Service.

iSazonov · 2017-09-19T14:25:45Z

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

+                bool objServiceShouldBeDisposed = false;
+                try
+                {
+                    if (_ParameterSetName.Equals("InputObject", StringComparison.OrdinalIgnoreCase) && InputObject != null)


Why are we using _ParameterSetName in the file (twice)? In all other files we use ParameterSetName.
I believe we should fix this.

Looks like line 1604 used this and @joandrsn was just following this pattern. Agree that we should change it to just ParameterSetName (including line 1604) for consistency.

It seems we could remove _ParameterSetName.Equals("InputObject", StringComparison.OrdinalIgnoreCase) at all.

I agree. InputObject check should be sufficient

iSazonov · 2017-09-19T14:28:56Z

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

+                }
+                catch (ArgumentException ex)
+                {
+                    //Cannot use WriteNonterminatingError as service is null


Please add space // Cannot.

iSazonov · 2017-09-19T14:29:02Z

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

+                }
+                catch (InvalidOperationException ex)
+                {
+                    //Cannot use WriteNonterminatingError as service is null


Please add space // Cannot

iSazonov · 2017-09-19T14:33:55Z

@joandrsn Please add Remove-Service in DefaultCommands.Tests.ps1 to pass CI.

markekraus · 2017-09-19T16:07:12Z

test/powershell/engine/Basic/DefaultCommands.Tests.ps1

 "Cmdlet", "Set-PSDebug",                                        ,                     $($FullCLR -or $CoreWindows -or $CoreUnix)
 "Cmdlet", "Set-PSSessionConfiguration",                         ,                     $($FullCLR -or $CoreWindows -or $CoreUnix)
 "Cmdlet", "Set-Service",                                        ,                     $($FullCLR -or $CoreWindows              )
+"Cmdlet", "Remove-Service",                                     ,                     $($FullCLR -or $CoreWindows              )


Can you move this up in the list so it is in alphabetical order? Should go between Remove-PSSnapin and Remove-TypeData

…e test which used the function class directly

joandrsn · 2017-09-19T19:41:15Z

I think I figured it out.
I changed the inheritance from Cmdlet to PSCmdlet to use ParameterSetName instead of _ParameterSetName.
This meant I had to rewrite a test which used the class directly. Unfortunatly, the test was too complicated for me to rewrite in the same function as before, so I split the test into 4 separate tests.
Also I'm not sure if the last test actually does anything. On my computer, the service was already running. So maybe I should create a service and then change that service? Looking for feedback.

markekraus · 2017-09-19T20:03:41Z

I think this probably needs to be reverted. I believe ServiceBaseCommand intentionally derives from Cmdlet so that the service cmdlets can be directly invoked. This would likely be a Bucket 1 or 2 Breaking Change.

joandrsn · 2017-09-19T20:15:16Z

Do you have a suggestion on how I would be able to use ParameterSetName instead? I can't currently see the solution if changing the inheiritance to PSCmdlet would be a breaking change.

markekraus · 2017-09-19T21:38:07Z

I think it was using _ParameterSetName "by design" as ParameterSetName is not available in commands that derive from Cmdlet instead of PSCmdlet. RemoveServiceCommand could derive from PSCmdlet but that would break the derived methods. ServiceBaseCommand could be switched to derive from PSCmdlet, but that would probably take a review committee approval.

Looking for guidance from other cmdlets, many of them are using

if (InputObject != null)

and

if (Name != null)

Example RenameLocalGroupCommand

I lean towards either switching to the null checks or just leaving it as _ParameterSetName. I can see benefits to the Service Cmdlets being directly invoked fro host apps, so Moving to PSCmdlet seems like a bad option.

@SteveL-MSFT @iSazonov thoughts?

SteveL-MSFT · 2017-09-20T00:49:20Z

Seems like the best solution here (preferring simplicity) is to revert back to deriving from Cmdlet as it originally was which perhaps was intentional. Would like to see the tests back to being -TestCases as it seems there a bunch of similar code having split up due to the change in inheritance.

iSazonov · 2017-09-20T05:10:47Z

Oops, I again got an artifact 😄
I unlike using _ParameterSetName because it is marked as "internal" and available through a hack with InternalsVisibleTo.
So I agree that we should revert last changes and remove _ParameterSetName.

…d rewrite test which used the function class directly" This reverts commit da41f7d.

…ut in Set-Service

SteveL-MSFT

Thanks for the contribution!

adityapatwardhan

LGTM

markekraus

LGTM

adityapatwardhan · 2017-09-21T00:24:56Z

@joandrsn Thanks for your contribution!

PingPongSet · 2019-01-15T11:49:00Z

What vesion of PowerShell is Remove-service available for? What about version 4?

iSazonov · 2019-01-15T14:25:45Z

@PingPongSet Windows PowerShell is frozen. All new features will be only added to PowerShell Core. Feel free download latest version, try and feedback.

PingPongSet · 2019-01-15T16:09:20Z

@iSazonov What vesion of PowerShell is Remove-service available for?

iSazonov · 2019-01-16T03:18:17Z

@PingPongSet I don' remember the version but you could look release notes for PowerShell Core 6.0. Also make only sense download latest PowerShell Core 6.0 version.

joandrsn added 2 commits September 18, 2017 17:44

[Feature]Remove-Service added to Management module

fb73312

[Feature]Capitalization

32759f4

joandrsn requested review from adityapatwardhan and daxian-dbw as code owners September 18, 2017 15:56

msftclas added the cla-already-signed label Sep 18, 2017

daxian-dbw requested review from SteveL-MSFT and removed request for daxian-dbw September 18, 2017 16:57

daxian-dbw assigned adityapatwardhan Sep 18, 2017

adityapatwardhan requested changes Sep 18, 2017

View reviewed changes

[Feature]Added test cases for Remove-Service, Removed return value

072582f

markekraus reviewed Sep 18, 2017

View reviewed changes

joandrsn added 3 commits September 18, 2017 21:14

[Feature]Documentation from copy-paste code corrected to reflect the …

1349f60

…new function

[Feature]Erroraction to be sure that an error is thrown in testcase

b1c7e4d

[Feature]Removed direct reference to module in Pester test

17c4d4a

markekraus suggested changes Sep 18, 2017

View reviewed changes

joandrsn added 2 commits September 18, 2017 21:40

[Feature]Removed extra line in test

2585dbe

[Feature]Use FullyQualifiedErrorId in Remove-Service test without a v…

b56dba9

…alid servicename

SteveL-MSFT requested changes Sep 18, 2017

View reviewed changes

joandrsn added 2 commits September 19, 2017 00:05

[Feature]Exposed Remove-Service

c519a9a

[Feature]Consistent casing, Named Arguments & Remove incorrect except…

cb2ee47

…ion on cleanup

SteveL-MSFT requested changes Sep 18, 2017

View reviewed changes

[Feature]Remove-Service test should fail if the service was not removed

2d7e000

iSazonov reviewed Sep 19, 2017

View reviewed changes

[Feature]Cleanup comments & add Remove-Service for CI

4ffd2c0

markekraus suggested changes Sep 19, 2017

View reviewed changes

[Feature]Remove-Service in CI set in alphabetic order

2fcf55e

markekraus approved these changes Sep 19, 2017

View reviewed changes

[Feature]Use ParameterSetName instead of _ParameterSetName and rewrit…

da41f7d

…e test which used the function class directly

joandrsn added 2 commits September 20, 2017 08:23

Revert "[Feature]Use ParameterSetName instead of _ParameterSetName an…

6753840

…d rewrite test which used the function class directly" This reverts commit da41f7d.

[Feature]Remove _ParameterSetName check & added test for pipeline inp…

427bc7d

…ut in Set-Service

SteveL-MSFT approved these changes Sep 20, 2017

View reviewed changes

adityapatwardhan approved these changes Sep 20, 2017

View reviewed changes

markekraus approved these changes Sep 20, 2017

View reviewed changes

adityapatwardhan merged commit f281671 into PowerShell:master Sep 21, 2017

adityapatwardhan mentioned this pull request Sep 21, 2017

REQ: Remove-Service cmdlet #4717

Closed

joandrsn mentioned this pull request Oct 26, 2017

Remove-Service documentation added. MicrosoftDocs/PowerShell-Docs#1809

Merged

		@@ -1,4 +1,5 @@
		Describe "Set/New-Service cmdlet tests" -Tags "Feature", "RequireAdminOnWindows" {
		Import-Module ".\Microsoft.PowerShell.Commands.Management.dll"

		@@ -1,4 +1,5 @@
		Describe "Set/New-Service cmdlet tests" -Tags "Feature", "RequireAdminOnWindows" {
		Import-Module "C:\Users\jsa\code\PowerShell\src\powershell-win-core\bin\Debug\netcoreapp2.0\win7-x64\Microsoft.PowerShell.Commands.Management.dll"

		{ Remove-Service -Name "testremoveservice" -ErrorAction 'Stop' } \| Should Throw
		}

[Feature]Added Remove-service to Management module #4858

[Feature]Added Remove-service to Management module #4858

Uh oh!

Conversation

joandrsn commented Sep 18, 2017

Uh oh!

msftclas commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markekraus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markekraus commented Sep 18, 2017

Uh oh!

joandrsn commented Sep 18, 2017

Uh oh!

adityapatwardhan commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joandrsn commented Sep 19, 2017