SqlServerRole: Add support for nested roles and remove case-sensitive checks

[nabrond]

Pull Request (PR) description

Adds support to the resource for nesting server role membership. Fixes an issue where role membership tests were performed in a case-sensitive manner. Updated the unit test template to v1.2.4 and refactored mocks and test structure to decrease the runtime and resolve some false test results.

This Pull Request (PR) fixes the following issues

• Fixes SqlServerRole: Add support for nested roles #1452
• Fixes SQLServerRole uses contains() which is case sensitive #1153
• Fixes SqlServerRole: Update unit tests to use the same (newest) unit test template #1086
• Partial fix of SqlServerDsc: Speed up testing #979

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 

[johlju]

Working on getting the new CI pipeline working, so the tests won't pass yet. I will review this and test manually. But would like to merge this once I got the new CI pipeline in.

[johlju]

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nabrond)

---

DSCResources/MSFT_SqlServerRole/MSFT_SqlServerRole.psm1, line 501 at r1 (raw file):

.Contains

Maybe we should not use case sensitive here either?

---

DSCResources/MSFT_SqlServerRole/MSFT_SqlServerRole.psm1, line 562 at r1 (raw file):

.Contains

Maybe we should not use case sensitive here either?

[johlju]

The new CI pipeline has merged. This changed the folder structure, and also removed the dev branch. Please rebase against the master branch, and make sure to get your changes into the the file in the new location (under source folder). 😃

Read more here about the new coding workflow:
https://dsccommunity.org/guidelines/contributing/#understand-the-coding-workflow
https://dsccommunity.org/guidelines/testing-guidelines/#running-tests
You can also use Invoke-Pester once you run build.ps1 -Task build (not documented yet)
https://dsccommunity.org/guidelines/contributing/#attach-your-fork-to-a-free-azure-devops-organization

[nabrond]

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @johlju)

---

DSCResources/MSFT_SqlServerRole/MSFT_SqlServerRole.psm1, line 501 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

.Contains

Maybe we should not use case sensitive here either?

My testing showed that these were not case sensitive, but I updated the code to avoid future confusion.

---

DSCResources/MSFT_SqlServerRole/MSFT_SqlServerRole.psm1, line 562 at r1 (raw file):

My testing showed that these were not case sensitive, but I updated the code to avoid future confusion.

[nabrond]

Ran into some issues with the CI conversion and my local git repo. Refactored a bit to get away from the Contains() calls in the add/drop functions.

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @johlju)

[johlju]

@nabrond What issues did you run into? Something I should document in the contribution guidelines?

[johlju]

@nabrond could you please rebase against branch master so we get rid of already merged commits (those that already been merged in other PR's)

[nabrond]

Rebased and pushed my branch again. Most of my issues were git-related. I never maintained master in my fork as everything was previously based off dev. Trying to get that back in-sync is still proving challenging, I continually encounter disconnected HEAD scenarios. I think once this PR is complete, I will recreate my copy of the repository for future work.

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

[johlju]

I thought git rebase would fix that for you. 🤔 I haven't seen the problem yet. But will look out for it.

[johlju]

There are still issues with the rebase. The PR wants to remove things from the change log.
I would do this to resolve this, will this fail with the error you mentioned?

git checkout master
git fetch origin master
git rebase origin/master
git push my --force
git checkout nabrond-fix-sqlserverrole
git rebase my/master
# fix any conflicts
git push my --force

[johlju]

You could also just delete the local master branch git branch -D master then checkout the master from upstream again, and force push that to your fork. Then rebase your working branch against your forks master branch.

[nabrond]

@johlju, I took at look at my git history for this branch and reverted the change that I think introduced the problems with CHANGELOG.MD. It looks like that change came in through one off my rebase operations. Hopefully this makes this PR good to go.

[johlju]

Reviewed 1 of 7 files at r2, 3 of 4 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nabrond)

a discussion (no related file):
Could you please add a integration tests that tests the nested roles? That adds a role into a role, and then removes the role from the role?

---

---

tests/Unit/MSFT_SqlServerRole.Tests.ps1, line 1146 at r6 (raw file):

speciifying

Typo

[johlju]

@nabrond Awesome work on this! 😃 Thank you! Would just love to have an integration tests, and I found a little typo.

[nabrond]

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

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add a integration tests that tests the nested roles? That adds a role into a role, and then removes the role from the role?

@johlju Integration tests added.

---

---

tests/Unit/MSFT_SqlServerRole.Tests.ps1, line 1146 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

speciifying

Typo

Done.

[johlju]

@nabrond I will continue the review tomorrow but a quick question in regards to the case-sensitivity. If are SQL instance is using a case-sensitive collation would that make it possible to have logins and users with different casing? 🤔 When looking at this now I thought that this might break existing use-cases by removing .Contains(). What are your thoughts? 🤔

[johlju]

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

---

CHANGELOG.md, line 39 at r8 (raw file):

Removed use of case-sensitive Contains() function when evalutating role membership.
``

In regards to the question in the main PR thread. This will not be an issue wit case-sensitive collations? It is never possible to have the same name of a login or group but with different casings?

[johlju]

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

---

CHANGELOG.md, line 39 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Removed use of case-sensitive Contains() function when evalutating role membership.
``

In regards to the question in the main PR thread. This will not be an issue wit case-sensitive collations? It is never possible to have the same name of a login or group but with different casings?

It is supported in SQL Server to have the same login with different casing (in sensitive collations). But it will fail when compiling the mof-file as mentioned here #1191.

[johlju]

@nabrond Thank you for the work on this! 😄