BREAKING CHANGE: SqlRole: Resource overhaul for tests #1623

Fiander · 2020-10-06T10:12:07Z

Major overhaul of SQLRole, added tests, and fix for issue #550

Pull Request (PR) description

some remodeling of SqlRole.
lots of extra tests,
fix for #550

This Pull Request (PR) fixes the following issues

Fixes #550

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

added tests, and fix for issue #550

fix #550 lots of extra tests

with new code get-DscConfiguration does not return MembersToInclude and MembersToExclude.

johlju

Thank you for this. This is gonna take a while to review for me. It also feels like a breaking change with the change for Get-TargetResource if users are depending on how it worked prior to this suggested change. But I can stand behind the change that it should only report 'Present' or 'Absent' if all mandatory parameter is in the current state, in this case if the role exist.

I did a quick glance, so this is just a first batch of comments. 🙂

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

CHANGELOG.md, line 8 at r3 (raw file):

-SqlRole

Please add a space after the dash - SqlRole.

CHANGELOG.md, line 10 at r3 (raw file):

- Major overhaul of resource, added lots of tests.
  - Added sanitize function for issue #550

Please make these entries more descriptive of what this PR changes so it is clear to other users how it works now (what changed). These entries are used for the release notes in the automatic release pipeline. Also reference the issue like previous entries for the specific change log entry the fixes that issue.

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 22 at r3 (raw file):

#returns $null if role does not exist. Else an array of all role members. This Array can be empty.

This comment seems out of place. Is it meant to be in a .NOTES section for design information to other contributors?

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 80 at r3 (raw file):

members

Members (upper-case 'M')

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 81 at r3 (raw file):

membersToInclude

MembersToInclude (upper-case 'M')

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 82 at r3 (raw file):

membersToExclude

MembersToExclude (upper-case 'M')

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 620 at r3 (raw file):

Sanitize-InputObjects

I rather see this using approved verbs (Get-Verb).
Suggest renaming this to what it does <verb>-MemberParameters (something better than InputObjects which could be anything) 🙂

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 647 at r3 (raw file):

Quoted 5 lines of code…

if ($MembersToInclude -or $MembersToExclude)
        {
            $errorMessage = $script:localizedData.MembersToIncludeAndExcludeParamMustBeNull
            New-InvalidOperationException -Message $errorMessage
        }

Suggest removing this and instead use the helper function out in the *-ResourceTarget function:
https://github.com/dsccommunity/DscResource.Common#assert-boundparameter

The module is available and imported at the top of this module script file.

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 652 at r3 (raw file):

Quoted 17 lines of code…

if ($ServerRoleName -eq 'sysadmin')
        {
            if ($Members -notcontains 'SA')
            {
                $Members += 'SA'
            }
        }
    }
    else
    {
        if ($ServerRoleName -eq 'sysadmin')
        {
            if ($MembersToExclude -contains 'SA')
            {
                $MembersToExclude = $MembersToExclude -ne 'SA'
            }
        }

Is it possible to remove some of the duplicate code here?

Fiander · 2020-10-07T17:04:41Z

Hi Johan, will do all those things.
I just did not see a simple way to integrate the skip SA part with the functions as they were.

For now just ignore the review I'm really fighting the integration tests. something is off on my test machine and no mof files are generated anymore. Of no single resource. not related to this resource. but i want to do this right.

I do have one request... could you please take a look at the unit test row 973 (It 'Should return false when ErrorAction is set to SilentlyContinue' )
I just don't believe this test works as expected. the success of this test seems just plain luck.
As i see it, the exception goes, and result will never get a value or even exist.
then, because it does not exist, it evaluates to $false what makes the test a success.
I would vote to remove this test, but it is not my code so i want your advice.

Last thing: would Get-CorrectedMemberParameters be a good name for renaming Sanitize-InputObjects?

codecov · 2020-10-12T23:10:57Z

Codecov Report

Merging #1623 into master will decrease coverage by 0%.
The diff coverage is 95%.

@@          Coverage Diff           @@
##           master   #1623   +/-   ##
======================================
- Coverage      98%     98%   -1%     
======================================
  Files          37      37           
  Lines        6014    6041   +27     
======================================
+ Hits         5925    5950   +25     
- Misses         89      91    +2

Flag	Coverage Δ
unit	`98% <95%> (-1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1	`98% <95%> (-2%)`	⬇️

[skip ci]

…o sqlrole

johlju

Sorry for not reviewing this sooner, but the regular work has been a lot lately and taking all the bandwidth.

Reviewed 5 of 6 files at r5.
Reviewable status: 5 of 6 files reviewed, 8 unresolved discussions (waiting on @Fiander)

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 152 at r5 (raw file):

        MembersToInclude = $MembersToInclude
        MembersToExclude = $MembersToExclude

Instead of returning $null I would rather that we keep this so it returns the actual values that was provided in the configuration. When you run Get-DscConfiguration or similar it is possible to compare the actual members with the desired members in thouse two properties. Although I can see it will not return the desired members if only the Members property was used... 🤔 But keeping these avoids a breaking change. What are your thoughts?

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 620 at r5 (raw file):

            #this is never reached. Commented out; This should be removed togeter with row 979 till 1017 in DSC_SqlRole.Tests.ps1, but not my decision
            #return $false

Yes we can remove it, and the test. Must be a left over that was missed in a previous change.

source/DSCResources/DSC_SqlRole/README.md, line 8 at r5 (raw file):

When the target role is sysadmin, SA wil not be removed from this role.

Maybe this is a bit more descriptive? "When the target role is sysadmin the DSC resource will prevent the user 'sa' from being removed. This is done to keep the DSC resource from throwing an error since SQL Server does not allow this user to be removed."

tests/Integration/DSC_SqlRole.Integration.Tests.ps1, line 103 at r5 (raw file):

Quoted 16 lines of code…

            $configurationParameters = @{
                OutputPath                 = $TestDrive
                # The variable $ConfigurationData was dot-sourced above.
                ConfigurationData          = $ConfigurationData
            }

            & $configurationName @configurationParameters

            $startDscConfigurationParameters = @{
                Path         = $TestDrive
                ComputerName = 'localhost'
                Wait         = $true
                Verbose      = $true
                Force        = $true
                ErrorAction  = 'Stop'
            }

This change will add a technical debt when we move to Pester 5. In Pester 5 code should either be inside a it-block or inside a before*-block. Also by changing this the test will look different than all other integration tests. What was the reason this block moved outside the it-block?

tests/Integration/DSC_SqlRole.Integration.Tests.ps1, line 153 at r5 (raw file):

Quoted 16 lines of code…

            $configurationParameters = @{
                OutputPath                 = $TestDrive
                # The variable $ConfigurationData was dot-sourced above.
                ConfigurationData          = $ConfigurationData
            }

            & $configurationName @configurationParameters

            $startDscConfigurationParameters = @{
                Path         = $TestDrive
                ComputerName = 'localhost'
                Wait         = $true
                Verbose      = $true
                Force        = $true
                ErrorAction  = 'Stop'
            }

same as previous comment.

tests/Integration/DSC_SqlRole.Integration.Tests.ps1, line 206 at r5 (raw file):

Quoted 16 lines of code…

            $configurationParameters = @{
                OutputPath                 = $TestDrive
                # The variable $ConfigurationData was dot-sourced above.
                ConfigurationData          = $ConfigurationData
            }

            & $configurationName @configurationParameters

            $startDscConfigurationParameters = @{
                Path         = $TestDrive
                ComputerName = 'localhost'
                Wait         = $true
                Verbose      = $true
                Force        = $true
                ErrorAction  = 'Stop'
            }

Same as previous comment.

tests/Unit/DSC_SqlRole.Tests.ps1, line 251 at r5 (raw file):

$result = Get-TargetResource @testParameters

What was the reason this block moved outside the it-block? This code should be inside the It-block so that any unexpected error that is thrown is correctly handled by the It-block.

I unit tests it is okay to duplicate code, because it is more important that the unit tests is easily readable. In this case it is okay that Get-TargetResource is called for each It-block.

See comment about Pester 5 as well that I mentioned in the integration tests.

Throughout the unit test.

tests/Unit/DSC_SqlRole.Tests.ps1, line 267 at r5 (raw file):

                It 'Should be executed once' {
                    Assert-MockCalled -CommandName Connect-SQL -Exactly -Times 1 -Scope Context
                }

We should call Get-TargetResource @testParameters in this it-block as well so that Scope can be It.

Throughout the code.

Fiander

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

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 152 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

        MembersToInclude = $MembersToInclude
        MembersToExclude = $MembersToExclude
Instead of returning $null I would rather that we keep this so it returns the actual values that was provided in the configuration. When you run Get-DscConfiguration or similar it is possible to compare the actual members with the desired members in thouse two properties. Although I can see it will not return the desired members if only the Members property was used... 🤔 But keeping these avoids a breaking change. What are your thoughts?

Because the return will always be the actual members in the role, and not the desired members in a role, id do not see the point in doing the same for $MembersToInclude and exclude. When using this, you wil always have to use other code for the original value of $Members. The way I made it, makes the function much more consistent. you get "present" if the role exists, and when it exists, you get the current members. The way it was: if a role does not exists, you still get $membersToInclude And $membersToExclude. To me that seems strange behavior.

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 620 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

            #this is never reached. Commented out; This should be removed togeter with row 979 till 1017 in DSC_SqlRole.Tests.ps1, but not my decision
            #return $false
Yes we can remove it, and the test. Must be a left over that was missed in a previous change.

I removed it.

source/DSCResources/DSC_SqlRole/README.md, line 8 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

When the target role is sysadmin, SA wil not be removed from this role.
Maybe this is a bit more descriptive? "When the target role is sysadmin the DSC resource will prevent the user 'sa' from being removed. This is done to keep the DSC resource from throwing an error since SQL Server does not allow this user to be removed."

I like your style :-) it is done

tests/Integration/DSC_SqlRole.Integration.Tests.ps1, line 103 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

            $configurationParameters = @{
                OutputPath                 = $TestDrive
                # The variable $ConfigurationData was dot-sourced above.
                ConfigurationData          = $ConfigurationData
            }

            & $configurationName @configurationParameters

            $startDscConfigurationParameters = @{
                Path         = $TestDrive
                ComputerName = 'localhost'
                Wait         = $true
                Verbose      = $true
                Force        = $true
                ErrorAction  = 'Stop'
            }
This change will add a technical debt when we move to Pester 5. In Pester 5 code should either be inside a it-block or inside a before*-block. Also by changing this the test will look different than all other integration tests. What was the reason this block moved outside the it-block?

to be honest.....
I just forgot to encapsulate it in a BeforeAll...
I see that code as setup code.
https://github.com/pester/Pester#put-setup-in-beforeall

tests/Integration/DSC_SqlRole.Integration.Tests.ps1, line 153 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

            $configurationParameters = @{
                OutputPath                 = $TestDrive
                # The variable $ConfigurationData was dot-sourced above.
                ConfigurationData          = $ConfigurationData
            }

            & $configurationName @configurationParameters

            $startDscConfigurationParameters = @{
                Path         = $TestDrive
                ComputerName = 'localhost'
                Wait         = $true
                Verbose      = $true
                Force        = $true
                ErrorAction  = 'Stop'
            }

same as previous comment.

Done.

tests/Integration/DSC_SqlRole.Integration.Tests.ps1, line 206 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

            $configurationParameters = @{
                OutputPath                 = $TestDrive
                # The variable $ConfigurationData was dot-sourced above.
                ConfigurationData          = $ConfigurationData
            }

            & $configurationName @configurationParameters

            $startDscConfigurationParameters = @{
                Path         = $TestDrive
                ComputerName = 'localhost'
                Wait         = $true
                Verbose      = $true
                Force        = $true
                ErrorAction  = 'Stop'
            }

Same as previous comment.

Done.

tests/Unit/DSC_SqlRole.Tests.ps1, line 251 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$result = Get-TargetResource @testParameters
What was the reason this block moved outside the it-block? This code should be inside the It-block so that any unexpected error that is thrown is correctly handled by the It-block.

I unit tests it is okay to duplicate code, because it is more important that the unit tests is easily readable. In this case it is okay that Get-TargetResource is called for each It-block.

See comment about Pester 5 as well that I mentioned in the integration tests.

Throughout the unit test.

to be honest.....
I just forgot to encapsulate it in a BeforeAll...
I see that part of the code as setup code.
https://github.com/pester/Pester#put-setup-in-beforeall

tests/Unit/DSC_SqlRole.Tests.ps1, line 267 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                It 'Should be executed once' {
                    Assert-MockCalled -CommandName Connect-SQL -Exactly -Times 1 -Scope Context
                }
We should call Get-TargetResource @testParameters in this it-block as well so that Scope can be It.

Throughout the code.

I did it this way to get more consistent code

we wil have to change this anyway for pester v5, but as of now i can not find how it should be.

from:
https://pester.dev/docs/commands/Assert-MockCalled
THIS COMMAND IS OBSOLETE AND WILL BE REMOVED SOMEWHERE DURING v5 LIFETIME, USE Should -Invoke INSTEAD.

johlju

Did you push a change, I see comments that things have been resolved but seems there is no commit. Maybe you waiting for answers on your comments, before pushing if so ignore this. 🙂

Reviewable status: 5 of 6 files reviewed, 8 unresolved discussions (waiting on @Fiander and @johlju)

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 152 at r5 (raw file):

Previously, Fiander wrote…

Because the return will always be the actual members in the role, and not the desired members in a role, id do not see the point in doing the same for $MembersToInclude and exclude. When using this, you wil always have to use other code for the original value of $Members. The way I made it, makes the function much more consistent. you get "present" if the role exists, and when it exists, you get the current members. The way it was: if a role does not exists, you still get $membersToInclude And $membersToExclude. To me that seems strange behavior.

Thanks for your view on it. I agree with your reasoning. I wonder if we should add a .NOTES section in the comment-based help for Get-TargetResource and add your reasoning above. So that another contributor does not add them back (and a reminder for maintainers if one would ever). 🤔

tests/Integration/DSC_SqlRole.Integration.Tests.ps1, line 103 at r5 (raw file):

Previously, Fiander wrote…

to be honest.....
I just forgot to encapsulate it in a BeforeAll...
I see that code as setup code.
https://github.com/pester/Pester#put-setup-in-beforeall

This line & $configurationName @configurationParameters is actually the "Should compile" of the It-block and is a test to verify the schema against a configuration. So I think that part is not setup code in this case. I agree the other stuff can go in a BeforeAll-block.

tests/Unit/DSC_SqlRole.Tests.ps1, line 251 at r5 (raw file):

Previously, Fiander wrote…

to be honest.....
I just forgot to encapsulate it in a BeforeAll...
I see that part of the code as setup code.
https://github.com/pester/Pester#put-setup-in-beforeall

That section explains the new behavior of Pester 5 where the main setup code for the test file must be in a Describe-block, in this test file that would be lines 1-42 that needs to be in a BeforeAll-block at the top of the file. That is not possible with Pester 4.

With that said I would be okay to have the $result = Get-TargetResource @testParameters in a BeforeEach-block so it is run prior each It-block. Though I'm not sure how Pester handles errors thrown in the BeforeEach/BeforeAll-block. But maybe you can just add a throw in the code and see how Pester handles it? If Pester catches the error correctly (outputs a correct error to the contributor) then I'm okay with it 😄

Thoughts?

tests/Unit/DSC_SqlRole.Tests.ps1, line 258 at r5 (raw file):

-Be $null

Can be -BeNullOrEmpty instead?

tests/Unit/DSC_SqlRole.Tests.ps1, line 267 at r5 (raw file):

Previously, Fiander wrote…

I did it this way to get more consistent code

we wil have to change this anyway for pester v5, but as of now i can not find how it should be.

from:
https://pester.dev/docs/commands/Assert-MockCalled
THIS COMMAND IS OBSOLETE AND WILL BE REMOVED SOMEWHERE DURING v5 LIFETIME, USE Should -Invoke INSTEAD.

Assert-MockCalled basically just have to be renamed to Should -Invoke for Pester 5. Otherwise the behavior is the same as far as I know (no breaking changes in the parameters).

Let se how this plays out depending on the previous comment. Resolving this comment for now.

Fiander

Reviewable status: 5 of 6 files reviewed, 8 unresolved discussions (waiting on @Fiander and @johlju)

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 652 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ($ServerRoleName -eq 'sysadmin')
        {
            if ($Members -notcontains 'SA')
            {
                $Members += 'SA'
            }
        }
    }
    else
    {
        if ($ServerRoleName -eq 'sysadmin')
        {
            if ($MembersToExclude -contains 'SA')
            {
                $MembersToExclude = $MembersToExclude -ne 'SA'
            }
        }

Is it possible to remove some of the duplicate code here?

euh... what duplicate code?

tests/Unit/DSC_SqlRole.Tests.ps1, line 251 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

That section explains the new behavior of Pester 5 where the main setup code for the test file must be in a Describe-block, in this test file that would be lines 1-42 that needs to be in a BeforeAll-block at the top of the file. That is not possible with Pester 4.

With that said I would be okay to have the $result = Get-TargetResource @testParameters in a BeforeEach-block so it is run prior each It-block. Though I'm not sure how Pester handles errors thrown in the BeforeEach/BeforeAll-block. But maybe you can just add a throw in the code and see how Pester handles it? If Pester catches the error correctly (outputs a correct error to the contributor) then I'm okay with it 😄

Thoughts?

in the parts where I test for exceptions, that part is in an execution block within an IT block. not all Contexts need to be checked

tests/Unit/DSC_SqlRole.Tests.ps1, line 258 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Be $null
Can be -BeNullOrEmpty instead?

Did i really mis two? fixed

Fiander

yes, i did yesterday and now again for the -Be $null

Reviewable status: 5 of 6 files reviewed, 8 unresolved discussions (waiting on @Fiander and @johlju)

Fiander

i think i saved it in my wrong repo. now should be good>?

Reviewable status: 5 of 6 files reviewed, 8 unresolved discussions (waiting on @Fiander and @johlju)

…#1626) - SqlDatabaseRole - Added test to ensure Add-SqlDscDatabaseRoleMember throws the expected error (issue #1620).

Fiander

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

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 152 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Thanks for your view on it. I agree with your reasoning. I wonder if we should add a .NOTES section in the comment-based help for Get-TargetResource and add your reasoning above. So that another contributor does not add them back (and a reminder for maintainers if one would ever). 🤔

wouldnt this reasoning be for all resources? Most of them already follow it, but SqlScript for example does not.
Ive added a .Notes section

tests/Integration/DSC_SqlRole.Integration.Tests.ps1, line 103 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This line & $configurationName @configurationParameters is actually the "Should compile" of the It-block and is a test to verify the schema against a configuration. So I think that part is not setup code in this case. I agree the other stuff can go in a BeforeAll-block.

After i've reed it for the fourth time I finally got what you mean. changed it.

tests/Unit/DSC_SqlRole.Tests.ps1, line 267 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Assert-MockCalled basically just have to be renamed to Should -Invoke for Pester 5. Otherwise the behavior is the same as far as I know (no breaking changes in the parameters).

Let se how this plays out depending on the previous comment. Resolving this comment for now.

OK

Fiander · 2020-11-05T11:40:26Z

pfiew... build failed on SqlLogin ( illegal registry thingy ) not on my code.

johlju

Reviewed 1 of 5 files at r6, 1 of 3 files at r8, 1 of 1 files at r10.
Reviewable status: 4 of 7 files reviewed, 1 unresolved discussion (waiting on @Fiander and @johlju)

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 152 at r5 (raw file):

Previously, Fiander wrote…

wouldnt this reasoning be for all resources? Most of them already follow it, but SqlScript for example does not.
Ive added a .Notes section

Yes the reasoning should be for all resources. Though, it can be hard for me as a maintainer to remember to enforce this when PR's are far apart. 🙂 Thanks for adding the notes section, this will help in future I think.

johlju · 2020-11-21T10:15:57Z

Closing this and will continue the review in the other PR.

Fiander and others added 7 commits October 6, 2020 12:00

Major overhaul of resource.

a361e0a

added tests, and fix for issue #550

major overhaul

bb949fc

fix #550 lots of extra tests

Merge branch 'master' into master

343df27

parameterblock added

14e1f04

Merge branch 'master' of https://github.com/Fiander/SqlServerDsc

65478eb

fix for changed parameter return in test

174e4e7

Forgot integration tests after rehaul.

c16cd25

with new code get-DscConfiguration does not return MembersToInclude and MembersToExclude.

johlju added the needs review The pull request needs a code review. label Oct 7, 2020

johlju requested changes Oct 7, 2020

View reviewed changes

johlju changed the title ~~SQLRole resource overhaul for tests. fix for #550~~ SQLRole: Resource overhaul for tests Oct 7, 2020

johlju changed the title ~~SQLRole: Resource overhaul for tests~~ SqlRole: Resource overhaul for tests Oct 7, 2020

johlju added the breaking change When used on an issue, the issue has been determined to be a breaking change. label Oct 7, 2020

johlju changed the title ~~SqlRole: Resource overhaul for tests~~ BREAKING CHANGE: SqlRole: Resource overhaul for tests Oct 7, 2020

Fiander added 4 commits October 12, 2020 21:42

With dubug code

fdd7ac4

debug pipeline

ddb2b45

debuf pipeline

a833614

new branch

aeff59c

Fiander and others added 11 commits October 13, 2020 08:29

with verbose

c772ac1

just do it

e4c3ecf

azure cicd working?

e92d1f3

change for cicd

683456b

test cicd

1563d5d

test change

91374be

Set up CI with Azure Pipelines

34f8b50

[skip ci]

fix with {

e3114f5

Merge branch 'sqlrole' of https://github.com/Fiander/SqlServerDsc int…

5fec5fa

…o sqlrole

yet another try

4d00273

jippie

55d7f95

Fiander and others added 2 commits October 16, 2020 09:43

axidential updated azure-pipelines with my own version

2cb8255

Update azure-pipelines.yml

dc92c33

johlju requested changes Oct 31, 2020

View reviewed changes

some changes as requested

423f6f5

Fiander commented Oct 31, 2020

View reviewed changes

johlju requested changes Nov 1, 2020

View reviewed changes

johlju self-requested a review November 1, 2020 12:01

Fiander commented Nov 1, 2020

View reviewed changes

Fiander added 2 commits November 1, 2020 13:29

-Be $null removed

7735fed

now should be good

5744e52

Fiander commented Nov 1, 2020

View reviewed changes

Fiander and others added 6 commits November 1, 2020 13:46

Merge branch 'master' into master

0f26946

fixed integrationTests for exeption

11f926c

Merge branch 'sqlrole'

901d29a

Merge branch 'master' of https://github.com/Fiander/SqlServerDsc

93ec9c3

fixed integrationTests for exeption

5d51aa5

SqlDatabaseRole: Add a test to check error message on helper function (…

56db008

…#1626) - SqlDatabaseRole - Added test to ensure Add-SqlDscDatabaseRoleMember throws the expected error (issue #1620).

Fiander commented Nov 2, 2020

View reviewed changes

Fiander added 3 commits November 4, 2020 18:49

IntegrationTest updates

7888735

update on integration tests

5343549

Merge branch 'master' of https://github.com/Fiander/SqlServerDsc

d37ca3d

Fiander mentioned this pull request Nov 16, 2020

BREAKING CHANGE: SqlRole: Resource overhaul for tests #1631

Merged

9 tasks

johlju reviewed Nov 21, 2020

View reviewed changes

johlju self-requested a review November 21, 2020 10:15

johlju closed this Nov 21, 2020

johlju removed breaking change When used on an issue, the issue has been determined to be a breaking change. needs review The pull request needs a code review. labels Nov 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING CHANGE: SqlRole: Resource overhaul for tests #1623

BREAKING CHANGE: SqlRole: Resource overhaul for tests #1623

Fiander commented Oct 6, 2020 •

edited

Loading

johlju left a comment

Fiander commented Oct 7, 2020

codecov bot commented Oct 12, 2020 •

edited

Loading

johlju left a comment

Fiander left a comment

johlju left a comment

Fiander left a comment

Fiander left a comment

Fiander left a comment

Fiander left a comment

Fiander commented Nov 5, 2020

johlju left a comment

johlju commented Nov 21, 2020

BREAKING CHANGE: SqlRole: Resource overhaul for tests #1623

BREAKING CHANGE: SqlRole: Resource overhaul for tests #1623

Conversation

Fiander commented Oct 6, 2020 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

johlju left a comment

Choose a reason for hiding this comment

Fiander commented Oct 7, 2020

codecov bot commented Oct 12, 2020 • edited Loading

Codecov Report

johlju left a comment

Choose a reason for hiding this comment

Fiander left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

Fiander left a comment

Choose a reason for hiding this comment

Fiander left a comment

Choose a reason for hiding this comment

Fiander left a comment

Choose a reason for hiding this comment

Fiander left a comment

Choose a reason for hiding this comment

Fiander commented Nov 5, 2020

johlju left a comment

Choose a reason for hiding this comment

johlju commented Nov 21, 2020

Fiander commented Oct 6, 2020 •

edited

Loading

codecov bot commented Oct 12, 2020 •

edited

Loading