SqlSetup: Fix for issue 448

[nesith]

Pull Request (PR) description

• Changes to SqlSetup
  • Changed the logic of how default value of FailoverClusterGroupName is
    set since that was preventing the resource to be able to be debugged
    (issue #448).

This Pull Request (PR) fixes the following issues

• Fixes SqlSetup: Unable to enable debug mode #448

Task list

• Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (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 Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[nesith]

@johlju PR

[johlju]

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nesith)

---

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

...as this was causing
the...

Maybe this is more descriptive: ...since that was preventing the resource to be able to be debugged ([issue #448](https://github.com/PowerShell/SqlServerDsc/issues/448)).

---

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1029 at r1 (raw file):

    # Fixing issue 448, setting FailoverClusterGroupName to default value if not specified in configuration
    

We can remove this blank row.

---

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1030 at r1 (raw file):

if(

We should use space between the if-statement and the parenthesis if (.

---

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1032 at r1 (raw file):

$boundParameters.FailoverClusterGroupName

I think it is better to assign it like below, because it it is not part of $PSBoundParameters we don't want it there after we set the default value either. That could indicate wrongly in later code that the user assigned the value when we in fact assigned the default value. $PSBoundParameters should normally only contain those parameters that the user assigned in the configuration. 🙂

$FailoverClusterGroupName = "SQL Server($InstanceName)"

---

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 1032 at r1 (raw file):

"SQL Server($InstanceName)"

There should be a space in the name "SQL Server ($InstanceName)" if I look at the old code. 🙂

---

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2155 at r1 (raw file):

    Write-Verbose -Message ($script:localizedData.FeaturesFound -f $($getTargetResourceResult.Features))

    # Fixing issue 448, setting FailoverClusterGroupName to default value if not specified in configuration

Please update this block with the same changes as was made above.

---

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.psm1, line 2194 at r1 (raw file):

}#>

This this extra #> should be removed?

---

DSCResources/MSFT_SqlSetup/MSFT_SqlSetup.schema.mof, line 56 at r1 (raw file):

[Write, Description("The name of the resource group to create for the clustered SQL Server instance.")] String FailoverClusterGroupName;

This should still say ... . Default is 'SQL Server (InstanceName)'., because it still applies, right?

[johlju]

There are merge conflicts since your PR was not based on the latest changes. Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history). If you don't know how to rebase your local dev and working branch, please look at how to Resolve merge conflicts.
Let me know if you need any assistance.

[nesith]

Sorry had a very busy couple of weeks at work, I will fix the issues sometime this week.

[johlju]

@nesith no worries. Looking forward merging this. 🙂

[johlju]

When you have time to work on this, then this PR need to be rebased too because of changes been merged into dev 😃

[codecov-io]

Codecov Report

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

@@          Coverage Diff          @@
##            dev   #1301    +/-   ##
=====================================
+ Coverage    98%     98%   +<1%     
=====================================
  Files        36      36            
  Lines      5331    5335     +4     
=====================================
+ Hits       5225    5229     +4     
  Misses      106     106

[nesith]

@johlju Hi, Seems the chnages I made to the dynamic variables as per your comments are breaking the tests, I will have a look at this this weekend

[johlju]

@nesith sounds great! :) Let me know when you are done and I continue the review.

[johlju]

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nesith)

a discussion (no related file):
We should add a test that covers this change. I will help resolve that in a day or so. Just a reminder to myself. 🙂

---

[johlju]

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

We should add a test that covers this change. I will help resolve that in a day or so. Just a reminder to myself. 🙂

This change was already covered by tests.

---

[johlju]

Reviewable status: complete! all files reviewed, all discussions resolved

[johlju]

Closing and reopening to kick off the tests again,

[johlju]

@nesith Thank you sending in this PR! I took it over the finish line now. 🙂