-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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 failing test #4802
Conversation
Blocked by #4803. |
473df67
to
babc348
Compare
@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 |
@markekraus I see. will fix. |
break; | ||
WriteNonTerminatingError(StartupType.ToString(), DisplayName, Name, | ||
new ArgumentException(), "CouldNotNewService", | ||
ServiceResources.UnsupportedStartupType, |
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.
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.
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.
will fix
@@ -201,4 +201,7 @@ | |||
<data name="ComputerAccessDenied" xml:space="preserve"> | |||
<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> |
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.
by New-Service
needs to be changed since the same applies to set-service
as well.
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.
will fix
@@ -163,8 +166,7 @@ Describe "Set/New-Service cmdlet tests" -Tags "Feature", "RequireAdminOnWindows" | |||
[System.Management.Automation.PSCredential]::new("username", | |||
(ConvertTo-SecureString "PlainTextPassword" -AsPlainText -Force))) | |||
}, | |||
# This test case fails due to #4803. Disabled for now. | |||
# @{name = 'badstarttype'; parameter = "StartupType"; value = "System"}, | |||
@{name = 'badstarttype'; parameter = "StartupType"; value = "System"}, |
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.
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.
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.
will change
@iSazonov @daxian-dbw feedback addressed |
/// <returns> | ||
/// If a supported StartupType is provided, funciton returns true, otherwise false. | ||
/// </returns> | ||
internal static bool GetNativeStartupType(ServiceStartMode StartupType, out DWORD dwStartType) |
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.
Maybe TryGetNativeStartupType()
?
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.
Will change
@@ -1684,22 +1682,14 @@ protected override void ProcessRecord() | |||
|| (ServiceStartMode)(-1) != StartupType) | |||
{ | |||
DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE; |
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 do the initialiazation in GetNativeStartupType. So we could remove this and below call by C# 7.0 pattern NativeMethods.GetNativeStartupType(StartupType, out DWORD dwStartType))
.
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.
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.
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.
Thanks for clafiry. I think we should leave as is.
Closed.
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.
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.
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.
Addressed
@@ -1684,22 +1682,14 @@ protected override void ProcessRecord() | |||
|| (ServiceStartMode)(-1) != StartupType) | |||
{ | |||
DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE; | |||
switch (StartupType) | |||
if ((ServiceStartMode)(-1) != StartupType && |
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.
It seems (ServiceStartMode)(-1) != StartupType &&
should be removed.
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.
Will change, but will need to change some other code to make this work. Seems like -1
shouldn't be needed at all.
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.
Actually, still need -1
which will be treated as equivalent to SERVICE_NO_CHANGE
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.
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)
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.
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.
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.
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.
I'll fix it so if it's (-1), it should default to Auto for New-Service
@daxian-dbw feedback addressed |
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.
LGTM except for a non-blocking comment.
@@ -1684,22 +1682,14 @@ protected override void ProcessRecord() | |||
|| (ServiceStartMode)(-1) != StartupType) | |||
{ | |||
DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE; |
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.
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.
@daxian-dbw can you review latest change? |
@SteveL-MSFT we cannot pass the value (-1) to |
@daxian-dbw yeah, I went through the code and for |
802c823
to
7b86b32
Compare
Last commit was reverted. We should be good to go. |
@SteveL-MSFT @daxian-dbw Is the PR ready to merge? |
@iSazonov Yes, it's good to go. |
missed -ErrorAction Stop
changed new-service to return error when given unsupported startuptype
Fix #4800
Fix #4803