BREAKING CHANGE: SqlDatabaseRole: Refactored to support adding/removing roles, managing membership

[pshamus]

Pull Request (PR) description

This PR updates the SqlDatabaseRole resource to properly manage database roles, including adding/removing the role as well as managing role membership. The behavior has been updated to more closely represent what is done with SqlServerRole.

In addition to the change in functionality, the new resource also does not add users to the database. This will be implemented in a separate resource called SqlDatabaseUser.

This Pull Request (PR) fixes the following issues

• Partially #845 (See BREAKING CHANGE: SqlDatabaseRole: Should only handle database roles and not create database users #847)
• Fixes #847
• Fixes #1252
• Fixes #1339

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 

[codecov-io]

Codecov Report

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

@@          Coverage Diff          @@
##            dev   #1367    +/-   ##
=====================================
+ Coverage    97%     97%   +<1%     
=====================================
  Files        35      35            
  Lines      5172    5230    +58     
=====================================
+ Hits       5066    5124    +58     
  Misses      106     106

[johlju]

Updated the PR description slightly so we are not auto-closing issue #845. 🙂 I modified issue #845.

[johlju]

Are you working on issue #846? If so, how far along are you with the new resource SqlDatabaseUser? Since we are introducing a breaking change here that won't create users, is there another way to create the users until we have that resource in-place? 🤔

[johlju]

@pshamus Thank you for this PR! It makes the resource so much more intuitive! 😃 I'm reviewing now.

[johlju]

Awesome work on this! Clean code, it was easy to review! 😃

Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @pshamus)

a discussion (no related file):
Could we add an example that shows using Include and Exclude in the same configuration?

---

---

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

Changes to SqlDatabaseRole

Maybe mentioning that it does not add users to the database no longer, and add the breaking change label on that too.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 81 at r1 (raw file):

Quoted 4 lines of code…

        [Parameter()]
        [ValidateSet('Present', 'Absent')]
        [System.String]
        $Ensure = 'Present'

Not seeing this non-mandatory parameter is being used in this function? So it can be removed from the parameter list of Get-TargetResource function?

https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#get-targetresource-should-not-contain-unused-non-mandatory-parameters

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 97 at r1 (raw file):

$currentEnsure = 'Absent'

No need to set this since we are throwing after this row?

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 110 at r1 (raw file):

$currentEnsure = 'Absent'

No need to set this since we are throwing after this row?

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 119 at r1 (raw file):

$currentEnsure = 'Absent'

No need to set this since we are throwing after this row?

---

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

$currentEnsure = 'Absent'

Since the resource creates a role, I think we should only set Ensure property when the role exist or does not exist. Even if a property is wrong (in this case members) on a role that exists it should return 'Present'.

That could mean we could move all this member camprison to the Test-TargetResource instead. Here we just return the actual members in the Members property?

I raised the question in the Slack #DSC community too. Please join through Slack or Discord https://github.com/PowerShell/DscResources/blob/master/CommunityCalls/2019-05-08/agenda.md

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 143 at r1 (raw file):

$currentEnsure = 'Absent'

See previous comment.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 157 at r1 (raw file):

$currentEnsure = 'Absent'

See previous comment.

---

Examples/Resources/SqlDatabaseRole/4-MembersToIncludeInDatabaseRole.ps1, line 5 at r1 (raw file):

    ... and that users CONTOSO\Barbara and CONTOSO\Fred are added as members
    of this role.

This could sound that the users are only added once. Maybe mentioned that it will always enforce that the users are members of the role, but will not change any other members.

---

Examples/Resources/SqlDatabaseRole/5-MembersToExcludeFromDatabaseRole.ps1, line 4 at r1 (raw file):

    ... and that users CONTOSO\Barbara and CONTOSO\Fred are not members of
    this role.

Same as previous comment.

[johlju]

Due to a recent merge to the dev branch there are merge conflicts since your PR is no longer 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.

[pshamus]

Glad it was easy to review. I take pride in my code formatting, sometimes too much so.

I’ll rebase when I get a sec.

Lastly, I have working code for SqlDatabaseUser but no unit tests yet. Those will take me a while to get going.

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @johlju and @pshamus)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add an example that shows using Include and Exclude in the same configuration?

Yep, I can do that.

---

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 81 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

        [Parameter()]
        [ValidateSet('Present', 'Absent')]
        [System.String]
        $Ensure = 'Present'

Not seeing this non-mandatory parameter is being used in this function? So it can be removed from the parameter list of Get-TargetResource function?

https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#get-targetresource-should-not-contain-unused-non-mandatory-parameters

I’ll remove that parameter.

---

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

Previously, johlju (Johan Ljunggren) wrote…

$currentEnsure = 'Absent'

Since the resource creates a role, I think we should only set Ensure property when the role exist or does not exist. Even if a property is wrong (in this case members) on a role that exists it should return 'Present'.

That could mean we could move all this member camprison to the Test-TargetResource instead. Here we just return the actual members in the Members property?

I raised the question in the Slack #DSC community too. Please join through Slack or Discord https://github.com/PowerShell/DscResources/blob/master/CommunityCalls/2019-05-08/agenda.md

What’s the purpose of Get-TargetResource then? From what I’ve seen in other SqlServerDsc resources, it returns the same parameter values passed. If it’s meant to show the current state, I can return Members but not MembersToInclude or MembersToExclude. What should be returned for MembersToInclude and MembersToExclude then?

[johlju]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @johlju and @pshamus)

---

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

Previously, pshamus (Paul Shamus) wrote…

What’s the purpose of Get-TargetResource then? From what I’ve seen in other SqlServerDsc resources, it returns the same parameter values passed. If it’s meant to show the current state, I can return Members but not MembersToInclude or MembersToExclude. What should be returned for MembersToInclude and MembersToExclude then?

It should return the current state of the object. For MembersToInclude and MembersToExclude you should return the values passed in the parameter as you already do. No change there.
We should always return Ensure = Present if the group exist and even if the members are not in desired state. Ensure property means group exist it does not exist.

In this case, if we want the user to know that members property is not in desired state we could potentially add Boolean read-only propertyMembersInDesiredState that says so.

Does it make sense?

[johlju]

Above it should have said “Ensure property means group exist or does not exist.”. On the mobile writing the comment, and it tends to autocorrect wrong 😄

[pshamus]

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

a discussion (no related file):

Previously, pshamus (Paul Shamus) wrote…

Yep, I can do that.

Done.

---

---

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

Previously, johlju (Johan Ljunggren) wrote…

Changes to SqlDatabaseRole

Maybe mentioning that it does not add users to the database no longer, and add the breaking change label on that too.

Done.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 81 at r1 (raw file):

Previously, pshamus (Paul Shamus) wrote…

I’ll remove that parameter.

Done.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 97 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$currentEnsure = 'Absent'

No need to set this since we are throwing after this row?

Done.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 110 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$currentEnsure = 'Absent'

No need to set this since we are throwing after this row?

Done.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 119 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$currentEnsure = 'Absent'

No need to set this since we are throwing after this row?

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

It should return the current state of the object. For MembersToInclude and MembersToExclude you should return the values passed in the parameter as you already do. No change there.
We should always return Ensure = Present if the group exist and even if the members are not in desired state. Ensure property means group exist it does not exist.

In this case, if we want the user to know that members property is not in desired state we could potentially add Boolean read-only propertyMembersInDesiredState that says so.

Does it make sense?

Done.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 143 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$currentEnsure = 'Absent'

See previous comment.

Done.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 157 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$currentEnsure = 'Absent'

See previous comment.

Done.

---

Examples/Resources/SqlDatabaseRole/4-MembersToIncludeInDatabaseRole.ps1, line 5 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    ... and that users CONTOSO\Barbara and CONTOSO\Fred are added as members
    of this role.

This could sound that the users are only added once. Maybe mentioned that it will always enforce that the users are members of the role, but will not change any other members.

Done.

---

Examples/Resources/SqlDatabaseRole/5-MembersToExcludeFromDatabaseRole.ps1, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    ... and that users CONTOSO\Barbara and CONTOSO\Fred are not members of
    this role.

Same as previous comment.

Done.

[johlju]

Reviewed 9 of 9 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pshamus)

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 85 at r2 (raw file):

$roleStatus = 'Present'

At this point we don't know if the role exists. Not until line 94 we know if it exist or not? So should we start her with $roleStatus = 'Absent' and then after line 94 it should change to $roleStatus = 'Present'?

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 167 at r2 (raw file):

MembersInDesiredState

If this is added, then we need to add a Read only property to the schema.mof. See other resource in SqlServerDsc that uses a read only property. The read only property should also be added to the read-only parameter list in the README.md.

See the resource SqlDatabaseDefaultLocation for examples in schema.mof and README.md.

---

Examples/Resources/SqlDatabaseRole/6-MembersToIncludeAndExcludeInDatabaseRole.ps1, line 13 at r2 (raw file):

param(

Please move parenthesis down to its own row.

---

Tests/Unit/MSFT_SqlDatabaseRole.Tests.ps1, line 449 at r2 (raw file):

-Be $null

preferably be changed to -BeNullOrEmtpy.

---

Tests/Unit/MSFT_SqlDatabaseRole.Tests.ps1, line 490 at r2 (raw file):

$result.MembersInDesiredState | Should -Be $true

We are not testing this being $false. Can we add additional tests that tests when each member parameter is not in desired state?

[pshamus]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johlju and @pshamus)

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 85 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$roleStatus = 'Present'

At this point we don't know if the role exists. Not until line 94 we know if it exist or not? So should we start her with $roleStatus = 'Absent' and then after line 94 it should change to $roleStatus = 'Present'?

My thought is to move it after line 148 in the event any exceptions are thrown.

---

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 167 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

MembersInDesiredState

If this is added, then we need to add a Read only property to the schema.mof. See other resource in SqlServerDsc that uses a read only property. The read only property should also be added to the read-only parameter list in the README.md.

See the resource SqlDatabaseDefaultLocation for examples in schema.mof and README.md.

No problem, I'll add that.

---

Examples/Resources/SqlDatabaseRole/6-MembersToIncludeAndExcludeInDatabaseRole.ps1, line 13 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

param(

Please move parenthesis down to its own row.

Will do.

---

Tests/Unit/MSFT_SqlDatabaseRole.Tests.ps1, line 449 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Be $null

preferably be changed to -BeNullOrEmtpy.

Will do.

---

Tests/Unit/MSFT_SqlDatabaseRole.Tests.ps1, line 490 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$result.MembersInDesiredState | Should -Be $true

We are not testing this being $false. Can we add additional tests that tests when each member parameter is not in desired state?

Will do.

[pshamus]

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

---

Examples/Resources/SqlDatabaseRole/6-MembersToIncludeAndExcludeInDatabaseRole.ps1, line 13 at r2 (raw file):

Previously, pshamus (Paul Shamus) wrote…

Will do.

Done.

---

Tests/Unit/MSFT_SqlDatabaseRole.Tests.ps1, line 490 at r2 (raw file):

Previously, pshamus (Paul Shamus) wrote…

Will do.

Done.

[johlju]

Closing and reopening to kick off the tests again.

[johlju]

Reviewed 6 of 6 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[johlju]

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