xSQLServerDatabaseDefaultLocation: New Resource

[PaulFeaser]

Pull Request (PR) description
Created new resource for setting Database Default Locations (Data/Log/Backup) for SQL Server

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

Task list:

• 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 

[codecov-io]

Codecov Report

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

@@         Coverage Diff          @@
##            dev   #856    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        31     32     +1     
  Lines      3442   3498    +56     
====================================
+ Hits       3322   3378    +56     
  Misses      120    120

[msftclas]

All CLA requirements met.

[PaulFeaser]

@johlju The CLA issue has been resolved.

[johlju]

@PaulFeaser Could you tell me how it was resolved so I know for future contributors? I see that you are in the Microsoft org now, but also saw that that didn't help at first.

[johlju]

It would be awesome if you could do an integration test for this resource as well, now when there is possible to run integration tests. Let me know if you are up to doing that as well.
See xSQLServerAlwaysOnService integration test as an example.

---

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 60 unresolved discussions.

---

CHANGELOG.md, line 112 at r1 (raw file):

  - Added support for configuring URL reservations and virtual directory names
    ([issue #570](https://github.com/PowerShell/xSQLServer/issues/570))
- Added resource

There was an recent release of xSQLServer so you need to rebase this PR and make sure to move these entries back to the unreleased section after rebase.

Please use git rebase (and not git merge or git pull).
If you have not rebased before, here's some help; https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts

---

README.md, line 511 at r1 (raw file):

This resource is used to configure database default locations for
and Backup for SQL Server.

maybe add 'user database'. Something like:
"This resource is used to configure the default locations for user databases. The types of default locations that can be changed is Data, Log and Backup."

---

README.md, line 521 at r1 (raw file):

* Target machine must be running SQL Server Database Engine 2008 or later.

#### Parameters

Missing RestartService parameter here.

---

README.md, line 521 at r1 (raw file):

* Target machine must be running SQL Server Database Engine 2008 or later.

#### Parameters

Make sure these parameter descriptions are aligned the same in README.md, schema.mof and comment-based help. See the text differ, so use any text you feel is best.

---

README.md, line 523 at r1 (raw file):

The name

The type

---

README.md, line 524 at r1 (raw file):

* **`[String]` DefaultLocationType** _(Key)_: The name of database default
  location (Data, Log, Backup) to be configured.

Please add the correct format for the values in the validate set. See other resources parameter descriptions as example.

---

README.md, line 525 at r1 (raw file):

The location of the default...

Maybe: "The path to the default..."

---

README.md, line 528 at r1 (raw file):

SQLInstance

Should be SQLInstanceName

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 32 at r1 (raw file):

        [ValidateNotNullOrEmpty()]
        [System.String]
        $SQLInstanceName = 'MSSQLSERVER',

Please remove the default value from mandatory parameter. Throughout.

https://ci.appveyor.com/project/PowerShell/xsqlserver/build/6.0.1663.0#L943

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 58 at r1 (raw file):

    switch ($DefaultLocationType)
    {
        "Data"

Please use single quotes.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 63 at r1 (raw file):

        }

        "Log"

Please use single quotes.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 68 at r1 (raw file):

        }

        "Backup"

Please use single quotes.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 129 at r1 (raw file):

System.boolean

System.Boolean (upper 'B').

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 137 at r1 (raw file):

    $isRestartNeeded = $false

    Write-Verbose -Message ($script:localizedData.VerifyChangeDefaultLocationType -f $DefaultLocationType)

Is this verbose message needed since there was another verbose message two rows up?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 142 at r1 (raw file):

Switch

switch (lower-case 's')

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 167 at r1 (raw file):

$getDefaultLocationPath

This seems to not assigned anywhere. And could the name be changed to maybe $currentValueDefaultLocationPath?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 180 at r1 (raw file):

    }

    if ($RestartService -and $isRestartNeeded)

Why not move this inside the try block and get rid of the $isRestartNeeded variable?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 182 at r1 (raw file):

RestartSQLServer

Please change the key to 'RestartSqlServer' (using lower-case in 'ql' in 'Sql'). Throughout.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 232 at r1 (raw file):

System.boolean

System.Boolean (upper 'B').

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 233 at r1 (raw file):

        [Parameter()]
        [System.boolean]
        $RestartService = $false

Missing this in the comment-based help.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 238 at r1 (raw file):

$parameters

Please change to $getTargetResourceParameters

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 245 at r1 (raw file):

$currentValues

Please change to $getTargetResourceResult.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 247 at r1 (raw file):

$isMDefaultLocationInDesiredState

Why is it a single 'M' here?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 249 at r1 (raw file):

$getDefaultLocationPath

Why not use $currentValues.DefaultLocationPath here directly? _After the above change $getTargetResourceResult.DefaultLocationPath

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.schema.mof, line 4 at r1 (raw file):

class MSFT_xSQLServerDatabaseDefaultLocation : OMI_BaseResource
{
    [Required, Description("The name of the SQL instance to be configured.")] String SQLInstanceName;

This needs to be a key as well, otherwise it's not possible to change this on two instances on the same host.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.schema.mof, line 6 at r1 (raw file):

DefaultLocationType

Wouldn't it be enough with just Type? The resource is called DefaultLocation...

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.schema.mof, line 7 at r1 (raw file):

DefaultLocationPath

Wouldn't it be enough with just Path? The resource is called DefaultLocation...

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 4 at r1 (raw file):

Default File Location

Change to lower-case letters.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 4 at r1 (raw file):

for SQL Instance

"for instance" should be enough?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 5 at r1 (raw file):

Info

Please write out the word to 'Information' (if that is what it should say). Rather not use abbreviations.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 6 at r1 (raw file):

Verifying the default '{0}' file path is changing.

Rephrase. Do you mean verify if it needs to be changed?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 8 at r1 (raw file):

default value
default file location
default location

Maybe use one of these in all strings to be consequent? Throughout in all of these strings.

Maybe use "path" instead of "location" depending on the terminology used in the parameter descriptions?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 12 at r1 (raw file):

The alter command for setting the default location failed

Minor: Think we should skip the reference to the 'alter' word and instead just say that changing the path failed?

Good as-is if you want to keep it.

---

Examples/Resources/xSQLServerDatabaseDefaultLocation/1-SetDatabaseDefaultLocation.ps1, line 10 at r1 (raw file):

[PSCredential]

[System.Management.Automation.PSCredential]

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 44 at r1 (raw file):

"C:\temp"

Single quotes. Also on the two lines below.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 88 at r1 (raw file):

AltLocationPath

Please change to 'AlterLocationPath'. Throughout.
See also next comment.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 89 at r1 (raw file):

AlterLocationPath

Please change to 'ExpectedAlterLocationPath'. Throughout.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 106 at r1 (raw file):

        #endregion

        Describe "MSFT_xSQLServerDatabaseDefaultLocation\Get-TargetResource" -Tag 'Get'{

Single quotes.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 107 at r1 (raw file):

        Describe "MSFT_xSQLServerDatabaseDefaultLocation\Get-TargetResource" -Tag 'Get'{
            Mock -CommandName Connect-SQL -MockWith $mockConnectSQL -Verifiable

Please use BeforeEach or BeforeAll around this one as well, to be consequent.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 109 at r1 (raw file):

            Mock -CommandName Connect-SQL -MockWith $mockConnectSQL -Verifiable

                Context 'When the system is either in the desired state or not in the desired state' {

Please move this back one indent.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 110 at r1 (raw file):

                Context 'When the system is either in the desired state or not in the desired state' {

Remove blank row.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 111 at r1 (raw file):

-testcases

-TestCases

Thorughout.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 118 at r1 (raw file):

$result

Please use $getTargetResourceResult. Throughout.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 118 at r1 (raw file):

                        )

                        $result = Get-TargetResource @mockDefaultParameters -DefaultLocationType $DefaultLocationType -DefaultLocationPath $DefaultLocationPath

Minor: You could use splatting to shorten this row. If so, throughout.
Good as-is if you don't want to change.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 127 at r1 (raw file):

-Scope Context

This should be scoped to 'It' instead. Throughout.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 132 at r1 (raw file):

        }

        Describe "MSFT_xSQLServerDatabaseDefaultLocation\Test-TargetResource" -Tag 'Test'{

Single quotes.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 137 at r1 (raw file):

'When the default path is already set.'

Maybe add another Context-block before this one 'When the system is in the desired state' to align more against the other unit tests (nested context-blocks).

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 143 at r1 (raw file):

$AltLocationPath

Doesn't seem to be used. Does it need to be here for the test cases to work?

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 146 at r1 (raw file):

                    )

                    Test-TargetResource @mockDefaultParameters -DefaultLocationType $DefaultLocationType -DefaultLocationPath $DefaultLocationPath | Should Be $true

Please assign Test-TargetResource to a variable $testTargetResourceResult. It is easier to review then (shorter lines).

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 148 at r1 (raw file):

                    Test-TargetResource @mockDefaultParameters -DefaultLocationType $DefaultLocationType -DefaultLocationPath $DefaultLocationPath | Should Be $true
                }
                It 'Should call the mock function Connect-SQL' {

Blank row before this one.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 152 at r1 (raw file):

'When the default path is different.'

Maybe add another Context-block before this one 'When the system is not in the desired state' to align more against the other unit tests (nested context-blocks).

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 152 at r1 (raw file):

                }
            }
            Context 'When the default path is different.' {

Blank row before this one.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 161 at r1 (raw file):

                    )

                    Test-TargetResource @mockDefaultParameters -DefaultLocationType $DefaultLocationType -DefaultLocationPath $AltLocationPath | Should Be $false

Please assign Test-TargetResource to a variable $testTargetResourceResult. It is easier to review then (shorter lines).

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 163 at r1 (raw file):

                    Test-TargetResource @mockDefaultParameters -DefaultLocationType $DefaultLocationType -DefaultLocationPath $AltLocationPath | Should Be $false
                }
                It 'Should call the mock function Connect-SQL' {

Blank row before this one.Blank row before this one.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 169 at r1 (raw file):

        }

        Describe "MSFT_xSQLServerDatabaseDefaultLocation\Set-TargetResource" -Tag 'Set'{

Single quotes.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 172 at r1 (raw file):

-MockWith {}

Parameter not needed when not mocking any code. Parameter can be removed.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 175 at r1 (raw file):

            }

            Context 'When the desired Default location parameter is not set.' {

Maybe add another Context-block before this one 'When the system is not in the desired state' to align more against the other unit tests (nested context-blocks).

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 192 at r1 (raw file):

                    {Set-TargetResource @mockDefaultParameters @setTargetResourceParameters} | Should Not Throw

Could you add an assert that Alter was called.

See the method used for asserting this in the below code for the method ReservURL():
https://github.com/PowerShell/xSQLServer/blob/fa87101768be91826a244d53ad9302e80f09f4bf/Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1#L73-L77
and
https://github.com/PowerShell/xSQLServer/blob/fa87101768be91826a244d53ad9302e80f09f4bf/Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1#L422-L456

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 201 at r1 (raw file):

'When the desired Default location parameter is not set.'

'When the default location fails to be changed'

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 225 at r1 (raw file):

                }
            }
        }

Please add another test for Set-TargetResource when there is no change is needed.

A Context-block 'When the system is in the desired state'.

---

Comments from Reviewable

[randomnote1]

I'm gonna look at the integration test in order learn it and then help out with it. We're throwing a lot at a first-timer here 😄

---

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

---

Comments from Reviewable

[johlju]

@randomnote1 Yes, I know. I hesitated to mention it. But @PaulFeaser did such awesome job with the unit tests so maybe it was a chance he was up to making a integration tests as well 😄 But...

@PaulFeaser Please ignore the comment about integration tests and we let @randomnote1 fix that after this is merged. 😄

Once we have integration tests for most resources I will start to enforce that for resource that can be integration tested.

---

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

---

Comments from Reviewable

[randomnote1]

@johlju, I'm going to do a PR into @PaulFeaser's branch with the integration tests. That way we'll be fully complete with this resource when the PR is completed.

---

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

---

Comments from Reviewable

[johlju]

Awesome! Even better. 😄

---

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

---

Comments from Reviewable

[PaulFeaser]

Review status: 0 of 10 files reviewed at latest revision, 60 unresolved discussions.

---

CHANGELOG.md, line 112 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

There was an recent release of xSQLServer so you need to rebase this PR and make sure to move these entries back to the unreleased section after rebase.

Please use git rebase (and not git merge or git pull).
If you have not rebased before, here's some help; https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts

Done.

---

README.md, line 511 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This resource is used to configure database default locations for
and Backup for SQL Server.

maybe add 'user database'. Something like:
"This resource is used to configure the default locations for user databases. The types of default locations that can be changed is Data, Log and Backup."

Done.

---

README.md, line 521 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missing RestartService parameter here.

Done.

---

README.md, line 521 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Make sure these parameter descriptions are aligned the same in README.md, schema.mof and comment-based help. See the text differ, so use any text you feel is best.

Done.

---

README.md, line 523 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The name

The type

Done.

---

README.md, line 524 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add the correct format for the values in the validate set. See other resources parameter descriptions as example.

Done.

---

README.md, line 525 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The location of the default...

Maybe: "The path to the default..."

Done.

---

README.md, line 528 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SQLInstance

Should be SQLInstanceName

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 32 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please remove the default value from mandatory parameter. Throughout.

https://ci.appveyor.com/project/PowerShell/xsqlserver/build/6.0.1663.0#L943

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 58 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 63 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 68 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 129 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

System.boolean

System.Boolean (upper 'B').

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 137 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Is this verbose message needed since there was another verbose message two rows up?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 142 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Switch

switch (lower-case 's')

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 167 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$getDefaultLocationPath

This seems to not assigned anywhere. And could the name be changed to maybe $currentValueDefaultLocationPath?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 180 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Why not move this inside the try block and get rid of the $isRestartNeeded variable?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 182 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

RestartSQLServer

Please change the key to 'RestartSqlServer' (using lower-case in 'ql' in 'Sql'). Throughout.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 232 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

System.boolean

System.Boolean (upper 'B').

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 233 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missing this in the comment-based help.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 238 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$parameters

Please change to $getTargetResourceParameters

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 245 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$currentValues

Please change to $getTargetResourceResult.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 247 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$isMDefaultLocationInDesiredState

Why is it a single 'M' here?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 249 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$getDefaultLocationPath

Why not use $currentValues.DefaultLocationPath here directly? _After the above change $getTargetResourceResult.DefaultLocationPath

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.schema.mof, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This needs to be a key as well, otherwise it's not possible to change this on two instances on the same host.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.schema.mof, line 6 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

DefaultLocationType

Wouldn't it be enough with just Type? The resource is called DefaultLocation...

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.schema.mof, line 7 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

DefaultLocationPath

Wouldn't it be enough with just Path? The resource is called DefaultLocation...

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Default File Location

Change to lower-case letters.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

for SQL Instance

"for instance" should be enough?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 5 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Info

Please write out the word to 'Information' (if that is what it should say). Rather not use abbreviations.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 6 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Verifying the default '{0}' file path is changing.

Rephrase. Do you mean verify if it needs to be changed?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 8 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

default value
default file location
default location

Maybe use one of these in all strings to be consequent? Throughout in all of these strings.

Maybe use "path" instead of "location" depending on the terminology used in the parameter descriptions?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 12 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The alter command for setting the default location failed

Minor: Think we should skip the reference to the 'alter' word and instead just say that changing the path failed?

Good as-is if you want to keep it.

Done.

---

Examples/Resources/xSQLServerDatabaseDefaultLocation/1-SetDatabaseDefaultLocation.ps1, line 10 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[PSCredential]

[System.Management.Automation.PSCredential]

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 44 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"C:\temp"

Single quotes. Also on the two lines below.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 88 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

AltLocationPath

Please change to 'AlterLocationPath'. Throughout.
See also next comment.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 89 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

AlterLocationPath

Please change to 'ExpectedAlterLocationPath'. Throughout.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 106 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Single quotes.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 107 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use BeforeEach or BeforeAll around this one as well, to be consequent.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 109 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please move this back one indent.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 110 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Remove blank row.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 111 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-testcases

-TestCases

Thorughout.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 118 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$result

Please use $getTargetResourceResult. Throughout.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 118 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Minor: You could use splatting to shorten this row. If so, throughout.
Good as-is if you don't want to change.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 127 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Scope Context

This should be scoped to 'It' instead. Throughout.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 132 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Single quotes.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 137 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'When the default path is already set.'

Maybe add another Context-block before this one 'When the system is in the desired state' to align more against the other unit tests (nested context-blocks).

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 143 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$AltLocationPath

Doesn't seem to be used. Does it need to be here for the test cases to work?

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 146 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please assign Test-TargetResource to a variable $testTargetResourceResult. It is easier to review then (shorter lines).

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 148 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank row before this one.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 152 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'When the default path is different.'

Maybe add another Context-block before this one 'When the system is not in the desired state' to align more against the other unit tests (nested context-blocks).

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 152 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank row before this one.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 161 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please assign Test-TargetResource to a variable $testTargetResourceResult. It is easier to review then (shorter lines).

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 163 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank row before this one.Blank row before this one.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 169 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Single quotes.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 172 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-MockWith {}

Parameter not needed when not mocking any code. Parameter can be removed.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 175 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe add another Context-block before this one 'When the system is not in the desired state' to align more against the other unit tests (nested context-blocks).

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 192 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add an assert that Alter was called.

See the method used for asserting this in the below code for the method ReservURL():
https://github.com/PowerShell/xSQLServer/blob/fa87101768be91826a244d53ad9302e80f09f4bf/Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1#L73-L77
and
https://github.com/PowerShell/xSQLServer/blob/fa87101768be91826a244d53ad9302e80f09f4bf/Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1#L422-L456

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 201 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'When the desired Default location parameter is not set.'

'When the default location fails to be changed'

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 225 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add another test for Set-TargetResource when there is no change is needed.

A Context-block 'When the system is in the desired state'.

wasn't sure about needing this since it is tested in the Set-TargeResource

---

Comments from Reviewable

[PaulFeaser]

@johlju, I've addressed the reviewed issues. I did end up changing the majority of the variable names as you suggested. I also added code to make sure that the directory exists. Because of that, I added code from @randomnote1 to make it cluster aware.

[johlju]

Awesome

---

Reviewed 1 of 8 files at r3, 7 of 9 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 12 at r5 (raw file):

<#
    .SYNOPSIS
    Returns the current path to the the desired default location for the Data, Log, or Backup files.

Can we indent the description one step more? See guideline here
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#functions-have-comment-based-help

Throughout.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 24 at r5 (raw file):

    .PARAMETER Path
    The path to the default directory to be configured.

We should add 'Not used in Get-TargetResource,' in the parameter description as per guideline Any unused parameters that must be included in a function definition should include 'Not used in <function_name>' in the help comment for that parameter in the comment-based help

See next comment as well.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 66 at r5 (raw file):

$Path

Should we change this to $currentPath so we don't mistaken it for the $Path variable which is not used in the Get-TargetResource? What do you think?

If so, throughout the function Get-TargetResource.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 143 at r5 (raw file):

[Boolean]

Please change to [System.Boolean] to be consequent.

@randomnote1's code does not show this because the code is from one of the resource that we not yet have made this change in. :)

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 148 at r5 (raw file):

if(

We should add a space between if an the open parenthesis if (

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 150 at r5 (raw file):

    if(-Not (Test-Path $Path))
    {
        throw ($script:localizedData.InvalidPath -f $Path )

Minor (non-blocking): Maybe we should remove space before the closing parenthesis to be consequent?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 259 at r5 (raw file):

[Boolean]

Please change to [System.Boolean] to be consequent.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 282 at r5 (raw file):

    {
        Write-Verbose -Message ($script:localizedData.NotActiveClusterNode -f $env:COMPUTERNAME,$SQLInstanceName )
        return $isDefaultPathInDesiredState

If we would add an elseif on the the next if-statement then we wouldn't need to return here, and we could remove one row of code? Good as-is too. Non-blocking.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.schema.mof, line 4 at r1 (raw file):

Previously, PaulFeaser (Paul Feaser) wrote…

Done.

The parameter SQLInstanceName was not changed to Key. I think it need to be key to be able to change two instances on the same server (same SQLServer).

If you want, you could regression test this by adding two instances to the example. The example should fail in the "common Pester tests", when run in AppVeyor (or running the DscResource.Tests/Meta.Tests.ps1 locally), unless SQLInstanceName is key.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 4 at r1 (raw file):

Previously, PaulFeaser (Paul Feaser) wrote…

Done.

Could we make it 'instance' - lower-case 'i'?

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 12 at r5 (raw file):

    ChangingPathFailed = Changing the default path failed.
    InvalidPath = The path '{0}' does not exist.
    NotActiveClusterNode = 'The node "{0}" is not actively hosting the instance "{1}". Exiting the test.'

The string should not be surrounded by single-quotes. I make this mistake often, when copy/pasting 😄

Minor: To be consequent maybe we should use single-quotes in this string like the other strings?

---

Examples/Resources/xSQLServerDatabaseDefaultLocation/1-SetDatabaseDefaultLocation.ps1, line 22 at r5 (raw file):

            SQLServer                     = 'SQLServer'
            SQLInstanceName               = 'DSC'
            ProcessOnlyOnActiveNode       = $true

Could you add a description to why this parameter is there? And maybe we should add it to 'Log' and 'Backup' as well?

See example https://github.com/PowerShell/xSQLServer/blob/dev/Examples/Resources/xSQLServerAlwaysOnAvailabilityGroup/3-CreateAvailabilityGroupDetailed.ps1

---

Tests/Integration/MSFT_xSQLServerDatabaseDefaultLocation.config.ps1, line 47 at r5 (raw file):

        xSQLServerDatabaseDefaultLocation 'Integration_Test'
        {
            DefaultLocationType  = 'Data'

Can we change these to the new parameter names so that the integration tests don't fail. 😄

Throughout the configurations in this file.

---

Tests/Integration/MSFT_xSQLServerDatabaseDefaultLocation.Integration.Tests.ps1, line 66 at r5 (raw file):

                    Start-DscConfiguration @startDscConfigurationParameters
                } | Should Not Throw

Since we merged a PR changing all pester tests to the new v4 syntax we should change these as well, i.e | Should -Not -Throw, | Should -Be, and so on.

See issue #863 for a script to do it. Or change manually throughout.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 128 at r5 (raw file):

                    $getTargetResourceResult = Get-TargetResource @mockDefaultParameters -Type $Type -Path $Path

                    $getTargetResourceResult.Path | Should Be $Path

Since we merged a PR changing all pester tests to the new v4 syntax we should change these as well, i.e | Should -Not -Throw, | Should -Be, and so on.

See issue #863 for a script to do it. Or change manually throughout.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 193 at r5 (raw file):

                        ProcessOnlyOnActiveNode = $mockProcessOnlyOnActiveNode
                    }
                    Test-TargetResource @testTargetResourceParameters | Should Be $true

Could we please add a blank row before this row?

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 206 at r5 (raw file):

                        ProcessOnlyOnActiveNode = $mockProcessOnlyOnActiveNodeOriginal
                    }
                    Test-TargetResource @testTargetResourceParameters | Should Be $true

Could we please add a blank row before this row?

---

Comments from Reviewable

[johlju]

@PaulFeaser Really awesome work on this one! Just a few more comments.

[PaulFeaser]

All the changes have been made.

---

Review status: 2 of 10 files reviewed at latest revision, 18 unresolved discussions.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 12 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we indent the description one step more? See guideline here
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#functions-have-comment-based-help

Throughout.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 24 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should add 'Not used in Get-TargetResource,' in the parameter description as per guideline Any unused parameters that must be included in a function definition should include 'Not used in <function_name>' in the help comment for that parameter in the comment-based help

See next comment as well.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 66 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Path

Should we change this to $currentPath so we don't mistaken it for the $Path variable which is not used in the Get-TargetResource? What do you think?

If so, throughout the function Get-TargetResource.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 143 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[Boolean]

Please change to [System.Boolean] to be consequent.

@randomnote1's code does not show this because the code is from one of the resource that we not yet have made this change in. :)

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 148 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if(

We should add a space between if an the open parenthesis if (

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 150 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Minor (non-blocking): Maybe we should remove space before the closing parenthesis to be consequent?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 259 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[Boolean]

Please change to [System.Boolean] to be consequent.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.psm1, line 282 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If we would add an elseif on the the next if-statement then we wouldn't need to return here, and we could remove one row of code? Good as-is too. Non-blocking.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/MSFT_xSQLServerDatabaseDefaultLocation.schema.mof, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The parameter SQLInstanceName was not changed to Key. I think it need to be key to be able to change two instances on the same server (same SQLServer).

If you want, you could regression test this by adding two instances to the example. The example should fail in the "common Pester tests", when run in AppVeyor (or running the DscResource.Tests/Meta.Tests.ps1 locally), unless SQLInstanceName is key.

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we make it 'instance' - lower-case 'i'?

Done.

---

DSCResources/MSFT_xSQLServerDatabaseDefaultLocation/en-US/MSFT_xSQLServerDatabaseDefaultLocation.strings.psd1, line 12 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The string should not be surrounded by single-quotes. I make this mistake often, when copy/pasting 😄

Minor: To be consequent maybe we should use single-quotes in this string like the other strings?

Done.

---

Examples/Resources/xSQLServerDatabaseDefaultLocation/1-SetDatabaseDefaultLocation.ps1, line 22 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a description to why this parameter is there? And maybe we should add it to 'Log' and 'Backup' as well?

See example https://github.com/PowerShell/xSQLServer/blob/dev/Examples/Resources/xSQLServerAlwaysOnAvailabilityGroup/3-CreateAvailabilityGroupDetailed.ps1

Done.

---

Tests/Integration/MSFT_xSQLServerDatabaseDefaultLocation.config.ps1, line 47 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we change these to the new parameter names so that the integration tests don't fail. 😄

Throughout the configurations in this file.

Done.

---

Tests/Integration/MSFT_xSQLServerDatabaseDefaultLocation.Integration.Tests.ps1, line 66 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Since we merged a PR changing all pester tests to the new v4 syntax we should change these as well, i.e | Should -Not -Throw, | Should -Be, and so on.

See issue #863 for a script to do it. Or change manually throughout.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 128 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Since we merged a PR changing all pester tests to the new v4 syntax we should change these as well, i.e | Should -Not -Throw, | Should -Be, and so on.

See issue #863 for a script to do it. Or change manually throughout.

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 193 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we please add a blank row before this row?

Done.

---

Tests/Unit/MSFT_xSQLServerDatabaseDefaultLocation.Tests.ps1, line 206 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we please add a blank row before this row?

Done.

---

Comments from Reviewable

[johlju]

---

Reviewed 8 of 8 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[johlju]

I will merge this as soon as the tests passes. Again, awesome work @PaulFeaser!

[PaulFeaser]

Thanks for all the help you provided. I appreciate your patience. Also thanks to @randomnote1 for guiding me through each step.

---

Review status: 9 of 10 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[johlju]

Thank you for contributing! 😄

---

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

---

Comments from Reviewable