xSQLServerSetup: Add support for clustered installations (Fixes #227)

[nabrond]

Added support to xSQLServerSetup for clustered installations. Ported code from existing xSQLServerFailoverCluster resource and updated to correct issues and add additional functionality.

• Added support for multi-subnet clusters
• Corrected clustered disk detection issue with asymmetric storage
• Removed Get-WmiObject calls
• Added localized error messages

This Pull Request (PR) fixes the following issues:
Fixes #227

• Change details added to Unreleased section of CHANGELOG.md?
• Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
• Examples appropriately updated?
• New/changed code adheres to Style Guidelines?
• Unit and (optional) Integration tests created/updated where possible?

---

This change is 

[johlju]

@nabrond Awesome! Waited for this one. 😄 I'm gonna review this one during the weekend.

[johlju]

Like the way you improved the tests! 👍
I will continue the review tomorrow. I just glanced over the easy stuff. It gonna take all day tomorrow to review this one 😉

---

Reviewed 3 of 6 files at r1, 1 of 3 files at r3.
Review status: 4 of 6 files reviewed at latest revision, 7 unresolved discussions.

---

README.md, line 754 at r3 (raw file):

* Target machine must be running Windows Server 2008 R2.

#### Parameters

Missing the Action parameter here. Please see the parameter 'BrowserSvcStartupType' for format.

---

README.md, line 796 at r3 (raw file):

* **ISSvcAccount** _(Write)_: Service account for Integration Services service.
* **[String] BrowserSvcStartupType** _(Write)_: Specifies the startup mode for SQL Server Browser service. { Automatic | Disabled | 'Manual' }
* **[String] FailoverClusterGroup**: The name of the resource group to create for the clustered SQL Server instance

Please add the type qualifier (Write) for all these new parameters. See format on the previous.

Also, please end the sentences with a full stop ('.').

---

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

class MSFT_xSQLServerSetup : OMI_BaseResource
{
    [Write, Description("The action to be performed. Default value is 'Install'."), ValueMap{"Install","InstallFailoverCluster","AddNode","PrepareFailoverCluster","CompleteFailoverCluster"}, Values{"Install","InstallFailoverCluster","AddNode","PrepareFailoverCluster","CompleteFailoverCluster"}]  String Action;

There is an extra white space between ] and 'String Action'

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.schema.mof, line 51 at r3 (raw file):

    [Read, Description("Output username for the Integration Services service.")] String ISSvcAccountUsername;
    [Write, Description("Specifies the startup mode for SQL Server Browser service."), ValueMap{"Automatic", "Disabled", "Manual"}, Values{"Automatic", "Disabled", "Manual"}] String BrowserSvcStartupType;
    [Write, Description("The name of the resource group to create for the clustered SQL Server instance")] String FailoverClusterGroup;

Please end the sentence with a full stop ('.').

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.schema.mof, line 52 at r3 (raw file):

    [Write, Description("Specifies the startup mode for SQL Server Browser service."), ValueMap{"Automatic", "Disabled", "Manual"}, Values{"Automatic", "Disabled", "Manual"}] String BrowserSvcStartupType;
    [Write, Description("The name of the resource group to create for the clustered SQL Server instance")] String FailoverClusterGroup;
    [Write, Description("Array of IP Addresses to be assigned to the clustered SQL Server instance")] String FailoverClusterIPAddress[];

Please end the sentence with a full stop ('.').

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 102 at r3 (raw file):

        $mockSqlServiceAccount = 'COMPANY\SqlAccount'
        $mockAgentServiceAccount = 'COMPANY\AgentAccount'
        $mockSourceFolder = 'Source' #  The parameter SourceFolder has a default value of 'Source', so lets mock that as well.

This row should not be needed any more.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 702 at r3 (raw file):

            $argumentHashTable = @{}

            ## break the argument string into a hash table

Single hastag and start chnage to 'Break' (upper 'B')

---

Comments from Reviewable

[nabrond]

Thanks for the feedback on the test changes @johlju! I wanted to convert the setup arguments array to a hashtable and needed a reliable method for testing command line arguments since there was no guaranteed order. I took care of the comments you have made so far and went ahead and cleaned up some of the stylistic problems across all files. If you can get through the rest of the code this weekend, I hope we can close this PR out this week and move on!

---

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

---

README.md, line 754 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missing the Action parameter here. Please see the parameter 'BrowserSvcStartupType' for format.

Done.

---

README.md, line 796 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add the type qualifier (Write) for all these new parameters. See format on the previous.

Also, please end the sentences with a full stop ('.').

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

There is an extra white space between ] and 'String Action'

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.schema.mof, line 51 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please end the sentence with a full stop ('.').

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.schema.mof, line 52 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please end the sentence with a full stop ('.').

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 102 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This row should not be needed any more.

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 702 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Single hastag and start chnage to 'Break' (upper 'B')

Done. This will be an issue all over my code. The double hash is a habit I cannot seem to break!

---

Comments from Reviewable

[johlju]

I will make sure to get thru the review today. That is my plan at least. 😄

A few comments to get things started.
(have to reboot the PC for new build of Windows 10 - involuntary break 😉 )

---

Reviewed 2 of 4 files at r4.
Review status: 4 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

README.md, line 754 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

The tests faild on the README.md. Please resolve these errors.

README.md: 754: MD022 Headers should be surrounded by blank lines
README.md: 755: MD032 Lists should be surrounded by blank lines

---

README.md, line 755 at r4 (raw file):

#### Parameters
* **[String] Action** _(Write)_: The action to be performed. Default value is 'Install'. {'Install' | 'InstallFailoverCluster' | 'AddNode' | 'PrepareFailoverCluster' | 'CompleteFailoverCluster'}

Please add white space after { and before }
Please remove ' from the values between the braces.
Please make 'Install' Italic since it is the default; *'Itallic'*

I make an issue to get this format into the specific contribution guidelines.

---

Comments from Reviewable

[nabrond]

Review status: 4 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

README.md, line 754 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The tests faild on the README.md. Please resolve these errors.

README.md: 754: MD022 Headers should be surrounded by blank lines
README.md: 755: MD032 Lists should be surrounded by blank lines

Done.

---

README.md, line 755 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add white space after { and before }
Please remove ' from the values between the braces.
Please make 'Install' Italic since it is the default; *'Itallic'*

I make an issue to get this format into the specific contribution guidelines.

Done.

---

Comments from Reviewable

[johlju]

This is awesome! I like the way you improved the code! I found just small thing, nothing big.

Would you be so kind to add examples for each installation option? At least with the minimum required parameters. Then we can improve those examples later.
If so, please create a new folder for the resource in the Examples folder, and see please use a previous example as a template. The function must have a certain name for the tests to be able to verify them.

I'm gonna review the tests next. I will take a little break first.

A question. have you ran this code in a lab to verify? I do not have cluster disks in my lab at the moment, so can't actually test the cluster bits. I'm hoping you have done that. If not I will install a storage server so I can get some cluster disks.

---

Reviewed 1 of 4 files at r4.
Review status: 5 of 6 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

README.md, line 414 at r4 (raw file):

None.

### xSQLServerFailoverClusterSetup

Could you add to the description that this resource is deprecated and will be removed at a later time and will be replaced by xSQLServerSetup.
Please add that to the change log as well.

---

README.md, line 796 at r4 (raw file):

* **ISSvcAccount** _(Write)_: Service account for Integration Services service.
* **[String] BrowserSvcStartupType** _(Write)_: Specifies the startup mode for SQL Server Browser service. { Automatic | Disabled | 'Manual' }
* **[String] FailoverClusterGroup** _(Write)_: The name of the resource group to create for the clustered SQL Server instance.

Could we call this parameter FailOverClusterGroupName instead?

---

README.md, line 797 at r4 (raw file):

* **[String] BrowserSvcStartupType** _(Write)_: Specifies the startup mode for SQL Server Browser service. { Automatic | Disabled | 'Manual' }
* **[String] FailoverClusterGroup** _(Write)_: The name of the resource group to create for the clustered SQL Server instance.
* **[IPAddress[]]FailoverClusterIPAddress** _(Write)_: Array of IP Addresses to be assigned to the clustered SQL Server instance.

Type is wrong, should be String[]

Please add to the description the format of these IP addresses.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 27 at r4 (raw file):

        Name of the SQL instance to be installed.

    .PARAMETER FailoverClusterGroup

These should not be here, since they are (correctly) not parameters in the Get-method :)

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 169 at r4 (raw file):

            New-VerboseMessage -Message 'Clustered instance detected'

            $clusteredSqlInstance = Get-CimInstance -Namespace root/MSCluster -ClassName MSCluster_Resource -Filter "Type = 'SQL Server'" |

We should also check for resource "Analysis Services (INSTANCENAME)" if Analysis Services is installed in a Cluster Group without Database Engine.
This could be added later. If so please, create an issue for this, so it will be done at a later time. :)

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 172 at r4 (raw file):

                Where-Object { $_.PrivateProperties.InstanceName -eq $InstanceName }

            if ($clusteredSqlInstance)

It shouldn't throw error here? It it should be clustered but there are no resources?

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 636 at r4 (raw file):

        $BrowserSvcStartupType,

        [Parameter(ParameterSetName = 'ClusterInstall')]

Does ParameterSetName fill any purpose other than to visualize (to Group) the parameters for Cluster together?

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 788 at r4 (raw file):

    }

    $setupArgs = @{}

Please change the variable $setupArgs to $setupArguments. Let's avoid abbreviations.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 792 at r4 (raw file):
This is the cluster validation, checking disks an so on. I think this must be a parameter so user can control the behavior.
If you like, you can submit an issue for this so it is solved later. That would make it a breaking change though, because a previous configuration (when run again) could break if the cluster is not passing validation.

For a SQL Server 2008 failover cluster instance to be in a supported scenario, the Windows Server 2008 Cluster Validation Report cannot contain errors. Confirm with Microsoft CSS that the cluster configuration is in a supported state.
The SkipRules parameter for setup is not a documented feature. You should not use this parameter to skip any other rules except the Cluster_VerifyForErrors rule unless Microsoft CSS directs you to do this.
https://support.microsoft.com/en-us/help/953748/error-message-when-you-install-sql-server-2008-on-a-windows-server-2008-based-cluster-the-cluster-either-has-not-been-verified-or-there-are-errors-or-failures-in-the-verification-report.-refer-to-kb953748-or-sql-server-books-online

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 803 at r4 (raw file):

Could this be used on the pipeline instead? Then you don't have to trim, and Select-Object -Unique is case-sensitive (C: and c: is not the same).

| Split-Path -Qualifier | Sort-Object -Unique

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 807 at r4 (raw file):

        # Get the disk resources that are available (not assigned to a cluster role)
        $availableStorage = Get-CimInstance -Namespace 'root/MSCluster' -ClassName 'MSCluster_ResourceGroup' -Filter "Name = 'Available Storage'" |

Could we make use of the cmdlet Get-ClusterAvailableDisk? Then we don't have to make sure we keep up with all edge case that this will fail? At a minimum you should only return Resource Type 'Physical Disk' (?).
Even if it should never happen, it is possible to get other resource types in Available Storage, I have seen it first hand.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 826 at r4 (raw file):

        # Ensure we have a unique listing of disks
        $failoverClusterDisks = $failoverClusterDisks | Select-Object -Unique

Same issue with case-sensitivity here? Use Sort-Object instead?

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 829 at r4 (raw file):

        # Ensure we mapped all required drives
        $requiredDriveCount = $requiredDrives | Measure-Object | Select-Object -ExpandProperty Count

Thought. Do you really need Measure-Object? 'MyString'.Count returns 1, and '('MyString1','MyString2').Count returns 2. And unassigned variable '$dkjdkj.Count' returns 0. Don't see the need for Measure-Object. Is there a scenario where this is need that I can't see?

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 830 at r4 (raw file):

        # Ensure we mapped all required drives
        $requiredDriveCount = $requiredDrives | Measure-Object | Select-Object -ExpandProperty Count
        $mappedDriveCount = $failoverClusterDisks | Measure-Object | Select-Object -ExpandProperty Count

Same thought about Measure-Object as above.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 841 at r4 (raw file):

    }

    # Detemine network mapping for specific cluster installation types

Typo here 'Determine'

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 846 at r4 (raw file):

        $clusterIPAddresses = @()

        # If no IP has been specified, configure DHCP on the first client network

Please add to the parameter description in the README (and comment-based help) that this is the case, when not specified it uses DHCP on the first client network.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 847 at r4 (raw file):

        # If no IP has been specified, configure DHCP on the first client network
        if (($FailoverClusterIPAddress | Measure-Object).Count -eq 0)

Same thought about Measure-Object as above.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 872 at r4 (raw file):

        # Ensure we mapped all required networks
        $suppliedNetworkCount = $FailoverClusterIPAddress | Measure-Object | Select-Object -ExpandProperty Count

Same thought about Measure-Object as above.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 873 at r4 (raw file):

        # Ensure we mapped all required networks
        $suppliedNetworkCount = $FailoverClusterIPAddress | Measure-Object | Select-Object -ExpandProperty Count
        $mappedNetworkCount = $clusterIPAddresses | Measure-Object | Select-Object -ExpandProperty Count

Same thought about Measure-Object as above.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 875 at r4 (raw file):

        $mappedNetworkCount = $clusterIPAddresses | Measure-Object | Select-Object -ExpandProperty Count

        # Determine whether we have mapping issues

Add to the comment
"...issues for the IP addresses"

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1004 at r4 (raw file):

    # Build the argument string to be passed to setup
    $arguments = ''
    foreach ($setupArg in $setupArgs.GetEnumerator())

Please change $setupArg to $currentSetupArgument so it is easily distinguished from $setupArguments.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1007 at r4 (raw file):

    {
        if ($setupArg.Value -ne '')
            {

Brace has moved one indent to far.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1012 at r4 (raw file):

            {
                # Sort and format the array
                $setupArgValue = ($setupArg.Value | Sort-Object | ForEach-Object { '"{0}"' -f $_ }) -join ' '

Write out the variable name '$setupArgumentValue'

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1060 at r4 (raw file):

    New-VerboseMessage -Message "Starting setup using arguments: $log"

    $process = StartWin32Process -Path $pathToSetupExecutable -Arguments $arguments.Trim()

Add a new row above this row? Then Trim() can be remove from this and the one below.
$arguments = $arguments.Trim()

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1410 at r4 (raw file):

is not valid for this cluster

Maybe "is not in desired state for this cluster."?

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1582 at r4 (raw file):

function ConvertTo-Decimal
{
    [CmdletBinding()]

Please add an output type.
PSSA warning: MSFT_xSQLServerSetup.psm1 (Line 1596): The cmdlet 'ConvertTo-Decimal' returns an object of type 'System.UInt32' but this type is not declared in the OutputType attribute.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1662 at r4 (raw file):

        [ValidateSet('SQL','AGT','IS','RS','AS','FT')]
        [String]
        $ServiceType

Comment-based help says 'AccountType', parameter says 'ServiceType'. I'd like 'AccountType' if I may choose ;)

---

Comments from Reviewable

[johlju]

Reviewed 1 of 1 files at r5.
Review status: 5 of 6 files reviewed at latest revision, 27 unresolved discussions.

---

Comments from Reviewable

[johlju]

Reviewed 1 of 4 files at r4.
Review status: all files reviewed at latest revision, 42 unresolved discussions.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 577 at r5 (raw file):

                (
                    New-Object Microsoft.Management.Infrastructure.CimInstance 'MSCluster_Resource','root/MSCluster' |
                        Add-Member -MemberType NoteProperty -Name 'Name' -Value "SQL Server ($mockInstanceName)" -PassThru -Force |

Please change @mockInstanceName to $mockCurrentInstancename so it is more clear it is dynamically set.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 579 at r5 (raw file):

                        Add-Member -MemberType NoteProperty -Name 'Name' -Value "SQL Server ($mockInstanceName)" -PassThru -Force |
                        Add-Member -MemberType NoteProperty -Name 'Type' -Value 'SQL Server' -TypeName 'String' -PassThru -Force | 
                        Add-Member -MemberType NoteProperty -Name 'PrivateProperties' -Value @{ InstanceName = $mockInstanceName } -PassThru -Force

Please change @mockInstanceName to $mockCurrentInstancename so it is more clear it is dynamically set.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 594 at r5 (raw file):

        $mockGetCimInstance_MSClusterNetwork = {

Remove this blank line

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 707 at r5 (raw file):

$argumentHashTable.Add($key, $value) | Out-Null

Minor: It think this method is faster

$null = $argumentHashTable.Add($key, $value)

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 711 at r5 (raw file):

            }

            foreach ($argumentKey in $mockStartWin32ProcessExpectedArgument.Keys)

You should also test so there are not to many or to few arguments in the $ArgumentHashTable than $mockStartWin32ProcessExpectedArgument.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 1752 at r5 (raw file):

                        }

                        $mockInstanceName = $mockDefaultInstance_InstanceName

Please change @mockInstanceName to $mockCurrentInstancename so it is more clear it is dynamically set.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 1769 at r5 (raw file):

                    }

                    It 'Should return the same values passsed as parameters' {

This description seems wrong. Maybe it should be "Should collect cluster information for a standalone instance"?

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2259 at r5 (raw file):

'Should return that the desired state is present for a clustered instance'

Maybe 'Should return that the desired state is present when the correct clustered instance was found'?

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2260 at r5 (raw file):

                It 'Should return that the desired state is present for a clustered instance' {
                    $mockInstanceName = $mockDefaultInstance_InstanceName

Please change @mockInstanceName to $mockCurrentInstancename so it is more clear it is dynamically set.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2521 at r5 (raw file):

                            $testParameters.Features = 'ADV_SSMS'

                            $mockStartWin32ProcessExpectedArgument =

This needs to be in the new format?

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2584 at r5 (raw file):

                    It 'Should set the system in the desired state when feature is SQLENGINE' {
                        $mockStartWin32ProcessExpectedArgument =

This needs to be in the new format?

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2628 at r5 (raw file):

                        It 'Should throw when feature parameter contains ''SSMS'' when installing SQL Server 2016' {
                            $testParameters.Features = 'SSMS'
                            $mockStartWin32ProcessExpectedArgument = ''

This needs to be in the new format?

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2635 at r5 (raw file):

                        It 'Should throw when feature parameter contains ''ADV_SSMS'' when installing SQL Server 2016' {
                            $testParameters.Features = 'ADV_SSMS'
                            $mockStartWin32ProcessExpectedArgument = ''

This needs to be in the new format?

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2643 at r5 (raw file):

                            $testParameters.Features = 'SSMS'

                            $mockStartWin32ProcessExpectedArgument =

This needs to be in the new format?

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2679 at r5 (raw file):

                            $testParameters.Features = 'ADV_SSMS'

                            $mockStartWin32ProcessExpectedArgument =

This needs to be in the new format?

---

Comments from Reviewable

[nabrond]

Thanks! I am pretty happy with how it came out. I still need to run this code in my lab yet. I am hoping to get that done this week.

---

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

---

README.md, line 414 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add to the description that this resource is deprecated and will be removed at a later time and will be replaced by xSQLServerSetup.
Please add that to the change log as well.

Done.

---

README.md, line 796 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we call this parameter FailOverClusterGroupName instead?

Done.

---

README.md, line 797 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Type is wrong, should be String[]

Please add to the description the format of these IP addresses.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 27 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

These should not be here, since they are (correctly) not parameters in the Get-method :)

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 169 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should also check for resource "Analysis Services (INSTANCENAME)" if Analysis Services is installed in a Cluster Group without Database Engine.
This could be added later. If so please, create an issue for this, so it will be done at a later time. :)

I will put this in as an issue to be handled in a later PR.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 172 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It shouldn't throw error here? It it should be clustered but there are no resources?

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 636 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Does ParameterSetName fill any purpose other than to visualize (to Group) the parameters for Cluster together?

Not any more. Removed. Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 788 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change the variable $setupArgs to $setupArguments. Let's avoid abbreviations.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 792 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This is the cluster validation, checking disks an so on. I think this must be a parameter so user can control the behavior.
If you like, you can submit an issue for this so it is solved later. That would make it a breaking change though, because a previous configuration (when run again) could break if the cluster is not passing validation.

For a SQL Server 2008 failover cluster instance to be in a supported scenario, the Windows Server 2008 Cluster Validation Report cannot contain errors. Confirm with Microsoft CSS that the cluster configuration is in a supported state.
The SkipRules parameter for setup is not a documented feature. You should not use this parameter to skip any other rules except the Cluster_VerifyForErrors rule unless Microsoft CSS directs you to do this.
https://support.microsoft.com/en-us/help/953748/error-message-when-you-install-sql-server-2008-on-a-windows-server-2008-based-cluster-the-cluster-either-has-not-been-verified-or-there-are-errors-or-failures-in-the-verification-report.-refer-to-kb953748-or-sql-server-books-online

Going to submit an issue on this one.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 803 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could this be used on the pipeline instead? Then you don't have to trim, and Select-Object -Unique is case-sensitive (C: and c: is not the same).

| Split-Path -Qualifier | Sort-Object -Unique

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 807 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we make use of the cmdlet Get-ClusterAvailableDisk? Then we don't have to make sure we keep up with all edge case that this will fail? At a minimum you should only return Resource Type 'Physical Disk' (?).
Even if it should never happen, it is possible to get other resource types in Available Storage, I have seen it first hand.

The cmdlet Get-ClusterAvailableDisk gets disks that are accessible to all nodes in the cluster, but have not yet been registered within the cluster. I believe this task is outside the scope of this resource.

I have only ever seen registered, unassigned storage resources (disks, volume groups, etc.) show in "Available Storage". This code is heavily dependent on the class structure within WMI to ensure that resources are not incorrectly associated.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 826 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same issue with case-sensitivity here? Use Sort-Object instead?

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 829 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Thought. Do you really need Measure-Object? 'MyString'.Count returns 1, and '('MyString1','MyString2').Count returns 2. And unassigned variable '$dkjdkj.Count' returns 0. Don't see the need for Measure-Object. Is there a scenario where this is need that I can't see?

Done. This is another old habit pattern that I need to shake.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 830 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same thought about Measure-Object as above.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 841 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Typo here 'Determine'

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 846 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add to the parameter description in the README (and comment-based help) that this is the case, when not specified it uses DHCP on the first client network.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 847 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same thought about Measure-Object as above.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 872 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same thought about Measure-Object as above.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 873 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same thought about Measure-Object as above.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 875 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add to the comment
"...issues for the IP addresses"

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1004 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change $setupArg to $currentSetupArgument so it is easily distinguished from $setupArguments.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1007 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Brace has moved one indent to far.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1012 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Write out the variable name '$setupArgumentValue'

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1060 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a new row above this row? Then Trim() can be remove from this and the one below.
$arguments = $arguments.Trim()

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1410 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

is not valid for this cluster

Maybe "is not in desired state for this cluster."?

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1582 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add an output type.
PSSA warning: MSFT_xSQLServerSetup.psm1 (Line 1596): The cmdlet 'ConvertTo-Decimal' returns an object of type 'System.UInt32' but this type is not declared in the OutputType attribute.

Done.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 1662 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Comment-based help says 'AccountType', parameter says 'ServiceType'. I'd like 'AccountType' if I may choose ;)

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 577 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change @mockInstanceName to $mockCurrentInstancename so it is more clear it is dynamically set.

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 579 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change @mockInstanceName to $mockCurrentInstancename so it is more clear it is dynamically set.

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 594 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Remove this blank line

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 707 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$argumentHashTable.Add($key, $value) | Out-Null

Minor: It think this method is faster

$null = $argumentHashTable.Add($key, $value)

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 711 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You should also test so there are not to many or to few arguments in the $ArgumentHashTable than $mockStartWin32ProcessExpectedArgument.

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 1752 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change @mockInstanceName to $mockCurrentInstancename so it is more clear it is dynamically set.

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 1769 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This description seems wrong. Maybe it should be "Should collect cluster information for a standalone instance"?

I agree, this is worded incorrectly. Changed to "Should collection information for a clustered instance." Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2259 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'Should return that the desired state is present for a clustered instance'

Maybe 'Should return that the desired state is present when the correct clustered instance was found'?

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2260 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change @mockInstanceName to $mockCurrentInstancename so it is more clear it is dynamically set.

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2521 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This needs to be in the new format?

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2584 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This needs to be in the new format?

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2628 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This needs to be in the new format?

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2635 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This needs to be in the new format?

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2643 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This needs to be in the new format?

Done.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2679 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This needs to be in the new format?

Done.

---

Comments from Reviewable

[johlju]

You should be happy! Great work! 😄 👍

---

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

---

README.md, line 797 at r4 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

Missing the format of how to provide the IP address.
Is it '10.0.0.1' or '10.0.0.1/255.0.0.0', or maybe some other format?

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 169 at r4 (raw file):

Previously, nabrond (Brandon) wrote…

I will put this in as an issue to be handled in a later PR.

That will be fine. Write Done on this comment when you have created an issue.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 792 at r4 (raw file):

Previously, nabrond (Brandon) wrote…

Going to submit an issue on this one.

That will be fine. Write Done on this comment when you have created an issue.

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 807 at r4 (raw file):

Previously, nabrond (Brandon) wrote…

The cmdlet Get-ClusterAvailableDisk gets disks that are accessible to all nodes in the cluster, but have not yet been registered within the cluster. I believe this task is outside the scope of this resource.

I have only ever seen registered, unassigned storage resources (disks, volume groups, etc.) show in "Available Storage". This code is heavily dependent on the class structure within WMI to ensure that resources are not incorrectly associated.

Great! Then we leave it as-is.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2628 at r5 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

This wasn't changed?

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2635 at r5 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

This neither? :)

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 1749 at r6 (raw file):

Should Be ''

On all of these throughout, could we use Should BeNullOrEmpty ?

---

Comments from Reviewable

[nabrond]

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

---

README.md, line 797 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missing the format of how to provide the IP address.
Is it '10.0.0.1' or '10.0.0.1/255.0.0.0', or maybe some other format?

Done. Added a note to use dotted-decimal formatting.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2628 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This wasn't changed?

Nothing to change for this test. Set-TargetResource should throw an exception before StartWin32Process is called and the hashtable compared.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 2635 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This neither? :)

See previous comment.

---

Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1, line 1749 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should Be ''

On all of these throughout, could we use Should BeNullOrEmpty ?

Done.

---

Comments from Reviewable

[nabrond]

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

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 169 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

That will be fine. Write Done on this comment when you have created an issue.

Done. (Issue #334)

---

DSCResources/MSFT_xSQLServerSetup/MSFT_xSQLServerSetup.psm1, line 792 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

That will be fine. Write Done on this comment when you have created an issue.

Done. Issue #335.

---

Comments from Reviewable

[johlju]

---

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

---

Comments from Reviewable

[johlju]

I will do a install in my lab with two single instances, if that still works then I merge this one. Then there is no change to the previous functionality.