SqlScript, SqlScriptQuery: Add support for disable variables parameter #1578

Kreby · 2020-06-30T04:40:50Z

Pull Request (PR) description

SqlScript
- Added the DisabledVariables parameter
SqlScriptQuery
- Added the DisabledVariables parameter

This PR adds support for the DisableVariables parameter from the Invoke-SqlCmd cmdlet. It should hopefully be relatively straight forward. The only thing I'd point out is in the integration tests used. I was able to keep the same SQL queries as the other tests. Just note that when the test is run with the DisableVariables parameter it will create the database and give it the name of the variable used

$(DatabaseName)

Despite being a bit odd looking it is a valid database name and when matched, just like the other tests, it will pass. It does also require, as mentioned in a comment, that the database is removed after the test runs to ensure the subsequent test succeeds as well. This is because the same variable name is used in both resource configuration data variables. I think it should suffice but please let me know if it doesn't.

This Pull Request (PR) fixes the following issues

Fixes SqlScript: DisableVariables from Invoke-SQLCmd not available #1422

Task list

Added an entry to the change log under the Unreleased section of the
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Community Testing Guidelines.
Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
New/changed code adheres to DSC Community Style Guidelines.

This change is

codecov · 2020-06-30T07:44:52Z

Codecov Report

Merging #1578 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1578   +/-   ##
======================================
  Coverage      98%     98%           
======================================
  Files          37      37           
  Lines        6074    6082    +8     
======================================
+ Hits         5970    5978    +8     
  Misses        104     104

johlju · 2020-06-30T17:13:24Z

@Kreby Thank you for this! 😃 I might not have time to review this until the weekend due other obligations, but will get onto this as soon as I can!

johlju

Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Kreby)

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

- Added the DisableVariables parameter ([issue #1422](https://github.com/dsccommunity/SqlServerDsc/issues/1422))

Can we add a full stop (.) at the en of the sentence to align with the other entries.

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

- Added the DisableVariables parameter ([issue #1422](https://github.com/dsccommunity/SqlServerDsc/issues/1422))

Can we add a full stop (.) at the en of the sentence to align with the other entries.

tests/Integration/DSC_SqlScript.config.ps1, line 32 at r1 (raw file):

Database3Name     = '$(DatabaseName)'

This is not used, right? We can remove it?

tests/Integration/DSC_SqlScript.config.ps1, line 261 at r1 (raw file):

Quoted 4 lines of code…

<#
    .SYNOPSIS
        Runs the SQL script with variables disabled.
#>

Can we add a .NOTES section with some of the text you wrote in the PR description about that it will create a database with the name $(DatabaseName) so next contributor more easily understand the logic. 🙂

tests/Integration/DSC_SqlScriptQuery.config.ps1, line 30 at r1 (raw file):

Database3Name   = '$(DatabaseName)'

This is not used, right? We can remove it?

tests/Integration/DSC_SqlScriptQuery.config.ps1, line 119 at r1 (raw file):

Quoted 4 lines of code…

<#
    .SYNOPSIS
        Runs the SQL query with variables disabled.
#>

Same comment about a .NOTES section as above.

johlju · 2020-07-02T11:46:37Z

Great work on this! Just minor changes then this is good to go. 😃

johlju

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Kreby)

tests/Integration/DSC_SqlScript.config.ps1, line 32 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Database3Name     = '$(DatabaseName)'
This is not used, right? We can remove it?

It was used in the integration tests, I missed that. Removed the comment. 🙂

tests/Integration/DSC_SqlScriptQuery.config.ps1, line 30 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Database3Name   = '$(DatabaseName)'
This is not used, right? We can remove it?

It was used in the integration tests, I missed that. Removed the comment. 🙂

Kreby · 2020-07-02T16:02:59Z

@johlju thanks for the kind words/support. I'm glad to help out. I've not used reviewable before so I was a little disoriented on my first look 😵. I wasn't quite sure how to respond appropriately to the individual comments with the few minutes I had.

I'll spend sometime later today or tomorrow to figure that out, make the requested changes, and update my branch. Thanks!

johlju · 2020-07-02T16:29:07Z

Yes, Reviewable can take a while to get used to (it did for me) but is awesome once you are. 🙂 Basically you just write Done as a comment when you have sent in a change for a comment, or write a comment when you want to discuss something. Once you answered the comments you like you hit the big green publish button at the top, that will publish all comments back to the PR as one big PR comment.

When you write Done for a comment then I can "Resolve" them in Reviewable.

…into add-disable-variables-for-sqlscript-sqlscriptquery

Kreby

Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @johlju)

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

Previously, johlju (Johan Ljunggren) wrote…

- Added the DisableVariables parameter ([issue #1422](https://github.com/dsccommunity/SqlServerDsc/issues/1422))
Can we add a full stop (.) at the en of the sentence to align with the other entries.

Done.

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

Previously, johlju (Johan Ljunggren) wrote…

- Added the DisableVariables parameter ([issue #1422](https://github.com/dsccommunity/SqlServerDsc/issues/1422))
Can we add a full stop (.) at the en of the sentence to align with the other entries.

Done.

tests/Integration/DSC_SqlScript.config.ps1, line 261 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

<#
    .SYNOPSIS
        Runs the SQL script with variables disabled.
#>
Can we add a .NOTES section with some of the text you wrote in the PR description about that it will create a database with the name $(DatabaseName) so next contributor more easily understand the logic. 🙂

I've updated the comment based help with details.

tests/Integration/DSC_SqlScriptQuery.config.ps1, line 119 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

<#
    .SYNOPSIS
        Runs the SQL query with variables disabled.
#>
Same comment about a .NOTES section as above.

I've updated the comment based help with details.

johlju

Reviewed 3 of 3 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

johlju · 2020-07-04T11:17:38Z

@Kreby thank you for this! I merged it and will do a full release a bit later today. 🙂

Kreby added 10 commits June 29, 2020 20:10

Add DisableVariables param to Invoke-SqlScript

eca7cf3

Add DisableVariables to SqlScript resource

27b7904

Add DisableVariables to SqlScriptQuery resource

16636e3

Add SqlScript integration tests

a9f61db

Add SqlScriptQuery integration tests

a472bd1

Added examples for using DisabledVariables

5ad2f70

Updated changelog

574c526

Fixed incorrect parameter name

4681a0f

Fixed a typo in the changelog

db2a95c

Remove extra characters from parameter help

9fe0785

Kreby marked this pull request as ready for review June 30, 2020 12:43

johlju added the needs review The pull request needs a code review. label Jun 30, 2020

johlju requested changes Jul 2, 2020

View reviewed changes

johlju reviewed Jul 2, 2020

View reviewed changes

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 2, 2020

Kreby added 4 commits July 3, 2020 13:23

Merge branch 'master' of https://github.com/dsccommunity/SqlServerDsc …

270dfeb

…into add-disable-variables-for-sqlscript-sqlscriptquery

Added correct punctuation.

06ae098

Fixed up comment based help

c9608df

Merge branch 'master' of https://github.com/dsccommunity/SqlServerDsc …

9fde376

…into add-disable-variables-for-sqlscript-sqlscriptquery

Kreby commented Jul 3, 2020

View reviewed changes

johlju approved these changes Jul 4, 2020

View reviewed changes

johlju merged commit 41fab4a into dsccommunity:master Jul 4, 2020

johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Aug 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SqlScript, SqlScriptQuery: Add support for disable variables parameter #1578

SqlScript, SqlScriptQuery: Add support for disable variables parameter #1578

Kreby commented Jun 30, 2020 •

edited by johlju

Loading

codecov bot commented Jun 30, 2020

johlju commented Jun 30, 2020 •

edited

Loading

johlju left a comment

johlju commented Jul 2, 2020

johlju left a comment

Kreby commented Jul 2, 2020

johlju commented Jul 2, 2020 •

edited

Loading

Kreby left a comment

johlju left a comment

johlju commented Jul 4, 2020

SqlScript, SqlScriptQuery: Add support for disable variables parameter #1578

SqlScript, SqlScriptQuery: Add support for disable variables parameter #1578

Conversation

Kreby commented Jun 30, 2020 • edited by johlju Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov bot commented Jun 30, 2020

Codecov Report

johlju commented Jun 30, 2020 • edited Loading

johlju left a comment

Choose a reason for hiding this comment

johlju commented Jul 2, 2020

johlju left a comment

Choose a reason for hiding this comment

Kreby commented Jul 2, 2020

johlju commented Jul 2, 2020 • edited Loading

Kreby left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Jul 4, 2020

Kreby commented Jun 30, 2020 •

edited by johlju

Loading

johlju commented Jun 30, 2020 •

edited

Loading

johlju commented Jul 2, 2020 •

edited

Loading