Set-SqlDscServerPermission: Added support for assigning permissions to a server role

[IAMJDA]

Pull Request (PR) description

Updated Set-SqlDscServerPermission to be able to set permission on server roles. Adapted unit tests so they should continue to work but missing support for the new logic.

This Pull Request (PR) fixes the following issues

• Fixes Set-SqlDscServerPermission: Not possible to set server permissions for a server role #2059

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 updated in the resource's README.md.
• Resource parameter descriptions updated in schema.mof.
• Comment-based help updated, including parameter descriptions.
• Localization strings updated.
• Examples updated.
• Unit tests updated. See DSC Community Testing Guidelines.
• Integration tests updated (where possible). See DSC Community Testing Guidelines.
• Code changes adheres to DSC Community Style Guidelines.

---

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94%. Comparing base (eefcd14) to head (3d3bc96).
Report is 4 commits behind head on main.

Additional details and impacted files

@@         Coverage Diff          @@
##           main   #2061   +/-   ##
====================================
  Coverage    94%     94%           
====================================
  Files       105     105           
  Lines      8088    8089    +1     
====================================
+ Hits       7661    7662    +1     
  Misses      427     427           

Flag	Coverage Δ
unit	94% <100%> (+<1%)	⬆️

Files with missing lines	Coverage Δ
source/Public/Set-SqlDscServerPermission.ps1	100% <100%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[johlju]

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

---

source/Classes/020.SqlPermission.ps1 line 373 at r1 (raw file):

            New-InvalidOperationException -Message $missingPrincipalMessage
        }

We should not need this check at all since it is already evaluated in the Set-SqlDscServerPermission command. I rather see this change for the resource SqlPermission in a separate PR as we also need integration tests. Please revert the changes for this file and move it to a separate PR. 🙂

Code quote:

        $testSqlDscIsPrincipalParameters = @{
            ServerObject      = $serverObject
            Name              = $this.Name
        }

        # This will test wether the principal exist.
        $isLogin = Test-SqlDscIsLogin @testSqlDscIsPrincipalParameters
        $isRole = Test-SqlDscIsRole @testSqlDscIsPrincipalParameters

        if (-not $isLogin -and -not $isRole)
        {
            $missingPrincipalMessage = $this.localizedData.NameIsMissing -f @(
                $this.Name,
                $this.InstanceName
            )

            New-InvalidOperationException -Message $missingPrincipalMessage
        }

---

source/Public/Set-SqlDscServerPermission.ps1 line 107 at r1 (raw file):

        if (-not $isLogin -and -not $isRole)
        {
            $missingPrincipalMessage = $script:localizedData.ServerPermission_MissingPrincipal -f $Name, $ServerObject.InstanceName

We need to update the localized string

SqlServerDsc/source/en-US/SqlServerDsc.strings.psd1

Line 28 in 666ef4e

ServerPermission_MissingPrincipal = The principal '{0}' is not a login on the instance '{1}'.

Code quote:

ServerPermission_MissingPrincipal

---

source/Public/Set-SqlDscServerPermission.ps1 line 117 at r1 (raw file):

                )
            )
            return

We should not need return here as we throw a terminating error?

Code quote:

return

---

tests/Unit/Public/Set-SqlDscServerPermission.Tests.ps1 line 430 at r1 (raw file):

                Mock -CommandName Test-SqlDscIsRole -MockWith {
                    return $false
                }

We should add a test that actually test the it is a role and not a login.

Code quote:

                Mock -CommandName Test-SqlDscIsRole -MockWith {
                    return $false
                }

[github-actions]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[IAMJDA]

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

---

source/Public/Set-SqlDscServerPermission.ps1 line 107 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We need to update the localized string

SqlServerDsc/source/en-US/SqlServerDsc.strings.psd1

Line 28 in 666ef4e

ServerPermission_MissingPrincipal = The principal '{0}' is not a login on the instance '{1}'.

Done.

---

source/Public/Set-SqlDscServerPermission.ps1 line 117 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should not need return here as we throw a terminating error?

Done.

---

tests/Unit/Public/Set-SqlDscServerPermission.Tests.ps1 line 430 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should add a test that actually test the it is a role and not a login.

Done.

---

source/Classes/020.SqlPermission.ps1 line 373 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should not need this check at all since it is already evaluated in the Set-SqlDscServerPermission command. I rather see this change for the resource SqlPermission in a separate PR as we also need integration tests. Please revert the changes for this file and move it to a separate PR. 🙂

Will do

[johlju]

Just a tiny comment, then I think this is good to go. Great work!

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IAMJDA)

---

tests/Unit/Public/Set-SqlDscServerPermission.Tests.ps1 line 53 at r1 (raw file):

Describe 'Set-SqlDscServerPermission' -Tag 'Public' {
    Context 'When the principal does not exist' {

Why was this Context removed? 🤔 It still worked pretty well to keep it, to indicate that we test when neither role or login exist?

Code quote:

Context 'When the principal does not exist' {

[github-actions]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[johlju]

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

[johlju]

@IAMJDA I took this one over the finish line. Looking forwards to more PRs. Thank you.

[IAMJDA]

@johlju Thanks for completing it. Didn't had the chance to fix it in the last weeks 😔