fix set-service failing test by SteveL-MSFT · Pull Request #4802 · PowerShell/PowerShell

SteveL-MSFT · Sep 10, 2017

missed -ErrorAction Stop
changed new-service to return error when given unsupported startuptype

iSazonov · Sep 11, 2017

Blocked by #4803.

missed -ErrorAction Stop

made cmdlet write error when using unsupported startuptype re-enabled test

markekraus · Sep 11, 2017

@SteveL-MSFT something should probably be done about Set-Service.Tests.ps1#L25. That will continue to fail on Linux and macOS as the literal will still be parsed even though the IT is being skipped, and Get-Service is unavailable on Linux/macOS.

SteveL-MSFT · Sep 11, 2017

@markekraus I see. will fix.

fix test so that a script block only runs if It is run

daxian-dbw · Sep 11, 2017

-                        break;
+                        WriteNonTerminatingError(StartupType.ToString(), DisplayName, Name,
+                            new ArgumentException(), "CouldNotNewService",
+                            ServiceResources.UnsupportedStartupType,


There is another instance of Assert(((ServiceStartMode)(-1)) == StartupType, "bad StartupType" in Set-Service. It would be the best if all can be fixed here.

daxian-dbw · Sep 11, 2017

    <value>The command cannot be used to configure the service '{0}' because access to computer '{1}' is denied. Run PowerShell as admin and run your command again.</value>
  </data>
+  <data name="UnsupportedStartupType" xml:space="preserve">
+    <value>The startup type '{0}' is not supported by New-Service.</value>


by New-Service needs to be changed since the same applies to set-service as well.

iSazonov · Sep 12, 2017

        },
-        # This test case fails due to #4803. Disabled for now.
-        # @{name = 'badstarttype'; parameter = "StartupType"; value = "System"},
+        @{name = 'badstarttype'; parameter = "StartupType"; value = "System"},


Should we add "Boot" too?
Should we check valid and invalid values from System.ServiceProcess.ServiceStartMode? If yes it is better refactor the tests. And for Set-Service too.

will change

turned redundant code into helper function used by New/Set-Service added tests for Set-Service and unsupported StartupTypes

SteveL-MSFT · Sep 12, 2017

@iSazonov @daxian-dbw feedback addressed

iSazonov · Sep 13, 2017

+        /// <returns>
+        /// If a supported StartupType is provided, funciton returns true, otherwise false.
+        /// </returns>
+        internal static bool GetNativeStartupType(ServiceStartMode StartupType, out DWORD dwStartType)


Maybe TryGetNativeStartupType()?

Will change

iSazonov · Sep 13, 2017

@@ -1684,22 +1682,14 @@ protected override void ProcessRecord()
                            || (ServiceStartMode)(-1) != StartupType)
                        {
                            DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE;


We do the initialiazation in GetNativeStartupType. So we could remove this and below call by C# 7.0 pattern NativeMethods.GetNativeStartupType(StartupType, out DWORD dwStartType)) .

I can make that change for New-Service, but for Set-Service, I need to use dwStartType on line 1697 so it has to be initialized.

Thanks for clafiry. I think we should leave as is.
Closed.

In New-Service, dwStartType is also used in line 2045 when calling NativeMethods.CreateServiceW.
It turns out the following code works:

static void Blah () { if (!int.TryParse("1", out int result)) { Console.WriteLine("Failed"); } Console.WriteLine(result); }

It would be the best if we can do it in a consistent way in those 2 places.

address PR feedback

daxian-dbw · Sep 13, 2017

                        {
                            DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE;
-                            switch (StartupType)
+                            if ((ServiceStartMode)(-1) != StartupType &&


It seems (ServiceStartMode)(-1) != StartupType && should be removed.

Will change, but will need to change some other code to make this work. Seems like -1 shouldn't be needed at all.

Actually, still need -1 which will be treated as equivalent to SERVICE_NO_CHANGE

By looking at the original code, for New-Service, SERVICE_AUTO_START would be used in case StartupType is -1. So it seems we cannot just use dwStartType = NativeMethods.SERVICE_NO_CHANGE for (ServiceStartMode)(-1)

New-Service defaults to Automatic whereas Set-Service defaults to -1 to indicate no change.

~~Is it possible that the value of StartupType can be set to (-1)?~~
~~In the original code, dwStartType will still be SERVICE_AUTO_START even if StartupType is (-1), while it will be SERVICE_NO_CHANGE after the change.~~

Turns out we can't do [System.ServiceProcess.ServiceStartMode]-1 in PowerShell, so we are good.

I'll fix it so if it's (-1), it should default to Auto for New-Service

made StartupType(-1) equivalent to Win32 SERVICE_NO_CHANGE and moved to TryGetNativeStartupType

SteveL-MSFT · Sep 14, 2017

@daxian-dbw feedback addressed

daxian-dbw

LGTM except for a non-blocking comment.

daxian-dbw · Sep 14, 2017

@@ -1684,22 +1682,14 @@ protected override void ProcessRecord()
                            || (ServiceStartMode)(-1) != StartupType)
                        {
                            DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE;


In New-Service, dwStartType is also used in line 2045 when calling NativeMethods.CreateServiceW.
It turns out the following code works:

static void Blah () { if (!int.TryParse("1", out int result)) { Console.WriteLine("Failed"); } Console.WriteLine(result); }

It would be the best if we can do it in a consistent way in those 2 places.

SteveL-MSFT · Sep 14, 2017

@daxian-dbw can you review latest change?

daxian-dbw · Sep 14, 2017

@SteveL-MSFT we cannot pass the value (-1) to -StartupType parameter. The parameter binding will fail because powershell cannot convert -1 to [System.ServiceProcess.ServiceStartMode]. That means StartupType in New-Service can never be -1, so we should be good without the last commit.
That's why I crossed out my original comment :)

SteveL-MSFT · Sep 14, 2017

@daxian-dbw yeah, I went through the code and for New-Service, StartupType defaults to Auto and can't ever be set to (-1), so I'll revert the last commit.

SteveL-MSFT · Sep 14, 2017

Last commit was reverted. We should be good to go.

iSazonov · Sep 15, 2017

@SteveL-MSFT @daxian-dbw Is the PR ready to merge?

daxian-dbw · Sep 15, 2017

@iSazonov Yes, it's good to go.
BTW, please don't directly use the history commit messages as the description when squashing and merging. Instead, it's better to summarize the changes of the PR.

SteveL-MSFT assigned iSazonov Sep 10, 2017

SteveL-MSFT requested a review from iSazonov September 10, 2017 21:38

msftclas added the cla-not-required label Sep 10, 2017

SteveL-MSFT mentioned this pull request Sep 10, 2017

Set-Service and New-Service test coverage #4785

Merged

[feature]

babc348

missed -ErrorAction Stop

SteveL-MSFT force-pushed the set-service-fail branch from 473df67 to babc348 Compare September 11, 2017 21:32

[feature]

c67a75e

made cmdlet write error when using unsupported startuptype re-enabled test

SteveL-MSFT requested review from adityapatwardhan and daxian-dbw as code owners September 11, 2017 21:57

SteveL-MSFT added the Breaking-Change breaking change that may affect users label Sep 11, 2017

[feature]

4bde7fb

fix test so that a script block only runs if It is run

daxian-dbw requested changes Sep 11, 2017

View reviewed changes

iSazonov reviewed Sep 12, 2017

View reviewed changes

[feature]

94561f4

turned redundant code into helper function used by New/Set-Service added tests for Set-Service and unsupported StartupTypes

iSazonov reviewed Sep 13, 2017

View reviewed changes

[feature]

25b43a6

address PR feedback

iSazonov approved these changes Sep 13, 2017

View reviewed changes

daxian-dbw reviewed Sep 13, 2017

View reviewed changes

[feature]

7b86b32

made StartupType(-1) equivalent to Win32 SERVICE_NO_CHANGE and moved to TryGetNativeStartupType

SteveL-MSFT mentioned this pull request Sep 14, 2017

Added WSMan Config provider tests #4756

Merged

daxian-dbw approved these changes Sep 14, 2017

View reviewed changes

SteveL-MSFT force-pushed the set-service-fail branch from 802c823 to 7b86b32 Compare September 14, 2017 22:18

iSazonov merged commit b723d6b into PowerShell:master Sep 15, 2017

SteveL-MSFT deleted the set-service-fail branch September 15, 2017 14:28

SteveL-MSFT mentioned this pull request Oct 4, 2017

beta.8 changelog draft #5006

Merged

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

Conversation

SteveL-MSFT commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Sep 11, 2017

Uh oh!

markekraus commented Sep 11, 2017

Uh oh!

SteveL-MSFT commented Sep 11, 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!

SteveL-MSFT commented Sep 12, 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!

daxian-dbw Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Sep 14, 2017

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Sep 14, 2017

Uh oh!

daxian-dbw commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Sep 14, 2017

Uh oh!

SteveL-MSFT commented Sep 14, 2017

Uh oh!

iSazonov commented Sep 15, 2017

Uh oh!

daxian-dbw commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

SteveL-MSFT commented Sep 10, 2017 •

edited

Loading

daxian-dbw Sep 14, 2017 •

edited

Loading

daxian-dbw commented Sep 14, 2017 •

edited

Loading

daxian-dbw commented Sep 15, 2017 •

edited

Loading