Changes to xSQLServerLogin (Fixes #105, #31)

[johlju]

• Changes to xSQLServerLogin
  • Fixed an issue when dropping logins (issue Error when dropping a login using the resource xSqlServerLogin #105).
  • Fixed an issue where it was not possible to add the same login to two instances on the same server (xSQLServerLogin cannot be used for the same login on multiple SQL server instances on the same server #31).
  • Added unit test for the resource.

---

This change is

[msftclas]

Hi @johlju, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[mbreakey3]

Reviewed 3 of 5 files at r1, 1 of 2 files at r2.
Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 179 [r2] (raw file):

        Context 'When the system is not in the desired state' {
            It 'Should return desired state as absent when desired windows user don''t exists' {

change this to 'user does not exist'

---

Comments from Reviewable

[mbreakey3]

Thank you so much for adding in all these tests!

[johlju]

@mbreakey3 It's fun coding these test, I learn a lot! 😄
And I'm looking forward to when all the tests are written. Then we can add new tests for new functionality and then do the code changes so you see the code you wrote did what was expected. :)

[mbreakey3]

Reviewed 2 of 2 files at r3.
Review status: 4 of 5 files reviewed at latest revision, 8 unresolved discussions.

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 120 [r3] (raw file):

            $result = Get-TargetResource @testParameters

            It 'Should not return the state as present' {

should this be 'Should return the state as present'?

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 144 [r3] (raw file):

            $result = Get-TargetResource @testParameters

            It 'Should not return the state as present' {

Again, should this be 'Should return...'?

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 198 [r3] (raw file):

                }

                $result = Test-TargetResource @testParameters

You shouldn't be calling this function more than once in an It statement - divide it up into multiple It statements

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 227 [r3] (raw file):

            }

            It 'Should return that desired state as present when desired windows group exist' {

'group exists'

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 240 [r3] (raw file):

            }

            It 'Should return that desired state as present when desired sql login exist' {

should this be 'Should return the desired state as $true..' ?

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 345 [r3] (raw file):

            }

            It 'Should not throw an error when desired login should be absent but are present' {

rephrase this It statement

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 428 [r3] (raw file):

            }

            It 'Should not throw an error when desired login should be absent but are already absent' {

rephrase this 'It' statement

---

Comments from Reviewable

[johlju]

Reviewed 1 of 5 files at r1, 1 of 2 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 179 [r2] (raw file):

Previously, mbreakey3 (Mariah) wrote…

change this to 'user does not exist'

Done

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 120 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

should this be 'Should return the state as present'?

You are correct. Fixed.

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 144 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Again, should this be 'Should return...'?

You are correct. Fixed.

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 198 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

You shouldn't be calling this function more than once in an It statement - divide it up into multiple It statements

Done

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 227 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

'group exists'

Done

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 240 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

should this be 'Should return the desired state as $true..' ?

I changed it a little, but did not change to $true, my thinking is that Test-method returns "$true" to say it is "Present". So I think it's more readable with "Present". But I can go with $true as well if you think it's better.

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 345 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

rephrase this It statement

So this one I had to rework totally. This one returned false positive because the code caught the error and never throw. I had to rework the resource, stub and mocking to make this test work.

---

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 428 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

rephrase this 'It' statement

Same here. Reworked the test totally.

---

Comments from Reviewable

[johlju]

@mbreakey3 Did I break AppVeyor with this commit? How can I see what's wrong?

[johlju]

@mbreakey3 Never mind, I missed that you merged in a PR. I resolved the conflict and now AppVeyor is happy again 😄

[mbreakey3]

Reviewed 2 of 3 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 154 [r5] (raw file):

                    Write-Verbose "Deleting SQL login $Name"

                    $sqlLogin = $($sql.Logins[ $Name ])

you don't need to add the space within the brackets and parentheses. But there should be one after the 'if' before the '(' on the line below

---

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 235 [r5] (raw file):

    )

    if( ( $PSCmdlet.ShouldProcess( $($sqlLogin.Name), "Remove login" ) ) ) {

Again, the spaces aren't needed within the parentheses, but there should be a space after the 'if'

---

Comments from Reviewable

[johlju]

Reviewed 1 of 1 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 154 [r5] (raw file):

Previously, mbreakey3 (Mariah) wrote…

you don't need to add the space within the brackets and parentheses. But there should be one after the 'if' before the '(' on the line below

Done. I remembered this one wrong from the StyleGuide. Now I have changed all the parentheses back and added a space where it suppose to be in the whole file. :)

---

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 235 [r5] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Again, the spaces aren't needed within the parentheses, but there should be a space after the 'if'

Done

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 1 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[mbreakey3]

Now there's just the merge conflicts....

[johlju]

@mbreakey3 Resolved the conflicts

[mbreakey3]

---

Comments from Reviewable