Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xSQLServerScript: Changes to the resource and tests #280

Merged
merged 8 commits into from
Jan 5, 2017

Conversation

johlju
Copy link
Member

@johlju johlju commented Dec 31, 2016

  • Changes to xSQLServerScript
    • All of the $null is now on the left side of equality comparisons.
    • All credential parameters now also has the type [System.Management.Automation.Credential()] to better work with PowerShell 4.0.
    • It is now possible to configure two instances on the same node, with the same script.
    • Added to the description text for the parameter 'Credential' describing how to authenticate using Windows Authentication.
    • Added examples to show how to authenticate using either SQL or Windows authentication.
    • Made changes according to the remaining review comments in PR Renamed MSFT_xSQLServerScript.Test to MSFT_xSQLServerScript.Tests #156.

This Pull Request (PR) fixes the following issues:
Fixes #266
Fixes #272
Fixes #273
Fixes #274

  • 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 Reviewable

@johlju johlju added the needs review The pull request needs a code review. label Dec 31, 2016
@johlju johlju force-pushed the changes-to-xSQLServerScript branch from 34b915d to 2ca5ca7 Compare January 1, 2017 11:43
johlju added 5 commits January 3, 2017 13:21
All of the $null is now on the left side of equality comparisons.
All credential parameters now also has the type [System.Management.Automation.Credential()] to work with PowerShell 4.0.
It is now possible to configure two instances on the same node, with the same script.
Added to the description text for the parameter 'Credential' describing how to authenticate using Windows Authentication.
Added examples to show how to authenticate using either SQL or Windows authentication.
Made changes according to the remaining review comments in PR dsccommunity#156
Added a note in README.md for xSQLServerScript that there is a known problem running the resource using PowerShell 4.0.
@johlju johlju force-pushed the changes-to-xSQLServerScript branch from 2ca5ca7 to 27751ba Compare January 3, 2017 12:22
@johlju
Copy link
Member Author

johlju commented Jan 3, 2017

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


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

  - Resolved all of the PSScriptAnalyzer warnings that was triggered in the common tests.
- Changes to xSQLServerScript
  - All credential parameters now also has the type [System.Management.Automation.Credential()] to better work with PowerShell 4.0.

Add here that there are a known problem running this in PowerShell 5.0 and README.md has been updated to address that.


DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 83 at r1 (raw file):

    .PARAMETER ServerInstance
        The name of an instance of the Database Engine. For default instances, only specify the computer name. For named instances, use the format ComputerName\InstanceName.
    

Blank spaces here


DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 138 at r1 (raw file):

    .PARAMETER ServerInstance
        The name of an instance of the Database Engine. For default instances, only specify the computer name. For named instances, use the format ComputerName\InstanceName.
    

Blank spaces here


DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 211 at r1 (raw file):

    .PARAMETER ServerInstance
        The name of an instance of the Database Engine. For default instances, only specify the computer name. For named instances, use the format ComputerName\InstanceName.
    

Blank spaces here


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

For default instances

For the default instance


DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.schema.mof, line 5 at r1 (raw file):

{
    [Key, Description("The name of an instance of the Database Engine. For default instances, only specify the computer name. For named instances, use the format ComputerName\\InstanceName")] String ServerInstance;
    [Key, Description("Path to SQL file that will perform Set action.")] String SetFilePath;

Please align these with README.md and comment-based help


Examples/Resources/xSQLServerScript/1-RunScriptUsingSQLAuthentication.ps1, line 13 at r1 (raw file):

        $SqlCredential
    )
    

Blank spaces


Examples/Resources/xSQLServerScript/2-RunScriptUsingWindowsAuthentication.ps1, line 14 at r1 (raw file):

        $WindowsCredential
    )
    

Blank spaces


Tests/Unit/MSFT_xSQLServerScript.Tests.ps1, line 138 at r1 (raw file):

            }
        }
        

Blank spaces


Tests/Unit/MSFT_xSQLServerScript.Tests.ps1, line 184 at r1 (raw file):

                It 'Should throw the correct error from Invoke-Sqlcmd' {
                    { Test-TargetResource @testParameters } | Should Throw $errorMessage
                }            

Blank spaces


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jan 3, 2017

Reviewed 6 of 7 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 11 unresolved discussions.


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

Previously, johlju (Johan Ljunggren) wrote…

Add here that there are a known problem running this in PowerShell 5.0 and README.md has been updated to address that.

Should have been 'PowerShell 4.0' in previous comment.

Done.


DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 83 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank spaces here

Done.


DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 138 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank spaces here

Done.


DSCResources/MSFT_xSQLServerScript/MSFT_xSQLServerScript.psm1, line 211 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank spaces here

Done.


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

    .PARAMETER Credential
        The credentials to use to authenticate using SQL Authentication. To authenticate using Windows Authentication, assing the credentials

Typo here 'assign'. Same in schema and README, and comment-based help.


Examples/Resources/xSQLServerScript/1-RunScriptUsingSQLAuthentication.ps1, line 13 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank spaces

Done.


Tests/Unit/MSFT_xSQLServerScript.Tests.ps1, line 138 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank spaces

Done.


Tests/Unit/MSFT_xSQLServerScript.Tests.ps1, line 184 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank spaces

Done.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jan 3, 2017

:LGTM:

If there are no more comments on this one in a day or so, I will merge it. Feel free to review this one.


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


Comments from Reviewable

@johlju johlju removed the needs review The pull request needs a code review. label Jan 4, 2017
@johlju johlju merged commit 7cf0a7f into dsccommunity:dev Jan 5, 2017
@johlju johlju deleted the changes-to-xSQLServerScript branch January 5, 2017 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment