BREAKING CHANGE: Updated xSQLServerConfiguration to support clustered instances (Fixes #144)

[nabrond]

The xSQLServerConfiguration resource only supported changing configuration values for the local computer, not for clustered instances. This PR makes the following changes:

• Updated MOF and resource parameters to be consistent with other resources [BREAKING CHANGE]
• Now using helper function Connect-SQL
• Created new internal restart function that supports clusters
• Removed unused internal functions and objects
• Created unit tests per template (Issue Pester Tests Need to be implemented for all existing Modules #92)
• Updated README to reflect changes

---

This change is

[msftclas]

Hi @nabrond, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Brandon Adams). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

[johlju]

@nabrond Could you add '(Fixes #144)' to the PR title? Then it will auto-close that issue on merge.

[mbreakey3]

Reviewed 3 of 5 files at r1, 1 of 1 files at r2.
Review status: 4 of 5 files reviewed at latest revision, 8 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 7 at r2 (raw file):

Import-Module $currentPath\..\..\xSQLServerHelper.psm1 -Verbose:$false -ErrorAction Stop

Function Get-TargetResource

function should be all lowercase

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 12 at r2 (raw file):

    [OutputType([System.Collections.Hashtable])]
    param(
        # Hostname of the SQL Server to be configured

Could you move these comments up above the function in help comment form

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 20 at r2 (raw file):

        [parameter(Mandatory = $false)]
        [System.String]
        $SQLInstanceName = "MSSQLSERVER",

Single quotes here

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 108 at r2 (raw file):

    if ($option.IsDynamic -eq $true)
    {  
        New-VerboseMessage -Message "Configuration option has been updated."

single quotes

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r2 (raw file):

        [parameter(Mandatory = $false)]
        [System.String]
        $SQLInstanceName = "MSSQLSERVER",

single quotes

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 60 at r2 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        Context "Validate returned properties" {

single quotes

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 64 at r2 (raw file):

            $result = Get-TargetResource @desiredState

            It "Property: SQLServer" {

single quotes for all of these 'It' statements as well.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 132 at r2 (raw file):

    #region Non-Exported Function Unit Tests
    InModuleScope $script:DSCResourceName {
        Describe "Testing Restart-SqlService" {

single quotes

---

Comments from Reviewable

[mbreakey3]

@johlju - If you get a chance would you mind looking over this PR for functionality?

[johlju]

@mbreakey3 absolutely I will do that

[mbreakey3]

Reviewed 1 of 5 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 7 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

function should be all lowercase

OK

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 12 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Could you move these comments up above the function in help comment form

OK

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 20 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Single quotes here

OK

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 108 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

single quotes

OK

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

single quotes

OK

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 60 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

single quotes

OK

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 64 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

single quotes for all of these 'It' statements as well.

OK

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 132 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

single quotes

OK

---

Comments from Reviewable

[johlju]

Awesome work!

I don't have a cluster in my lab at home, so have to verify the cluster bits tomorrow at work.
So I will continue the review tomorrow. But it put together some comments to start with. 😄

---

Reviewed 2 of 5 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r3 (raw file):

        $SQLServer,

        [parameter(Mandatory = $false)]

Please change all [paramater to [Parameter (upper case 'P')
There are more than one that needs to change.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

        [System.Boolean]
        $RestartService = $false

This parameter could be remove from the Get-method since it does not do anything. In the returned hash table you could just return $null. Less to test in the tests too.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 61 at r3 (raw file):

    if(!$option)
    {
        throw "Specified option '$OptionName' was not found!"

Please use New-TerminatingError here (helper function)

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 64 at r3 (raw file):

    }

    $returnValue = @{

You can return the hashtable directly instead of assigning it to a variable and returning the variable.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 128 at r3 (raw file):

    if(!$option)
    {
        throw "Specified option '$OptionName' was not found!"

Please use New-TerminatingError here (helper function). That will make the string message to be reused from the Get-method also. And allow localization in the future.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r3 (raw file):

    $sql.Configuration.Alter()

    if ($option.IsDynamic -eq $true)

Could we instead check if the running value $option.RunValue is different than config value, and if so restart the service if the user asked us to?
I think some config values are dynamic even if IsDynamic is set to false, and we don't want to restart the service unnecessary. Correct me if I'm wrong.

See remarks:
https://msdn.microsoft.com/en-us/library/microsoft.sqlserver.management.smo.configproperty.runvalue.aspx

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 145 at r3 (raw file):

    else
    {
        Write-Warning 'Configuration option has been updated. SQL Server restart is required!'

Could we change the test to something like "Configuration option has been updated. Manual restart of SQL Server is required for the change to take effect!"

Bonus points 😄 : Create a new helper function like New-TerminatingError, i.e New-WarningMessage, to be able to use localization strings.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 193 at r3 (raw file):

    )

    $state = Get-TargetResource -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName -OptionName $OptionName -OptionValue $OptionValue -RestartService $RestartService

Could you use splatting on these parameters instead?

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):

        ## Get the cluster resources
        New-VerboseMessage -Message 'Getting cluster resource for SQL Server' 
        $SqlService = Get-WmiObject -Namespace root/MSCluster -Class MSCluster_Resource -Filter "Type = 'SQL Server' AND Name LIKE '%$($ServerObject.ServiceName)%'"

Could you change this to $sqlService = .... And all other occurrences.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):

        New-VerboseMessage -Message 'Getting cluster resource for SQL Server Agent'
        $AgentService = Get-WmiObject -Namespace root/MSCLuster -Class MSCluster_Resource -Filter "Type = 'SQL Server Agent' AND Name LIKE '%$($ServerObject.ServiceName)%'"

Could you change this to $agentService = .... And all other occurrences.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r3 (raw file):

        ## Stop the SQL Server resource
        New-VerboseMessage -Message 'SQL Server resource --> Offline'
        $SqlService.TakeOffline(120)

Could we make this a parameter in the set-method instead of using a hard-coded value?

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 236 at r3 (raw file):

        ## Start the SQL Agent resource
        New-VerboseMessage -Message 'SQL Server Agent --> Online'
        $AgentService.BringOnline(120)

Could we make this a parameter in the set-method instead of using a hard-coded value? Same parameter as above would work.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 241 at r3 (raw file):

    {
        New-VerboseMessage -Message 'Getting SQL Service information'
        $SqlService = Get-Service -DisplayName "SQL Server ($($ServerObject.ServiceName))"

Could we get the service using the service name instead´of the display name?

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 242 at r3 (raw file):

        New-VerboseMessage -Message 'Getting SQL Service information'
        $SqlService = Get-Service -DisplayName "SQL Server ($($ServerObject.ServiceName))"
        $AgentService = $SqlService.DependentServices | Where-Object { $_.StartType -ne ''}

Should we not start this only when StartType is `Automatic´?

$SqlService.DependentServices | Where-Object { $_.StartType -eq 'Automatic' }

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 249 at r3 (raw file):

        ## Start the SQL Server Agent service
        if ($AgentService)

If $AgentService contains more than one dependent service it will start all. But the message will say it only starts Agent. Think we would either change the message to "Starting dependent services" or loop thur each so we can tell which dependent service is started.
I know that default, today, it will only contain one dependent service.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 28 at r3 (raw file):

    OptionName = "user connections"
    OptionValue = 500
    RestartService = $False

Change to $false

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 38 at r3 (raw file):

{
    #region Not in the desired state
    Describe 'The system is not in the desired state' {

If you change this to Describe "$($script:DSCResourceName)" it's easier to see what resource is tested.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 60 at r3 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        Context 'Validate returned properties' {

And change the context to 'The system is not in the desired state'

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 64 at r3 (raw file):

            $result = Get-TargetResource @desiredState

            It 'Property: SQLServer' {

Can you change all It-blocks messages to start with 'Should...'
You can add all but one of the It-blocks together in an It-block like this

It 'Should return the same values as passed as parameters' {
    $result.SQLServer | Should Be $desiredState.SQLServer
    $result.SQLInstanceName | Should Be $desiredState.SQLInstanceName
    $result.OptionName | Should Be $desiredState.OptionName
}

It 'Should not return the same value as passed for the parameter OptionValue' {
    $result.OptionValue | Should Not Be $desiredState.OptionValue
}

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 85 at r3 (raw file):

        }

        It 'Test method returns false' {

This one should be in the same context as above It-blocks

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 89 at r3 (raw file):

        }

        It 'Set method calls Connect-SQL' {

This one should be in the same context as above It-blocks

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 91 at r3 (raw file):

        It 'Set method calls Connect-SQL' {
            ## attempt to bring the system into the desired state
            Get-TargetResource @desiredState

You don't call Set-method. And you should Assert that the mocks for setting the value and restart service is not called here.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 94 at r3 (raw file):

            # Check that our mock was called at least 3 times
            ##Assert-MockCalled -CommandName Connect-SQL -Times 1 -Scope Describe

Remove this comment if it should not be used.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 98 at r3 (raw file):

        }
    }
    #endregion Not in the desired state

You should have a test to test when OptionName is missing and throws.

---

Comments from Reviewable

[johlju]

Sorry it took a day extra for me to review the cluster bits in this code. To much work at work so didn't have time until now. :)

I will review the tests once all these changes are done.

---

Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 57 at r3 (raw file):

    ## get the configuration option
    $option = $sql.Configuration.Properties | where { $_.DisplayName -eq $optionName }

Please change whereto Where-Object

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 124 at r3 (raw file):

    ## get the configuration option
    $option = $sql.Configuration.Properties | where {$_.DisplayName -eq $optionName}

Please change whereto Where-Object

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):

        ## Get the cluster resources
        New-VerboseMessage -Message 'Getting cluster resource for SQL Server' 
        $SqlService = Get-WmiObject -Namespace root/MSCluster -Class MSCluster_Resource -Filter "Type = 'SQL Server' AND Name LIKE '%$($ServerObject.ServiceName)%'"

These two Get-WmiObject for SQL Server and SQL Server Agent seems to only work for named instances in my tests.

• If the default instance is used then no WMI object will be returned. In that case the ServiceName property will contain the value 'MSSQLSERVER', but the Name property of the cluster resource will be 'SQL Server'
• The wildcard serach could potentially return two resource objects. If an instance is called 'SQL2008', and another is called 'SQL2008NEW' then both will be returned when trying to change the instance named 'SQL2008'. Change the code to something similar like below would solve that.

$sqlServerTypeName = 'SQL Server'
$SqlService = Get-WmiObject -Namespace root/MSCluster -Class MSCluster_Resource -Filter "Type = '$sqlServerTypeName' AND Name LIKE '$sqlServerTypeName ($($ServerObject.ServiceName))'"

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):

        New-VerboseMessage -Message 'Getting cluster resource for SQL Server Agent'
        $AgentService = Get-WmiObject -Namespace root/MSCLuster -Class MSCluster_Resource -Filter "Type = 'SQL Server Agent' AND Name LIKE '%$($ServerObject.ServiceName)%'"

Same problems exist for the SQL Server Agent WMI query as for the SQL Server WMI query.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):

class MSFT_xSQLServerConfiguration : OMI_BaseResource
{
    [Key, Description("The SQL Server for the configuration option.")] String SQLServer;

Add to the description of the SQLServer property in the schema and README.md something like "... or the DNS name of the cluster group".

Could you also please align these descriptions between the Schema, README.md and the comment based help in the private functions, so they all are the same descriptions.

---

Comments from Reviewable

[nabrond]

Review status: 1 of 6 files reviewed at latest revision, 29 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change all [paramater to [Parameter (upper case 'P')
There are more than one that needs to change.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This parameter could be remove from the Get-method since it does not do anything. In the returned hash table you could just return $null. Less to test in the tests too.

Correct me if I am wrong, but aren't all TargetResource methods in a Resource to have the same parameters? See https://msdn.microsoft.com/en-us/powershell/dsc/authoringResourceMOF#writing-the-resource-script

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 57 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change whereto Where-Object

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 61 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use New-TerminatingError here (helper function)

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 64 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You can return the hashtable directly instead of assigning it to a variable and returning the variable.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 124 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change whereto Where-Object

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 128 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use New-TerminatingError here (helper function). That will make the string message to be reused from the Get-method also. And allow localization in the future.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we instead check if the running value $option.RunValue is different than config value, and if so restart the service if the user asked us to?
I think some config values are dynamic even if IsDynamic is set to false, and we don't want to restart the service unnecessary. Correct me if I'm wrong.

See remarks:
https://msdn.microsoft.com/en-us/library/microsoft.sqlserver.management.smo.configproperty.runvalue.aspx

The IsDynamic property comes directly from the sys.configurations table's is_dynamic column. I believe this remark is simply noting that if you update the ConfigValue for a non-dynamic configuration item, it will not be reflected in the RunValue property until after the instance is restarted.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 145 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we change the test to something like "Configuration option has been updated. Manual restart of SQL Server is required for the change to take effect!"

Bonus points 😄 : Create a new helper function like New-TerminatingError, i.e New-WarningMessage, to be able to use localization strings.

Updated wording; will look into creating a helper function.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 193 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you use splatting on these parameters instead?

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you change this to $sqlService = .... And all other occurrences.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you change this to $agentService = .... And all other occurrences.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same problems exist for the SQL Server Agent WMI query as for the SQL Server WMI query.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we make this a parameter in the set-method instead of using a hard-coded value?

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 236 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we make this a parameter in the set-method instead of using a hard-coded value? Same parameter as above would work.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 241 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we get the service using the service name instead´of the display name?

I think with the information supplied, this is the best method to identify the service.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 242 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should we not start this only when StartType is `Automatic´?

$SqlService.DependentServices | Where-Object { $_.StartType -eq 'Automatic' }

Checks for dependent services that are running. I believe this is better than checking for start-up type as there could be viable scenarios where an automatic service is turned off and should not be restarted automatically.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 249 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If $AgentService contains more than one dependent service it will start all. But the message will say it only starts Agent. Think we would either change the message to "Starting dependent services" or loop thur each so we can tell which dependent service is started.
I know that default, today, it will only contain one dependent service.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add to the description of the SQLServer property in the schema and README.md something like "... or the DNS name of the cluster group".

Could you also please align these descriptions between the Schema, README.md and the comment based help in the private functions, so they all are the same descriptions.

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 28 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to $false

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 38 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If you change this to Describe "$($script:DSCResourceName)" it's easier to see what resource is tested.

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 60 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And change the context to 'The system is not in the desired state'

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 64 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you change all It-blocks messages to start with 'Should...'
You can add all but one of the It-blocks together in an It-block like this

It 'Should return the same values as passed as parameters' {
    $result.SQLServer | Should Be $desiredState.SQLServer
    $result.SQLInstanceName | Should Be $desiredState.SQLInstanceName
    $result.OptionName | Should Be $desiredState.OptionName
}

It 'Should not return the same value as passed for the parameter OptionValue' {
    $result.OptionValue | Should Not Be $desiredState.OptionValue
}

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 85 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This one should be in the same context as above It-blocks

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 89 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This one should be in the same context as above It-blocks

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 91 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You don't call Set-method. And you should Assert that the mocks for setting the value and restart service is not called here.

I think this was just bad commenting in my code. Get will never call Restart-SqlService.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 94 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Remove this comment if it should not be used.

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 98 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You should have a test to test when OptionName is missing and throws.

Done.

---

Comments from Reviewable

[nabrond]

Thanks for the feedback @johlju. Fixing these things and writing the tests exposed other issues that had be fixed along the way. I think at this point, all your comments have been answered.

---

Review status: 1 of 6 files reviewed at latest revision, 29 unresolved discussions.

---

Comments from Reviewable

[nabrond]

Review status: 1 of 6 files reviewed at latest revision, 29 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

These two Get-WmiObject for SQL Server and SQL Server Agent seems to only work for named instances in my tests.

• If the default instance is used then no WMI object will be returned. In that case the ServiceName property will contain the value 'MSSQLSERVER', but the Name property of the cluster resource will be 'SQL Server'
• The wildcard serach could potentially return two resource objects. If an instance is called 'SQL2008', and another is called 'SQL2008NEW' then both will be returned when trying to change the instance named 'SQL2008'. Change the code to something similar like below would solve that.

$sqlServerTypeName = 'SQL Server'
$SqlService = Get-WmiObject -Namespace root/MSCluster -Class MSCluster_Resource -Filter "Type = '$sqlServerTypeName' AND Name LIKE '$sqlServerTypeName ($($ServerObject.ServiceName))'"

Rewrote these WMI calls to better identify the SQL instances.

---

Comments from Reviewable

[mbreakey3]

Reviewed 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Correct me if I am wrong, but aren't all TargetResource methods in a Resource to have the same parameters? See https://msdn.microsoft.com/en-us/powershell/dsc/authoringResourceMOF#writing-the-resource-script

They all must have the same Mandatory parameters but Set and Test are the only ones that need to have all of the same parameters.

---

Comments from Reviewable

[nabrond]

Reviewed 1 of 5 files at r1, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

They all must have the same Mandatory parameters but Set and Test are the only ones that need to have all of the same parameters.

Unless there is a style-based or otherwise compelling reason, I would like to keep this parameter as-is. In addition to the consistency between functions, I appreciate the ease of calling Get-TargetResource from Test-TargetResource.

$state = Get-TargetResource @PSBoundParameters

versus

$params = @{
    SQLServer = $SQLServer
    SQLInstanceName = $SQLInstanceName
    OptionName = $OptionName
    OptionValue = $OptionValue
}

$state = Get-TargetResource @params

---

Comments from Reviewable

[mbreakey3]

Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Unless there is a style-based or otherwise compelling reason, I would like to keep this parameter as-is. In addition to the consistency between functions, I appreciate the ease of calling Get-TargetResource from Test-TargetResource.

$state = Get-TargetResource @PSBoundParameters

versus

$params = @{
SQLServer = $SQLServer
SQLInstanceName = $SQLInstanceName
OptionName = $OptionName
OptionValue = $OptionValue
}

$state = Get-TargetResource @params

I understand, you can leave as is. Just add a comment for that param in the help comment above that it's not used in this funciton

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

Comments from Reviewable

[nabrond]

Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

I understand, you can leave as is. Just add a comment for that param in the help comment above that it's not used in this funciton

Done.

---

Comments from Reviewable

[johlju]

Will verify the cluster code tomorrow (I will try to get an opportunity).
And I still have to review the tests, which I have not done fully yet.

---

Reviewed 1 of 5 files at r1, 4 of 5 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

@mbreakey3 I think this would be good to add to the style guideline maybe? The part that if unused non-mandatory parameters are added to the Get-method then to the comment based help "add a comment for that param in the help comment above that it's not used in this function". Could you look at get that into a guideline? Or I could submit an issue also.

@mbreakey3 I raised this question in another PR as well, see PR #147. It would be awesome if you could read and comment on that too, and see if that is something that needs to be added to a guideline as well.
If you don't have any other comments than what was said here, then on that PR I will add a comment to do the same as was done here, until there is a guideline that says differently. :)

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 57 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 61 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 64 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 124 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 128 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

The IsDynamic property comes directly from the sys.configurations table's is_dynamic column. I believe this remark is simply noting that if you update the ConfigValue for a non-dynamic configuration item, it will not be reflected in the RunValue property until after the instance is restarted.

Yes, you are correct. Read this one wrong. Stand corrected. :)

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 145 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Updated wording; will look into creating a helper function.

It would be awesome if you could create the helper function, would be appreciated. But for now this looks good to me.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 193 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 236 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 241 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

I think with the information supplied, this is the best method to identify the service.

I think the ServiceName would not change between versions of SQL, but higher risk that the DisplayName could change. But lets leave it as is.

LGTM.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 242 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Checks for dependent services that are running. I believe this is better than checking for start-up type as there could be viable scenarios where an automatic service is turned off and should not be restarted automatically.

Ah, good point! Could you please add that as a comment? I think that it would be good for next coder to know your thought here.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 249 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 58 at r5 (raw file):

    ## get the configuration option
    $option = $sql.Configuration.Properties | Where-Object { $_.DisplayName -eq $optionName }

$optionName should be $OptionName since it is referencing a parameter, not a local variable.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 140 at r5 (raw file):

    {
        New-VerboseMessage -Message 'Configuration option has been updated, restarting instance...'
        Restart-SqlService -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

Could we add a parameter to the Set-method (Timeout parameter to the schema) so that Restart-SqlService can be called with a user provided timeout value?

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 211 at r5 (raw file):

    .EXAMPLE
    $server = Connect-SQL -SQLServer $env:ComputerName

This is not needed in the example since the function calls Connect-SQL?

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 212 at r5 (raw file):

    .EXAMPLE
    $server = Connect-SQL -SQLServer $env:ComputerName
    Restart-SqlService -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

Could this line maybe be without the variables, and add the timeout variable?

Restart-SqlService -SQLServer localhost -SQLInstanceName MSSQLSERVER -Timeout 60

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r5 (raw file):

    )

    if (!$ServerObject)

This check is not necessary anymore since you remove the parameter. Change so the check is done after you assign the $serverObject.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 234 at r5 (raw file):

    if (!$ServerObject)
    {
        $ServerObject = Connect-SQL -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

Change $ServerObject to $serverObject

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 247 at r5 (raw file):

        ## Stop the SQL Server resource
        New-VerboseMessage -Message 'SQL Server resource --> Offline'

Maybe add which resource (the name) you are bringing offline too? Could you please change the message to something like this "Bringing the resource SQL Server offline"
Since it could take a while, I also think the user would be interested in a message 'Resource ... is offline' after the TakeOffline() has run? What do you think?

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 251 at r5 (raw file):

        ## Start the SQL Agent resource
        New-VerboseMessage -Message 'SQL Server Agent --> Online'

Could you do the same for BringOnline() as you do for the TakeOffline() part? (a message before and after bringing online the resource, since it could take a while)

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):

"... or the DNS name of the cluster group"

I don't see this change :) My thought is that the VCO differ from the host name, so I think it should say both. Are you of a different opinion?

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 110 at r5 (raw file):

            }

            It 'Test method returns false' {

This It-block should start with 'Should...'

---

Comments from Reviewable

[nabrond]

Review status: 2 of 7 files reviewed at latest revision, 14 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 242 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah, good point! Could you please add that as a comment? I think that it would be good for next coder to know your thought here.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 58 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$optionName should be $OptionName since it is referencing a parameter, not a local variable.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 140 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a parameter to the Set-method (Timeout parameter to the schema) so that Restart-SqlService can be called with a user provided timeout value?

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 211 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This is not needed in the example since the function calls Connect-SQL?

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 212 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could this line maybe be without the variables, and add the timeout variable?

Restart-SqlService -SQLServer localhost -SQLInstanceName MSSQLSERVER -Timeout 60

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This check is not necessary anymore since you remove the parameter. Change so the check is done after you assign the $serverObject.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 234 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change $ServerObject to $serverObject

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 247 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe add which resource (the name) you are bringing offline too? Could you please change the message to something like this "Bringing the resource SQL Server offline"
Since it could take a while, I also think the user would be interested in a message 'Resource ... is offline' after the TakeOffline() has run? What do you think?

Done. I do not think there is value to adding another message between `TakeOffline()` and `BringOnline()` since nothing else occurs between their calls.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 251 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you do the same for BringOnline() as you do for the TakeOffline() part? (a message before and after bringing online the resource, since it could take a while)

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"... or the DNS name of the cluster group"

I don't see this change :) My thought is that the VCO differ from the host name, so I think it should say both. Are you of a different opinion?

I think that "hostname" and "DNS name" are synonymous.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 110 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This It-block should start with 'Should...'

Done. Corrected a few others as well.

---

Comments from Reviewable

[johlju]

Reviewed 4 of 5 files at r6.
Review status: 6 of 7 files reviewed at latest revision, 17 unresolved discussions.

---

README.md, line 354 at r6 (raw file):

  - Added support for clustered SQL instances
  - BREAKING CHANGE: Updated parameters to align with other resources (SQLServer / SQLInstanceName)
* Created unit tests for resource

Could you add for which resource a test was added? See example under 2.0.0.0 below

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 140 at r5 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

Awesome! Please add the parameter to the README.md as well 😄

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 247 at r5 (raw file):

Previously, nabrond (Brandon) wrote…

Done. I do not think there is value to adding another message between TakeOffline() and BringOnline() since nothing else occurs between their calls.

Agree. LGTM

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 59 at r6 (raw file):

    )

    if (! $sql)

There is a blank space here between exclamation mark and the variable, which is not done in other places. Remove the blank space here or change the other places?

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r6 (raw file):

Default is 'MSSQLServer'

Please change to 'MSSQLSERVER' as the default instance name in the description (same as on the parameters).
Please change that in all of the comment based help comments, the README.md and the schema.mof.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 263 at r6 (raw file):

    $serverObject = Connect-SQL -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

    if ($serverObject.IsClustered)

Wouldn't be a good idea to check whether $serverObject is actually assigned before accessing properties from it? If it not set, then the Else-block would be called. It think it would be better to throw an error if the $serverObject does not contain the expected object.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

I think that "hostname" and "DNS name" are synonymous.

Okay. Let's leave it as is. :)

LGTM

---

Comments from Reviewable

[johlju]

@nabrond Thanks for adding the function New-WarningMessage. One step closer to get all strings localized 😄

[nabrond]

Review status: 6 of 7 files reviewed at latest revision, 11 unresolved discussions.

---

README.md, line 354 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add for which resource a test was added? See example under 2.0.0.0 below

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 140 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Awesome! Please add the parameter to the README.md as well 😄

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 59 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

There is a blank space here between exclamation mark and the variable, which is not done in other places. Remove the blank space here or change the other places?

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Default is 'MSSQLServer'

Please change to 'MSSQLSERVER' as the default instance name in the description (same as on the parameters).
Please change that in all of the comment based help comments, the README.md and the schema.mof.

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 263 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Wouldn't be a good idea to check whether $serverObject is actually assigned before accessing properties from it? If it not set, then the Else-block would be called. It think it would be better to throw an error if the $serverObject does not contain the expected object.

That logic seems to be handled in the Connect-SQL function around line 48.

---

Comments from Reviewable

[johlju]

Verified the cluster code. Seems perfect. Nice job!

Still haven't reviewed the tests, will do as soon as possible.

---

Reviewed 3 of 3 files at r7.
Review status: 6 of 7 files reviewed at latest revision, 6 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 263 at r6 (raw file):

Previously, nabrond (Brandon) wrote…

That logic seems to be handled in the Connect-SQL function around line 48.

You are right. `Connect-SQL` throws if it is not set. LGTM.

---

Comments from Reviewable

[mbreakey3]

Review status: 6 of 7 files reviewed at latest revision, 1 unresolved discussion.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

@mbreakey3 I think this would be good to add to the style guideline maybe? The part that if unused non-mandatory parameters are added to the Get-method then to the comment based help "add a comment for that param in the help comment above that it's not used in this function".
Could you look at get that into a guideline? Or I could submit an issue also.

@mbreakey3 I raised this question in another PR as well, see PR #147. It would be awesome if you could read and comment on that too, and see if that is something that needs to be added to a guideline as well.
If you don't have any other comments than what was said here, then on that PR I will add a comment to do the same as was done here, until there is a guideline that says differently. :)

@johlju - Yeah, if you could open an issue on this in [DscResources](https://github.com/PowerShell/DscResources) that would be great

---

Comments from Reviewable

[mbreakey3]

Reviewed 2 of 5 files at r6, 3 of 3 files at r7.
Review status: 6 of 7 files reviewed at latest revision, 7 unresolved discussions.

---

xSQLServerHelper.psm1, line 151 at r7 (raw file):

        $WarningType,

        [Parameter(Mandatory = $false)]

You can take out this line since Mandatory = $false is the default

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):

# Load Common Code
Import-Module $currentPath\..\..\xSQLServerHelper.psm1 -Verbose:$false -ErrorAction Stop

This should use 'Join-Path' instead of '' Also, Verbose:$false can be removed

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 112 at r7 (raw file):

        $SQLServer,

        [Parameter(Mandatory = $false)]

remove this line:

[Parameter(Mandatory = $false)]

since it's the default value

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 178 at r7 (raw file):

    The desired value of the SQL configuration option

    .PARAMETER RestartService

Any parameter that's not being used in Test-TargetResource, should have a comment here: 'Not used in this function'

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 248 at r7 (raw file):

    (
        [Parameter(Mandatory = $true)]
        [string]

[String]

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 277 at r7 (raw file):

        ## Start the SQL Agent resource
        New-VerboseMessage -Message 'Bringing the SQL Server resources online.'

Can you add a more descriptive Verbose message so that the user knows this is a new Service that is being brought online rather than the one that was just brought offline?

---

Comments from Reviewable

[johlju]

@mbreakey3 could you do a review loop thru the test in this PR also?

---

Reviewed 1 of 5 files at r6.
Review status: all files reviewed at latest revision, 22 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

@johlju - Yeah, if you could open an issue on this in DscResources that would be great

Done so. 😄 Closing this comment.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 86 at r7 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        Context 'The system is not in the desired state' {

You should add a context block for testing when the system is in desired state (for the Get-method)

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 91 at r7 (raw file):

            It 'Should return the same values as passed for property SQLServer' {
                $result.SQLServer | Should Be $desiredState.SQLServer

Those It-blocks that used $desireState.<property> you could gather them into one It-block. To something like It 'Should return the same values as passed as parameters'

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 110 at r7 (raw file):

            }

            It 'Should cause Test-TargetResource to return false' {

This It-block should be moved to the Describe-block that tests the Test-method

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 115 at r7 (raw file):

            It 'Should call Connect-SQL mock when getting the current state' {
                Get-TargetResource @desiredState

Two options here

1. You should not need this Get-TargetResource, since you called it after the Context block. And if you remove Get-TargetResource I think you should change the Assert-MockCalled scope to -Scope Context
2. If you want to keep Get-TargetResource, on the next line you should change the Assert-MockCalled scope to -Scope It.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 120 at r7 (raw file):

            It 'Should call New-TerminatingError mock when a bad option name is specified' {
                { Get-TargetResource @invalidOption } | Should Throw

Please add a check so that only the correct Throw is caught (tested OK), like | Should Throw "This is a test". Throw that message you are testing from your mock.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 121 at r7 (raw file):

            It 'Should call New-TerminatingError mock when a bad option name is specified' {
                { Get-TargetResource @invalidOption } | Should Throw
                Assert-MockCalled -ModuleName $script:DSCResourceName -CommandName New-TerminatingError -Scope Describe -Times 1

Change the Assert-MockCalled scope to -Scope It.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 150 at r7 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        ## Test-TargetResource should return true when in the desired state

Need to add a test when Test-TargetResource it is not in desired state also

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 214 at r7 (raw file):

            }

            It 'Should warn about restart when required, but not requested' {

How do we know it did warn? Think we need to Mock and Assert the New-WarningMessage here?

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 220 at r7 (raw file):

            It 'Should call Connect-SQL to get option values' {
                Assert-MockCalled -ModuleName $script:DSCResourceName -CommandName Connect-SQL -Scope Describe -Times 3

Could you change scope to -Scope Context here?

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 279 at r7 (raw file):

                Mock -CommandName Start-Service {} -Verifiable

                It 'Should not throw an exception when restarting a default instance' {

I don't see how the function would ever throw? And you are mocking everything so it can never throw. Maybe add a mock Get-Service, Get-WmiObject and Restart-Service that will throw if passed a value that was "not expected"?
Also add mocks to the cluster part here also (Get-WmiObject) where the mock throws. So if it is hit, then an error will be thrown and the test will fail.

Do the same for the cluster part below.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 283 at r7 (raw file):

                }

                It 'Should not throw an exception when restarting an instance with no SQL Agent' {

Same as previous comment

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 288 at r7 (raw file):

                It 'Should use mock methods a specific number of times' {
                    Assert-MockCalled -CommandName Get-Service -Scope Context -Exactly -Times 2

Assert these in each It-block instead, with scope set to -Scope It

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 344 at r7 (raw file):

                } -Verifiable -ParameterFilter { $Query -match "^ASSOCIATORS OF" }

                It 'Should not throw an exception when restarting a default instance' {

Please do the same as for the previous context block

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 348 at r7 (raw file):

                }

                It 'Should not throw an exception when restarting a named instance' {

Please do the same as for the previous context block

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 353 at r7 (raw file):

                It 'Should use mock methods a specific number of times' {
                    Assert-MockCalled -Scope Context -CommandName Get-WmiObject -Exactly -Times 4

Assert these in each It-block instead, with scope set to -Scope It

---

Comments from Reviewable

[nabrond]

Merge conflict has been resolved.

---

Review status: 3 of 7 files reviewed at latest revision, 23 unresolved discussions.

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 2 files at r8, 3 of 3 files at r9.
Review status: all files reviewed at latest revision, 25 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

Could you change this to use similar formatting as [this file](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_ServiceResource.Tests.ps1#L5) rather than using piping?

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r9 (raw file):

{
    [CmdletBinding()]
    [OutputType([hashtable])]

These capitalized types were fine as they were. You don't need the 'System...' but the type should still be capitalized. As in 'Boolean' and 'Int32'

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 268 at r9 (raw file):

        New-VerboseMessage -Message 'Getting active cluster resource SQL Server Agent'
        $agentService = Get-WmiObject -Namespace root/MSCluster -Query "ASSOCIATORS OF {$sqlService} WHERE ResultClass = MSCluster_Resource" | Where-Object { ($_.Type -eq "SQL Server Agent") -and ($_.State -eq 2) }

Could you split this up into 2+ lines to make it more readable?

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 61 at r7 (raw file):

Previously, nabrond (Brandon) wrote…

The #regions came from the template. They do nothing in my editor, so I removed them. Done.

Thanks! Sorry about that, we've recently been in the process of updating our templates, so I forget what we have in there currently.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 449 at r9 (raw file):

                        $mock | Add-Member -MemberType ScriptMethod -Name TakeOffline -Value { param ( [int] $TimeOut ) }
                        $mock | Add-Member -MemberType ScriptMethod -Name BringOnline -Value { param ( [int] $Timeout ) }

could you ensure that the $TimeOut variable capitalization is consistent? Either $Timeout or $TimeOut

---

Comments from Reviewable

[nabrond]

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Could you change this to use similar formatting as this file rather than using piping?

Since this is multiple directories up, are you expecting something like this?

Import-Module -Name (Join-Path -Path (Split-Path $PSScriptRoot -Parent) `
                               -ChildPath '..\xSQLServerHelpers.psm1') `
                               -Force

---

Comments from Reviewable

[nabrond]

Review status: 5 of 7 files reviewed at latest revision, 20 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r9 (raw file):

Previously, mbreakey3 (Mariah) wrote…

These capitalized types were fine as they were. You don't need the 'System...' but the type should still be capitalized. As in 'Boolean' and 'Int32'

Done.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 268 at r9 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Could you split this up into 2+ lines to make it more readable?

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 449 at r9 (raw file):

Previously, mbreakey3 (Mariah) wrote…

could you ensure that the $TimeOut variable capitalization is consistent? Either $Timeout or $TimeOut

Done.

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 2 files at r10.
Review status: 6 of 7 files reviewed at latest revision, 18 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):

Previously, nabrond (Brandon) wrote…

Since this is multiple directories up, are you expecting something like this?

Import-Module -Name (Join-Path -Path (Split-Path $PSScriptRoot -Parent) `
                               -ChildPath '..\xSQLServerHelpers.psm1') `
                               -Force

Almost:

Import-Module -Name (Join-Path -Path (Split-Path (Split-Path $PSScriptRoot -Parent) -Parent) `
                               -ChildPath 'CommonResourceHelper.psm1') `
                               -Force

^I think that should do it.
We're trying to get rid of all of the '\' since that's not guaranteed to work on all OS's 

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/powershell/xsqlserver/143)*
<!-- Sent from Reviewable.io -->

[mbreakey3]

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Almost:

Import-Module -Name (Join-Path -Path (Split-Path (Split-Path $PSScriptRoot -Parent) -Parent) `
                               -ChildPath 'CommonResourceHelper.psm1') `
                               -Force

^I think that should do it.
We're trying to get rid of all of the '' since that's not guaranteed to work on all OS's

Sorry, replace 'CommonResourceHelper.psm1' with 'xSQLServerHelpers.psm1'

---

Comments from Reviewable

[nabrond]

Review status: 5 of 8 files reviewed at latest revision, 16 unresolved discussions.

---

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Sorry, replace 'CommonResourceHelper.psm1' with 'xSQLServerHelpers.psm1'

Done.

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 1 files at r11.
Review status: 6 of 7 files reviewed at latest revision, 16 unresolved discussions.

---

Comments from Reviewable

[johlju]

Reviewed 1 of 2 files at r8, 1 of 3 files at r9, 1 of 2 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 120 at r7 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

Sorry I was unclear. You didn't have to add another It-block. Please just replace `Should Throw` with `Should Throw 'ConfigurationOptionNotFound'`. And remove the It-block below. Concatenating the two It-blocks will result in the same test.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 20 at r11 (raw file):

$defaultState = @{
    SQLServer = "CLU01"

Please change to ' on all of these strings in all of the hash tables.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 82 at r11 (raw file):

                        Properties = @(
                            @{
                                DisplayName = "user connections"

Use ' around this string

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 119 at r11 (raw file):

                        Properties = @(
                            @{
                                DisplayName = "user connections"

Use ' around this string

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 156 at r11 (raw file):

                        Properties = @(
                            @{
                                DisplayName = "user connections"

Use ' around this string

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 193 at r11 (raw file):

                    Properties = @(
                        @{
                            DisplayName = "user connections"

Please surround the string with '

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 225 at r11 (raw file):

                    Properties = @(
                        @{
                            DisplayName = "user connections"

Please surround the string with '

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 244 at r11 (raw file):

                    Properties = @(
                        @{
                            DisplayName = "show advanced options"

Please surround the string with '

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 295 at r11 (raw file):

                Mock -CommandName Connect-SQL -MockWith {
                    return @{
                        Name = "MSSQLSERVER"

Please surround the string with '. Do this for all the strings below as well (those without variables in them of course).

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 394 at r11 (raw file):

                Mock -CommandName Connect-SQL -MockWith {
                    return @{
                        Name = "MSSQLSERVER"

Please surround the string with '. Do this for all the strings below as well (those without variables in them of course).

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 456 at r11 (raw file):

                It 'Should restart SQL Server and SQL Agent resources for a clustered default instance' {
                    { Restart-SqlService -SQLServer "CLU01" -SQLInstanceName "MSSQLSERVER" } | Should Not Throw

Please surround the string with '.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 466 at r11 (raw file):

                It 'Should restart SQL Server and SQL Agent resources for a clustered named instance' {
                    { Restart-SqlService -SQLServer "CLU01" -SQLInstanceName "NAMEDINSTANCE" } | Should Not Throw

Please surround the string with '.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 476 at r11 (raw file):

                It 'Should not try to restart a SQL Agent resource that is not online' {
                    { Restart-SqlService -SQLServer "CLU01" -SQLInstanceName "STOPPEDAGENT" } | Should Not Throw

Please surround the string with '.

---

Comments from Reviewable

[nabrond]

Review status: 6 of 7 files reviewed at latest revision, 13 unresolved discussions.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 120 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Sorry I was unclear. You didn't have to add another It-block. Please just replace Should Throw with Should Throw 'ConfigurationOptionNotFound'. And remove the It-block below. Concatenating the two It-blocks will result in the same test.

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 20 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to ' on all of these strings in all of the hash tables.

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 82 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Use ' around this string

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 119 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Use ' around this string

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 156 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Use ' around this string

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 193 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please surround the string with '

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 225 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please surround the string with '

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 244 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please surround the string with '

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 295 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please surround the string with '. Do this for all the strings below as well (those without variables in them of course).

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 394 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please surround the string with '. Do this for all the strings below as well (those without variables in them of course).

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 456 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please surround the string with '.

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 466 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please surround the string with '.

Done.

---

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 476 at r11 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please surround the string with '.

Done.

---

Comments from Reviewable

[johlju]

---

Reviewed 1 of 1 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[johlju]

@mbreakey3 Can you take a glance at this and merge when you feel it looks okay?

[mbreakey3]

Reviewed 1 of 1 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable