Added xSQLServerScript Resource

[dcrreynolds]

xSQLServerScript resource to extend DSCs Get/Set/Test functionality to T-SQL

---

This change is

[msftclas]

Hi @dcrreynolds, 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 (David Reynolds). 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;

[dcrreynolds]

The automated test are not fans of using UserName and Password, however, it directly matches how Invoke-SqlCmd works. I opted to leave it matching Invoke-SqlCmd to stay consistent with the cmdlet, but am open to changing it too.

[dcrreynolds]

Any opinions of this? Do we follow the PowerShell standard and use a PSCred object, or use the Invoke-SqlCmd language and update the test to not fail here.

[aultt]

I believe everything else in xSqlServer is utilizing PSCred Object so for consistency I would go with PSCred

[mbreakey3]

Reviewed 1 of 5 files at r2, 2 of 3 files at r3.
Review status: 3 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

README.md, line 278 [r3] (raw file):

* **SetFilePath**: (Key) Path to SQL file that will perform Set action.
* **GetFilePath**: (Key) Path to SQL file that will perform Get action. SQL Queries returned by this function are returned by the Get-DscConfiguration cmdlet with the GetResult parameter.
* **TestFilePath**: (Key) Path to SQL file that will perform Test action. Any Script that does not throw an error and returns null is evaluated to true. Invoke-SqlCmd treats SQL Print statements as verbose text, this will not cause a the Test to return false. 

Typo here - "this will not cause a the Test to return false."

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 8 [r3] (raw file):

# TODO: Customize these parameters...
$Global:DSCModuleName      = 'MSFT_xSQLServerScript' # Example xNetworking

Take out comments from template - also the tests templates recently got updated to change these variables from global to script, so can you change both of these to be $script: variables?

---

Comments from Reviewable

[kilasuit]

With the latest updates to SSMS the SQLPS module was renamed to SQLServer so baring this in mind should we not have a check in here for the change of module name for this along the lines of

If (Get-Module SQLPS -ListAvailable) 
    {
        Write-Verbose "SQLPS Module Found"
        $module = Get-Module SQLPS -ListAvailable
    } 
    else 
    {
    Write-Verbose "SQLPS Module Not found - Using updated SQLServer module "
    $module = Get-Module SQLServer -ListAvailable
    }

Import-Module $module -WarningAction SilentlyContinue -ErrorAction Stop

Though this would need to be actioned in the same way across all of the resources where this is currently in place

Referenced this in #91 as well

[dcrreynolds]

Thanks for the feedback! I'll get this fixed up.

[kilasuit]

@dcrreynolds - My thoughts would be to deal with the last comment as per #91 (with a better suggestion in there too) in a separate PR just thought it needed to be called out as being something that needed to be thought of thats all

[dcrreynolds]

@mbreakey3 I've added your changes in, however, Reviewable keeps crashing and will not let me mark the comments as done. @kilasuit I would agree with updating the resource as one PR for #91 it will help ensure the fix is done uniformly.

[mbreakey3]

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

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 29 [r4] (raw file):

try
{
    InModuleScope $DSCResourceName {

We're also not using the InModuleScope anymore for testing exported functions due to the fact that it can lead to conflicts.

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 89 [r4] (raw file):

            It "Should return true if Invoke-SqlCmd returns null" {
                Mock -CommandName Import-Module -MockWith {}

Since you're mocking these same commands for almost all of these It statements you can create a Context block (which is almost the same as a Describe block) and put all of the It statements that use that mocked function within that Context block and just declare the mock once at the top of that Context block

---

Comments from Reviewable

[dcrreynolds]

Just started over with the test using the current unit test template.

[johlju]

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

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 30 [r3] (raw file):

Previously, kilasuit (Ryan Yates) wrote…

With the latest updates to SSMS the SQLPS module was renamed to SQLServer so baring this in mind should we not have a check in here for the change of module name for this along the lines of

If (Get-Module SQLPS -ListAvailable) 

    {

        Write-Verbose "SQLPS Module Found"

        $module = Get-Module SQLPS -ListAvailable

    } 

    else 

    {

    Write-Verbose "SQLPS Module Not found - Using updated SQLServer module "

    $module = Get-Module SQLServer -ListAvailable

    }



Import-Module $module -WarningAction SilentlyContinue -ErrorAction Stop

Though this would need to be actioned in the same way across all of the resources where this is currently in place

Referenced this in #91 as well

It could be changed to use the helper function `Import-SQLPSModule` (in the xSQLServerHelper module). Then we only have to change the logic of which module is loaded (the #91 issue) in one place. Is this doable?

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 7 [r5] (raw file):

    param
    (
        [parameter(Mandatory = $true)]

Can you change this to [Parameter (using capital P). If you can do this here and in all other places in the resource code?

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 49 [r5] (raw file):

    $returnValue = @{
        ServerInstance = [System.String]$ServerInstance

Can you change this to [System.String] $ServerInstance (white space between type and variable). The same for the rest too below.

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 167 [r5] (raw file):

    {
        Write-Verbose $_
        return $false

Everytime an error occurs in this try-block you will send $false back to LCM so it will run the Set-method. Is this your intended behaviour? You should not throw and error instead so an error is reported back to the user? Ie. throw New-TerminatingError

---

Comments from Reviewable

[johlju]

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

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 167 [r5] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Everytime an error occurs in this try-block you will send $false back to LCM so it will run the Set-method. Is this your intended behaviour? You should not throw and error instead so an error is reported back to the user? Ie. throw New-TerminatingError

With 'everytime' I meant that it will return false on all errors, even connection problems.

---

Comments from Reviewable

[johlju]

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

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 108 [r5] (raw file):

function Test-TargetResource

A suggestion or and idea. Might not work for your intended behavior.

Seems like most of the code is the same as in the Get-method. Would it work to call the Get-method from the Test-method instead? Looks like you could save a few lines of code.
Also the Get-method isn't called from LCM (currently) so right now that method don't do anything (somebody correct me if I'm wrong). Which I think means that if you leverage the Get-method from Test method, you might be able to remove the GetFilePath parameter since it seems they do the same (that is "get current status" and Get returns current status, and Test returns true or false).

---

Comments from Reviewable

[mbreakey3]

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 29 [r4] (raw file):

Previously, mbreakey3 (Mariah) wrote…

We're also not using the InModuleScope anymore for testing exported functions due to the fact that it can lead to conflicts.

LGTM

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 89 [r4] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Since you're mocking these same commands for almost all of these It statements you can create a Context block (which is almost the same as a Describe block) and put all of the It statements that use that mocked function within that Context block and just declare the mock once at the top of that Context block

LGTM

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[dcrreynolds]

Review status: 2 of 5 files reviewed at latest revision, 9 unresolved discussions.

---

README.md, line 278 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Typo here - "this will not cause a the Test to return false."

Done.

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 30 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It could be changed to use the helper function Import-SQLPSModule (in the xSQLServerHelper module). Then we only have to change the logic of which module is loaded (the #91 issue) in one place. Is this doable?

Done.

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 7 [r5] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you change this to [Parameter (using capital P). If you can do this here and in all other places in the resource code?

Done.

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 49 [r5] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you change this to [System.String] $ServerInstance (white space between type and variable). The same for the rest too below.

Done.

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 108 [r5] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

A suggestion or and idea. Might not work for your intended behavior.

Seems like most of the code is the same as in the Get-method. Would it work to call the Get-method from the Test-method instead? Looks like you could save a few lines of code.
Also the Get-method isn't called from LCM (currently) so right now that method don't do anything (somebody correct me if I'm wrong). Which I think means that if you leverage the Get-method from Test method, you might be able to remove the GetFilePath parameter since it seems they do the same (that is "get current status" and Get returns current status, and Test returns true or false).

Good point about Test and Get being similar. I broke it into a separate function that would also supports Set. GetFilePath is used to extend the regular use of Get-DscConfiguration to a SQL script.

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 167 [r5] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

With 'everytime' I meant that it will return false on all errors, even connection problems.

A connection problem is an interesting case, but what you describe is the intended behavior. The SQL script at $TestFilePath will be run, if an exception is thrown or output generated then it is treated as a fail. For example, the Test SQL script may check for the existence of an account (or any config for that matter) and use raiserror() if it is not found. Then Set would run a Set SQL script to create the account. Get-DscConfiguration would run the SQL script at $GetFilePath and return any information about the account returned by the get SQL script.

This has been really helpful environments that have large SQL scripts that they have been using, but now want to add them to their DSC deployment.

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 8 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Take out comments from template - also the tests templates recently got updated to change these variables from global to script, so can you change both of these to be $script: variables?

Done.

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 29 [r4] (raw file):

Previously, mbreakey3 (Mariah) wrote…

LGTM

Done.

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 89 [r4] (raw file):

Previously, mbreakey3 (Mariah) wrote…

LGTM

Done.

---

Comments from Reviewable

[mbreakey3]

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

---

README.md, line 334 [r6] (raw file):

### Unreleased
* Added resources
  - xSQLServerScript

Script or replication? It should be the same across the board

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 2 [r6] (raw file):

<#
.Synopsis

.SYNOPSIS should be in all caps - also can you indent the contents of the block comment?

---

Comments from Reviewable

[johlju]

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

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 30 [r3] (raw file):

Previously, dcrreynolds (David Reynolds) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 7 [r5] (raw file):

Previously, dcrreynolds (David Reynolds) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 49 [r5] (raw file):

Previously, dcrreynolds (David Reynolds) wrote…

Done.

LGTM

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 108 [r5] (raw file):

Previously, dcrreynolds (David Reynolds) wrote…

Good point about Test and Get being similar. I broke it into a separate function that would also supports Set. GetFilePath is used to extend the regular use of Get-DscConfiguration to a SQL script.

LGTM

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 167 [r5] (raw file):

Previously, dcrreynolds (David Reynolds) wrote…

A connection problem is an interesting case, but what you describe is the intended behavior. The SQL script at $TestFilePath will be run, if an exception is thrown or output generated then it is treated as a fail. For example, the Test SQL script may check for the existence of an account (or any config for that matter) and use raiserror() if it is not found. Then Set would run a Set SQL script to create the account. Get-DscConfiguration would run the SQL script at $GetFilePath and return any information about the account returned by the get SQL script.

This has been really helpful environments that have large SQL scripts that they have been using, but now want to add them to their DSC deployment.

Invoke-Sqlcmd returns `SqlPowerShellSqlExecutionException` type using `$_.Exception.GetType().Name` when using both `RAISEERROR()` and `THROW`. Better approach would be using that as the type for the catch-statement, if possible.

Could you check for that exception, and return $false only when that exception is thrown by Invoke-SqlCmd, and re-throw any other exception to the user? Would that improve error handling and still making the resource work as you intended?

---

Comments from Reviewable

[dcrreynolds]

Review status: 2 of 5 files reviewed at latest revision, 4 unresolved discussions.

---

README.md, line 334 [r6] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Script or replication? It should be the same across the board

Script. It got missed in the merge. Should be correct now.

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 167 [r5] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Invoke-Sqlcmd returns SqlPowerShellSqlExecutionException type using $_.Exception.GetType().Name when using both RAISEERROR() and THROW. Better approach would be using that as the type for the catch-statement, if possible.

Could you check for that exception, and return $false only when that exception is thrown by Invoke-SqlCmd, and re-throw any other exception to the user? Would that improve error handling and still making the resource work as you intended?

Awesome suggestion! Done.

---

Tests/Unit/MSFT_xSQLServerScript.Test.ps1, line 2 [r6] (raw file):

Previously, mbreakey3 (Mariah) wrote…

.SYNOPSIS should be in all caps - also can you indent the contents of the block comment?

Done.

---

Comments from Reviewable

[johlju]

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

---

DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 167 [r5] (raw file):

Previously, dcrreynolds (David Reynolds) wrote…

Awesome suggestion! Done.

LGTM

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 3 files at r6, 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable