SqlSetup: Add missing properties: Service Startup Type (SQLENGINE/AGT,AS,IS,RS) #1184

mdaniou · 2018-07-14T15:58:17Z

Pull Request (PR) description

Added new parameters to allow to define the Startup Types of the services while installing them

This Pull Request (PR) fixes the following issues

Fixes #1165

Task list

Added an entry under the Unreleased section in the CHANGELOG.md? Entry
should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md?
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help?
Comment-based help added/updated?
Localization strings added/updated in all localization files as appropriate?
Examples appropriately added/updated?
Unit tests added/updated?
See DSC Resource Testing Guidelines.
Integration tests added/updated (where possible)?
See DSC Resource Testing Guidelines.
New/changed code adheres to
DSC Resource Style Guidelines
and Best Practices?

This change is

…artup types of the installed services.

codecov-io · 2018-07-14T16:57:26Z

Codecov Report

Merging #1184 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##            dev   #1184    +/-   ##
=====================================
+ Coverage    97%     97%   +<1%     
=====================================
  Files        33      33            
  Lines      4016    4044    +28     
=====================================
+ Hits       3929    3957    +28     
  Misses       87      87

johlju · 2018-07-14T21:56:44Z

Awesome!! I will review this one after the weekend :)

johlju

Reviewed 3 of 6 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @mdaniou)

a discussion (no related file):
Can we add the once that are possible to the integration test as well? Not all services are installed in the tests yet, so at least test those that can.

SQL Engine, SQL Agent, Browser

Config: https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Integration/MSFT_SqlSetup.config.ps1#L193
Tests: https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Integration/MSFT_SqlSetup.Integration.Tests.ps1#L244

CHANGELOG.md, line 37 at r2 (raw file):

    service, the Agent service, the Analysis service and the Integration Service.
    The new optional parameters are respectively SqlSvcStartupType, AgtSvcStartupType,
    AsSvcStartupType, IsSvcStartupType and RsSvcStartupType.

Can we add the following to the end of the sentence? Referencing the issue.

...RsSvcStartupType. ([issue #1165](https://github.com/PowerShell/SqlServerDsc/issues/1165).

You may also add your name to this change if you like (optional)

* Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account)

README.md, line 1636 at r2 (raw file):

* **`[PSCredential]` ISSvcAccount** _(Write)_: Service account for Integration
  Services service.
* **`[String]` SqlSvcStartupType** _(Write)_: Specifies the startup mode for

Should we add something to the description what the default is if left out? 🤔 If so, please update comment-based help and schema.mof as well.

README.md, line 1637 at r2 (raw file):

'Manual'

Should be just Manual without the single quotes? To be like to other two. Throughout.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 648 at r2 (raw file):

    .PARAMETER SqlSvcStartupType
       Specifies the startup mode for SQL Server Engine service

We should end the sentence with full stop (.) to be consequent. Throughout.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1291 at r2 (raw file):

 $setupArguments += @{ AgtSvcStartupType = 'Automatic' }

If this is the default value for this property, then I suggest to add that in an else-block on the if-block below. No need to set it twice if it is assigned a value in the configuration.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1291 at r2 (raw file):

$setupArguments += @{ AgtSvcStartupType = 'Automatic' }

If this is the default value, then we should highlight that in the README.md like { *Automatic* | Disabled | 'Manual' }. Making the value 'Automatic' italic to show it's the default. It should also say in the description that it is the default.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1298 at r2 (raw file):

$null -ne $SqlSvcStartupType

We can use $PSBoundParameters.ContainsKey for this as well?

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1319 at r2 (raw file):

            $setupArguments += (Get-ServiceAccountParameters -ServiceAccount $RSSvcAccount -ServiceType 'RS')
        }
        if ($null -ne $RsSvcStartupType)

We can use $PSBoundParameters.ContainsKey for this as well?

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1365 at r2 (raw file):

        }

        if ( $null -ne $AsSvcStartupType )

We can use $PSBoundParameters.ContainsKey for this as well?

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1378 at r2 (raw file):

        }

        if ( $null -ne $IsSvcStartupType )

We can use $PSBoundParameters.ContainsKey for this as well?

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1685 at r2 (raw file):

    .PARAMETER SqlSvcStartupType
       Specifies the startup mode for SQL Server Engine service

We should end the sentence with full stop (.) to be consequent. Throughout.

Examples/Resources/SqlSetup/6-InstallNamedInstanceSingleServerWithAgtSvcStartupTypeDisabled.ps1, line 4 at r2 (raw file):

    .EXAMPLE
        This example shows how to install a named instance of SQL Server on a single server.
    .NOTES

Add a blank row before this row.

mdaniou

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @mdaniou and @johlju)

CHANGELOG.md, line 37 at r2 (raw file):

Name/Alias (@github_account)
ok , thanks

README.md, line 1636 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should we add something to the description what the default is if left out? 🤔 If so, please update comment-based help and schema.mof as well.

I don't think that we should specify that. If the default installation configs are updated in the future, we would have to update the information here as well.
We already know that whatever you don't specify will be default.
Now about the Agent service that is set to Manual by default when installing Sql Server whereas it is Automatic in this DSC, I think that it is a problem. However I didn't want to make any breaking change ...

In conclusion, I might want to specify the default value for the agent only or remove the Automatic value if left out (then creating a breaking change).
Do you get my point ?

README.md, line 1637 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'Manual'
Should be just Manual without the single quotes? To be like to other two. Throughout.

Right, I copies/pasted the Browser description :)

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 648 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should end the sentence with full stop (.) to be consequent. Throughout.

Ok, I do that for the whole script at the same time.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1291 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

 $setupArguments += @{ AgtSvcStartupType = 'Automatic' }
If this is the default value for this property, then I suggest to add that in an else-block on the if-block below. No need to set it twice if it is assigned a value in the configuration.

I had done that at the beginning then I had problems during the tests as it was not taking the account the else block for some reasons. I used this workaround.
Don't ask me more details as I wouldn't be able to explain. :)
I create the else block and I'll see. Maybe I was a bit tired !

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1298 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$null -ne $SqlSvcStartupType
We can use $PSBoundParameters.ContainsKey for this as well?

I agree, although I was not sure. Would you qualify this as a good practice ?

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1319 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We can use $PSBoundParameters.ContainsKey for this as well?

yes

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1365 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We can use $PSBoundParameters.ContainsKey for this as well?

yes

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1378 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We can use $PSBoundParameters.ContainsKey for this as well?

yes

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1685 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should end the sentence with full stop (.) to be consequent. Throughout.

ok

Examples/Resources/SqlSetup/6-InstallNamedInstanceSingleServerWithAgtSvcStartupTypeDisabled.ps1, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank row before this row.

sure

mdaniou · 2018-07-17T16:55:24Z

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Can we add the once that are possible to the integration test as well? Not all services are installed in the tests yet, so at least test those that can.

SQL Engine, SQL Agent, Browser

Config: https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Integration/MSFT_SqlSetup.config.ps1#L193
Tests: https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Integration/MSFT_SqlSetup.Integration.Tests.ps1#L244

I don't feel comfortable touching all these integration stuff. Let me first handle all the other comments and I will have a look at this.
To be honest, I spend 10x more time working on the test part then actually updating the DSC script.

johlju · 2018-07-17T16:59:37Z

@mdaniou I know, it's the same for me - the tests take a lot of work - but it is worth the time in the end 🙂

johlju · 2018-07-19T11:37:24Z

@mdaniou Just a gentle reminder to write 'Done' on the review comments when you are finished, so I know I should review again. No hurry though, take the time you need. :)

johlju · 2018-07-25T09:18:43Z

@mdaniou Is this one ready for review again?

mdaniou · 2018-07-25T10:01:56Z

@johlju not yet. This is holiday time so I will handle that in August.
I have asked some questions you can answer though. If you could have a look. :)
I need to work on the integration improvements.

johlju

Thanks for you reply. Answer the questions. 🙂
Just a couple of review comments left, I have acknowledge the others

Have a lovely holiday!

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju)

a discussion (no related file):

Previously, mdaniou (Maxime Daniou) wrote…

I don't feel comfortable touching all these integration stuff. Let me first handle all the other comments and I will have a look at this.
To be honest, I spend 10x more time working on the test part then actually updating the DSC script.

Only the integration tests left now I think. Let me know if you need any pointers with those.

README.md, line 1636 at r2 (raw file):

Previously, mdaniou (Maxime Daniou) wrote…

I don't think that we should specify that. If the default installation configs are updated in the future, we would have to update the information here as well.
We already know that whatever you don't specify will be default.
Now about the Agent service that is set to Manual by default when installing Sql Server whereas it is Automatic in this DSC, I think that it is a problem. However I didn't want to make any breaking change ...

In conclusion, I might want to specify the default value for the agent only or remove the Automatic value if left out (then creating a breaking change).
Do you get my point ?

I agree with what you say here. I also agree that Automatic value should not be default if that parameter is left out.
Can you please submit a new issue to track this? That could be another PR that introduces a breaking change for this.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1291 at r2 (raw file):

Previously, mdaniou (Maxime Daniou) wrote…

I had done that at the beginning then I had problems during the tests as it was not taking the account the else block for some reasons. I used this workaround.
Don't ask me more details as I wouldn't be able to explain. :)
I create the else block and I'll see. Maybe I was a bit tired !

Looks like the tests agrees with this change :)

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1291 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$setupArguments += @{ AgtSvcStartupType = 'Automatic' }
If this is the default value, then we should highlight that in the README.md like { *Automatic* | Disabled | 'Manual' }. Making the value 'Automatic' italic to show it's the default. It should also say in the description that it is the default.

Withdraw this comment as this will be obsolete when the new breaking change issue discussed in another comment is fixed in another PR.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1298 at r2 (raw file):

Previously, mdaniou (Maxime Daniou) wrote…

I agree, although I was not sure. Would you qualify this as a good practice ?

More being consequent, as the other code used it, than good practice. Although it is easier to read, and it now only adds the argument if the parameter was passed, as the previous could have added the argument if the value was change from a $null value in the previous code.

stale · 2018-08-09T11:09:46Z

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

mdaniou · 2018-08-09T12:33:12Z

yes I know I need to handle this ! :)

johlju · 2018-08-10T08:26:37Z

@mdaniou great if we can get this over the finish line. 😃 The abandoned label should be removed once you push next time.

johlju · 2018-08-10T08:27:23Z

I forgot to add a 😃, so updated previous comment 😄

mdaniou · 2018-08-16T14:30:46Z

README.md, line 1636 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I agree with what you say here. I also agree that Automatic value should not be default if that parameter is left out.
Can you please submit a new issue to track this? That could be another PR that introduces a breaking change for this.

Cool thanks, Issue #1198 created

mdaniou · 2018-08-27T14:55:52Z

Tests/Integration/MSFT_SqlSetup.Integration.Tests.ps1, line 251 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we add the properties to be tested for 'Automatic' here (the value you set them to in the configuration file).
AgtSvcStartupType
AsSvcStartupType
SqlSvcStartupType

This would only work if I modify the Get-TargetResource function right. My understanding is that this function is run by Get-DscConfiguration.
Could you please confirm that ?

mdaniou · 2018-08-27T15:40:35Z

Tests/Integration/MSFT_SqlSetup.Integration.Tests.ps1, line 251 at r4 (raw file):

Previously, mdaniou (Maxime Daniou) wrote…

This would only work if I modify the Get-TargetResource function right. My understanding is that this function is run by Get-DscConfiguration.
Could you please confirm that ?

I have done some tests and got the answer. :)

johlju · 2018-08-28T05:42:06Z

@mdaniou Great that you found the answer. 🙂

mdaniou · 2018-08-28T13:29:02Z

Tests/Integration/MSFT_SqlSetup.Integration.Tests.ps1, line 251 at r4 (raw file):

Previously, mdaniou (Maxime Daniou) wrote…

I have done some tests and got the answer. :)

Done.

mdaniou · 2018-08-28T14:02:58Z

All good here I think 👍

johlju

Great work! This is almost complete now! Just a minor comment.

Reviewed 2 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mdaniou)

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 138 at r6 (raw file):

Quoted 4 lines of code…

        If ($SqlSvcStartupType -eq 'Auto')
        {
            $SqlSvcStartupType = 'Automatic'
        }

Could we make this a helper function instead? Passing in the StartMode and returning the correct value. Then we don't have to repeat the same if-block 5 times? What do you think?

… value

mdaniou

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @johlju)

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 138 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

        If ($SqlSvcStartupType -eq 'Auto')
        {
            $SqlSvcStartupType = 'Automatic'
        }
Could we make this a helper function instead? Passing in the StartMode and returning the correct value. Then we don't have to repeat the same if-block 5 times? What do you think?

Done.

johlju

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mdaniou)

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 135 at r7 (raw file):

        $agentServiceAccountUsername = $agentServiceCimInstance.StartName

        $SqlSvcStartupType = Set-StringToAutomatic $sqlServiceCimInstance.StartMode

We should use named parameters, so this should be -StartMode $sqlServiceCimInstance.StartMode (see next comment for the rename of the parameter).

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2343 at r7 (raw file):

Set-StringToAutomatic

I suggest we rename the function to ConvertTo-StartupType, then it will be able to handle future types that need to be converted.
Update the comment-based help to explain what it does.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2349 at r7 (raw file):

$String

I think we should call this parameter $StartMode, and make sure the the comment-based help is updated to explain what should be passed in.

…To-StartupType.

johlju

@mdaniou Looking much better! Just minor comments left! After these comments I think it ready to merge. 🙂

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mdaniou)

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 135 at r8 (raw file):

$SqlSvcStartupType = ConvertTo-StartupType $sqlServiceCimInstance.StartMode

We should use named parameters, we should add -StartMode here. so this should be

$SqlSvcStartupType = ConvertTo-StartupType -StartMode $sqlServiceCimInstance.StartMode

Throughout on the 4 other occurances to.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2338 at r8 (raw file):

Modify value to be 'Automatic' if it is 'Auto'. Otherwise returns the same string

Could we change this description to something like: "Converts the start mode property returned by a Win32_Service CIM object to the resource properties *StartupType equivalent"

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2340 at r8 (raw file):

String

Should me StartMode

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2341 at r8 (raw file):

String to check.

Maybe we can change this to "The StartMode to convert."

mdaniou

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 135 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$SqlSvcStartupType = ConvertTo-StartupType $sqlServiceCimInstance.StartMode
We should use named parameters, we should add -StartMode here. so this should be
$SqlSvcStartupType = ConvertTo-StartupType -StartMode $sqlServiceCimInstance.StartMode
Throughout on the 4 other occurances to.

Done.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2338 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Modify value to be 'Automatic' if it is 'Auto'. Otherwise returns the same string
Could we change this description to something like: "Converts the start mode property returned by a Win32_Service CIM object to the resource properties *StartupType equivalent"

Done.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2340 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

String
Should me StartMode

Done.

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2341 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

String to check.
Maybe we can change this to "The StartMode to convert."

Done.

johlju

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2340 at r8 (raw file):

Previously, mdaniou (Maxime Daniou) wrote…

Done.

This one wasn't changed, should be .PARAMETER StartMode.

mdaniou · 2018-08-30T11:00:48Z

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2340 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This one wasn't changed, should be .PARAMETER StartMode.

Yes indeed ...

johlju

Reviewed 1 of 1 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved

johlju · 2018-08-30T11:08:49Z

@mdaniou Awesome work on this one! Just waiting for the tests to pass, then I merge.

mdaniou · 2018-08-30T15:08:41Z

👍
Awesome ! The test/integration part is quite tough ! :)

mdaniou · 2018-08-30T15:17:24Z

@johlju : It seems like the entry for this change in the CHANGE.LOG is not in UNRELEASED. Instead it is under 11.4.0.0.
Not sure it is intended.

https://github.com/PowerShell/SqlServerDsc/blob/dev/CHANGELOG.md#11400

johlju · 2018-08-30T15:29:33Z

Yes, tests are a hill to climb over until you get hang of it, but they are well worth the effort!

Yes, I saw that the change log merged into wrong place. I does that after a release. I have a PR open to fix it. Waiting for the test to pass and then I will merge that PR. Thanks for noticing and reporting it!

johlju · 2018-08-30T15:31:01Z

Btw, I usually see that the change log will act up when I review, and ask for a rebase against dev, this time I missed it. :)

mdaniou added 2 commits July 14, 2018 17:40

Apply changes to add new parameters that would allow to define the st…

1bf58c5

…artup types of the installed services.

Updates some changes to remove some formatting issues

563ebc2

Fixing tabs and markdowns (trying)

b5f937f

mdaniou mentioned this pull request Jul 14, 2018

SqlSetup: Missing Configuration Feature/Options : Service Startup Type (SQLENGINE/AGT,AS,IS,RS) #1165

Closed

johlju added the needs review The pull request needs a code review. label Jul 14, 2018

johlju changed the title ~~Fixes 1165 / SqlSetup / Missing Configuration Feature/Options : Service Startup Type (SQLENGINE/AGT,AS,IS,RS)~~ SqlSetup:Missing Configuration Feature/Options : Service Startup Type (SQLENGINE/AGT,AS,IS,RS) Jul 17, 2018

johlju changed the title ~~SqlSetup:Missing Configuration Feature/Options : Service Startup Type (SQLENGINE/AGT,AS,IS,RS)~~ SqlSetup: Add missing properties: Service Startup Type (SQLENGINE/AGT,AS,IS,RS) Jul 17, 2018

johlju requested changes Jul 17, 2018

View reviewed changes

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 17, 2018

mdaniou commented Jul 17, 2018

View reviewed changes

mdaniou added 2 commits July 18, 2018 09:55

Applying changes after code reviews

7281f12

removes tabs on the DSC file

5e3757d

johlju requested changes Jul 26, 2018

View reviewed changes

stale bot added the abandoned The pull request has been abandoned. label Aug 9, 2018

johlju mentioned this pull request Aug 16, 2018

BREAKING CHANGE: SqlSetup: Default startup option for the agent service should be Manual instead of Automatic #1198

Closed

Testing the integration modifications

6c8df4e

stale bot removed the abandoned The pull request has been abandoned. label Aug 17, 2018

updating the integration script to validate the startup configurations

d4cce97

Update resource to support the startup mode in the Get function

6c43024

Update the test script to improve code coverage

20b4d74

johlju requested changes Aug 29, 2018

View reviewed changes

Create helper function to reduce code redundancy arount the StartMode…

9490948

… value

mdaniou commented Aug 29, 2018

View reviewed changes

johlju requested changes Aug 29, 2018

View reviewed changes

mdaniou added 3 commits August 29, 2018 13:04

Fix the function Set-StringToAutomatic to accept empty parameter

b96e3f1

Modify the name of the function from Set-StringToAutomatic to Convert…

26e7d9b

…To-StartupType.

Add [Parameter()] as this is required

11a82aa

johlju requested changes Aug 30, 2018

View reviewed changes

mdaniou commented Aug 30, 2018

View reviewed changes

Updating the comments

6b8d94e

johlju requested changes Aug 30, 2018

View reviewed changes

Update the PARAMETER name

32c033f

johlju approved these changes Aug 30, 2018

View reviewed changes

johlju merged commit 3714325 into dsccommunity:dev Aug 30, 2018

johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Aug 30, 2018

mdaniou deleted the mdaniou/MSFT_SqlSetup_ServiceStartupImplementation branch August 30, 2018 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SqlSetup: Add missing properties: Service Startup Type (SQLENGINE/AGT,AS,IS,RS) #1184

SqlSetup: Add missing properties: Service Startup Type (SQLENGINE/AGT,AS,IS,RS) #1184

mdaniou commented Jul 14, 2018 •

edited by johlju

Loading

codecov-io commented Jul 14, 2018 •

edited

Loading

johlju commented Jul 14, 2018

johlju left a comment

mdaniou left a comment

mdaniou commented Jul 17, 2018

johlju commented Jul 17, 2018

johlju commented Jul 19, 2018

johlju commented Jul 25, 2018

mdaniou commented Jul 25, 2018

johlju left a comment

stale bot commented Aug 9, 2018

mdaniou commented Aug 9, 2018

johlju commented Aug 10, 2018 •

edited

Loading

johlju commented Aug 10, 2018

mdaniou commented Aug 16, 2018

mdaniou commented Aug 27, 2018

mdaniou commented Aug 27, 2018

johlju commented Aug 28, 2018

mdaniou commented Aug 28, 2018

mdaniou commented Aug 28, 2018

johlju left a comment

mdaniou left a comment

johlju left a comment

johlju left a comment

mdaniou left a comment

johlju left a comment

mdaniou commented Aug 30, 2018

johlju left a comment

johlju commented Aug 30, 2018

mdaniou commented Aug 30, 2018

mdaniou commented Aug 30, 2018 •

edited

Loading

johlju commented Aug 30, 2018

johlju commented Aug 30, 2018

SqlSetup: Add missing properties: Service Startup Type (SQLENGINE/AGT,AS,IS,RS) #1184

SqlSetup: Add missing properties: Service Startup Type (SQLENGINE/AGT,AS,IS,RS) #1184

Conversation

mdaniou commented Jul 14, 2018 • edited by johlju Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov-io commented Jul 14, 2018 • edited Loading

Codecov Report

johlju commented Jul 14, 2018

johlju left a comment

Choose a reason for hiding this comment

mdaniou left a comment

Choose a reason for hiding this comment

mdaniou commented Jul 17, 2018

johlju commented Jul 17, 2018

johlju commented Jul 19, 2018

johlju commented Jul 25, 2018

mdaniou commented Jul 25, 2018

johlju left a comment

Choose a reason for hiding this comment

stale bot commented Aug 9, 2018

mdaniou commented Aug 9, 2018

johlju commented Aug 10, 2018 • edited Loading

johlju commented Aug 10, 2018

mdaniou commented Aug 16, 2018

mdaniou commented Aug 27, 2018

mdaniou commented Aug 27, 2018

johlju commented Aug 28, 2018

mdaniou commented Aug 28, 2018

mdaniou commented Aug 28, 2018

johlju left a comment

Choose a reason for hiding this comment

mdaniou left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

mdaniou left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

mdaniou commented Aug 30, 2018

johlju left a comment

Choose a reason for hiding this comment

johlju commented Aug 30, 2018

mdaniou commented Aug 30, 2018

mdaniou commented Aug 30, 2018 • edited Loading

johlju commented Aug 30, 2018

johlju commented Aug 30, 2018

mdaniou commented Jul 14, 2018 •

edited by johlju

Loading

codecov-io commented Jul 14, 2018 •

edited

Loading

johlju commented Aug 10, 2018 •

edited

Loading

mdaniou commented Aug 30, 2018 •

edited

Loading