BREAKING CHANGE: xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership: New Resource

Pull Request (PR) description
Add new resource xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.
This Pull Request (PR) fixes the following issues:
Fixes #425
Task list:

[X] Change details added to Unreleased section of CHANGELOG.md?
[X] Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
[X] Examples appropriately updated?
[X] New/changed code adheres to Style Guidelines?
[X] Unit and (optional) Integration tests created/updated where possible?

This change is

Finally got this in!
Please note, this doesn't currently support using automatic seeding. I plan on adding that in after this PR is completed.

Codecov Report

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

@@         Coverage Diff          @@
##            dev   #593    +/-   ##
====================================
+ Coverage    81%    82%    +1%
====================================
  Files        32     33     +1
  Lines      3434   3691   +257
====================================
+ Hits       2788   3045   +257
  Misses      646    646

Seems to be a problem with one of the common tests. I can look at that so we can get that fixed.
https://ci.appveyor.com/project/PowerShell/xsqlserver/build/5.0.1215.0#L202
A few other errors as well but seems like regular problems. Let me know if you need any help.

Looks like a few of the changes I made in the SMO stub didn't work well with some of the older AG tests. I'm working on fixing those tests.

@johlju, I think I got all my tests worked out. The only failure appveyor is reporting now is the module manifest check.

@randomnote1 Great work! I hoped I would have fixed that by now, but haven't gotten to it yet. I will try to fix it this weekend.

The problem is that the build worker does not have WMF 5.1 - seems that particular line that is failing needs WMF 5.1. I tried to change to another build worker image that already has WMF 5.1, but that failed with another problem. I opened an issue for that in DscResources.Tests (https://github.com/PowerShell/DscResource.Tests/issues/140).
My tests by fixing so I could use that build worker resulted i that the test was successful.
As a workaround I'm currently trying to use the same approach that PSDscResource does, by installing WMF 5.1 on the build worker. If that work we can start with that until the issue above is fixed.

You got a problem in the CHANGELOG.md still. You need to add a blank line after # Unreleased.
https://ci.appveyor.com/project/PowerShell/xsqlserver/build/5.0.1237.0#L6971

Okay, so I got a working fix for the common test - my thought is that you take the changes I did in my working branch and add them to this PR. Does that work for you? Or if you want I can send in a PR with the change. But though we could skip that to get this into your PR a little faster. 😄
You find the changes here:
https://github.com/johlju/xSQLServer/tree/test-framework-version-v2
And the only two files you need is:
appveyor.yml
Tests/TestHelpers/WMF5Dot1Installation.psm1
This fix is from the PSDscResources repository.

Cool. I pulled the files into my branch and pushed them into the PR. Should know shortly if everything works!

Seems to work! 😄

@randomnote1  I will start reviewing this one this weekend!

Sounds good. I wish you luck! I don't think this will be an easy review...

It will be fun :) Looking forward to it :p

Reviewed 5 of 13 files at r4, 1 of 4 files at r5, 5 of 8 files at r6, 2 of 3 files at r7, 1 of 1 files at r8.
Review status: 14 of 17 files reviewed at latest revision, 20 unresolved discussions.

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

- Added new resource
  - xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership
- **BREAKING CHANGE: The module now requires WMF 5.**

I think we need to add this as a breaking change under 'Changes to xSQLServer' at the top? Thinking that it should be the first one sees reading the change log.
Also add that it is because it it a requirement for class based resources.

README.md, line 172 at r8 (raw file):

[String]

Should be [String[]]

README.md, line 177 at r8 (raw file):

* **[String] AvailabilityGroupName** _(Key)_: The name of the availability group in which to manage the database membership(s).
* **[String] BackupPath** _(Required)_: The path used to seed the availability group replicas. This should be a path that is accessible by all of the replicas.
* **[String] Ensure** _(Write)_: Specifies the membership of the database(s) in the availability group. Default is Present. { *Present* | Absent | Exactly }

There are more text in the help.txt file for this parameter, can we get that into the parameter description or in the resource description (somewhere) if that is better.

README.md, line 177 at r8 (raw file):

* **[String] AvailabilityGroupName** _(Key)_: The name of the availability group in which to manage the database membership(s).
* **[String] BackupPath** _(Required)_: The path used to seed the availability group replicas. This should be a path that is accessible by all of the replicas.
* **[String] Ensure** _(Write)_: Specifies the membership of the database(s) in the availability group. Default is Present. { *Present* | Absent | Exactly }

I have comment on the Excatly value of Ensure parameter over at issue #425. Let's discuss it there.

README.md, line 178 at r8 (raw file):

PSDscRunAsAccount

Should be 'PsDscRunAsCredential'. In two locations.

xSQLServer.psd1, line 37 at r8 (raw file):

RequiredAssemblies

Does this work if SQL Server is not installed. Will this fail unless SQL Server is already installed?

xSQLServerHelper.psm1, line 978 at r8 (raw file):

}

function Test-AvailabilityReplicaSeedingModeAutomatic

Please add comment-based help

xSQLServerHelper.psm1, line 978 at r8 (raw file):

}

function Test-AvailabilityReplicaSeedingModeAutomatic

This helper function, should/can it preferably be used by the other AlwaysOn resource? If so, can you create an issue for changing the resource to use this helper functions?

xSQLServerHelper.psm1, line 984 at r8 (raw file):

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]

Please change to [System.String].

xSQLServerHelper.psm1, line 988 at r8 (raw file):

        [Parameter(Mandatory = $true)]
        [String]

Please change to [System.String].

xSQLServerHelper.psm1, line 993 at r8 (raw file):

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]

Please change to [System.String].

xSQLServerHelper.psm1, line 998 at r8 (raw file):

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]

Please change to [System.String].

xSQLServerHelper.psm1, line 1036 at r8 (raw file):

}

function Get-PrimaryReplicaServerObject

Please add comment-based help

xSQLServerHelper.psm1, line 1036 at r8 (raw file):

}

function Get-PrimaryReplicaServerObject

This helper function, should/can it preferably be used by the other AlwaysOn resource? If so, can you create an issue for changing the resource to use this helper functions?

xSQLServerHelper.psm1, line 1060 at r8 (raw file):

}

function Test-ImpersonatePermissions

Please add comment-based help

xSQLServerHelper.psm1, line 1060 at r8 (raw file):

}

function Test-ImpersonatePermissions

This helper function, should/can it preferably be used by the other AlwaysOn resource? If so, can you create an issue for changing the resource to use this helper functions?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/en-US/about_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.help.txt, line 39 at r8 (raw file):

PSDscRunAsAccount

Should be 'PsDscRunAsCredential'. In two locations.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/en-US/about_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.help.txt, line 43 at r8 (raw file):

    If set to $false, the owner of the database will be the PSDscRunAsAccount.

    The default is '$true'.

Need a new line at the end of the file.

Examples/Resources/xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/2-RemoveDatabasesFromAvailabilityGroup.ps1, line 100 at r8 (raw file):

        }

        xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership 'TestAGDatabaseMemberships'

I think you could remove all the resource to create the AlwaysOn replicas here. If you should remove databases, then yu probably already have done that, and you have that example in the first example.

Tests/Unit/xSQLServerHelper.Tests.ps1, line 883 at r8 (raw file):

        Context 'When the supplied server object is not the primary replica' {
            It 'Should the server object of the primary replica' {

Maybe 'Should return the actual server object...'?

Comments from Reviewable

This is impressive work @randomnote1! 😄

Review status: 11 of 17 files reviewed at latest revision, 34 unresolved discussions, some commit checks failed.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 70 at r8 (raw file):

        }

        return $currentConfiguration

Now I'm out of my depth here, but it seems that you could just return $this after setting the public variables (the parameters) to the actual values. In this case the DatabaseName to the actual databasenames and MatchDatabaseOwner to $false in the group does not exist.
Do you have an opinion on that?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 87 at r8 (raw file):

        if ( $databasesToAddToAvailabilityGroup.Count -gt 0 )
        {
            New-VerboseMessage -Message ( "Adding the following databases to the '{0}' availability group: {1}" -f $this.AvailabilityGroupName,( $databasesToAddToAvailabilityGroup -join ', ' ) )

You have switch the variables here.
This verbose message can be moved down to line 104, inside that if-block. Then you can remove this entire if-block

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 93 at r8 (raw file):

        if ( $databasesToRemoveFromAvailabilityGroup.Count -gt 0 )
        {
            New-VerboseMessage -Message ( "Removing the following databases from the '{0}' availability group: {1}" -f $this.AvailabilityGroupName,( $databasesToRemoveFromAvailabilityGroup -join ', ' ) )

You have switch the variables here.
This verbose message can be moved down to line 398, inside that if-block. Then you can remove this entire if-block. I guess you wanted to write out what will be done ("adding and removing this") at the start, but I think these verbose messages can just as well be in each block that actually do the action.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 139 at r8 (raw file):

                $database = $primaryServerObject.Databases[$databaseName]

                # Verify the prerequisites prior to joining the database to the availability group

Please make this a comment block
<#
    Text...
#>

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 171 at r8 (raw file):

FilestreamDirectoryName

Should this be FileStreamDirectoryName, upper 'S' in 'Stream'. If so, please  change all other occurrences.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 175 at r8 (raw file):

$availbilityReplicaFilestreamLevel

Typo in 'availability'; should be $availabilityReplicaFilestreamLevel
Same for all other occurrences.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 178 at r8 (raw file):

                    foreach ( $availabilityGroupReplica in $secondaryReplicas )
                    {
                        $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name

Always the default instance?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 184 at r8 (raw file):

                    if ( $availbilityReplicaFilestreamLevel.Values -contains 'Disabled' )
                    {
                        $prerequisiteCheckFailures += ( 'Filestream is disabled on the following instances: {0}' -f ( $availbilityReplicaFilestreamLevel.Keys -join ', ' ) )

This will list all instances, not just the ones that are wrong. It should just list the ones that are wrong.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 191 at r8 (raw file):

$availbilityReplicaContainmentEnabled

Typo in 'availability'; should be $availbilityReplicaContainmentEnabled

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 194 at r8 (raw file):

                    foreach ( $availabilityGroupReplica in $secondaryReplicas )
                    {
                        $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name

Always the default instance?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 200 at r8 (raw file):

                    if ( $availbilityReplicaContainmentEnabled.Values -notcontains 'None' )
                    {
                        $prerequisiteCheckFailures += ( 'Contained Database Authentication is not enabled on the following instances: {0}' -f ( $availbilityReplicaContainmentEnabled.Keys -join ', ' ) )

This will list all instances, not just the ones that are wrong. It should just list the ones that are wrong.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 206 at r8 (raw file):

                # Ensure the data and log file paths exist on all replicas
                $databaseFileDirectories = @()
                $databaseFileDirectories += $database.FileGroups.Files.FileName | ForEach-Object { Split-Path -Path $_ -Parent } | Select-Object -Unique

Please add -Process before the script block. Also, could we add the contents in the script block onto a new row, indented 4 spaces and the closing brace on yet another row? To shortening the row a bit.
Do you need Select-Object -Unique here? You do that on row 208.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 207 at r8 (raw file):

                $databaseFileDirectories = @()
                $databaseFileDirectories += $database.FileGroups.Files.FileName | ForEach-Object { Split-Path -Path $_ -Parent } | Select-Object -Unique
                $databaseFileDirectories += $database.LogFiles.FileName | ForEach-Object { Split-Path -Path $_ -Parent } | Select-Object -Unique

Please add -Process before the script block. Also, could we add the contents in the script block onto a new row, indented 4 spaces and the closing brace on yet another row ? To shortening the row a bit.
Do you need Select-Object -Unique here? You do that on row 208.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 210 at r8 (raw file):

                $databaseFileDirectories = $databaseFileDirectories | Select-Object -Unique

                $availabilityReplicaMissingDirectories = @{}

Just a note to continue the review from here $availabilityReplicaMissingDirectories = @{}.

Comments from Reviewable

@randomnote1 Saw you just pushed a new commit, so I will wait a while to continue the review so that the review comments don't get out of "line" to much. 😄

Yeah, I wanted to get the "Ensure" changes done before you did too much work. I'll respond to what you reviewed shortly.

@randomnote1 No worries! Just thought your push would disturb Reviewable so "saved" just in case 😄 But it handle it so have continued the review of the resource. I will review the new commit of "Ensure" tomorrow, as well as the tests.
See the tests are failing. I guess you are on that.

Man, I don't know how you find some of these inconsistencies. Nice work reviewing!

Review status: 6 of 17 files reviewed at latest revision, 33 unresolved discussions.

CHANGELOG.md, line 30 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I think we need to add this as a breaking change under 'Changes to xSQLServer' at the top? Thinking that it should be the first one sees reading the change log.
Also add that it is because it it a requirement for class based resources.

Done.

README.md, line 172 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

[String]

Should be [String[]]

Done.

README.md, line 177 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
There are more text in the help.txt file for this parameter, can we get that into the parameter description or in the resource description (somewhere) if that is better.

Done.

README.md, line 177 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I have comment on the Excatly value of Ensure parameter over at issue #425. Let's discuss it there.

Done.

README.md, line 178 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

PSDscRunAsAccount

Should be 'PsDscRunAsCredential'. In two locations.

Done.

xSQLServer.psd1, line 37 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

RequiredAssemblies

Does this work if SQL Server is not installed. Will this fail unless SQL Server is already installed?

That probably doesn't need to be there anymore. I will check on that.

xSQLServerHelper.psm1, line 978 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add comment-based help

Wow...can't believe I forgot these!
Done.

xSQLServerHelper.psm1, line 978 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This helper function, should/can it preferably be used by the other AlwaysOn resource? If so, can you create an issue for changing the resource to use this helper functions?

The comments got slightly out of line. Which function is this in reference to?

xSQLServerHelper.psm1, line 984 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please change to [System.String].

Done.

xSQLServerHelper.psm1, line 988 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please change to [System.String].

Done.

xSQLServerHelper.psm1, line 993 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please change to [System.String].

Done.

xSQLServerHelper.psm1, line 998 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please change to [System.String].

Done.

xSQLServerHelper.psm1, line 1036 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add comment-based help

Done.

xSQLServerHelper.psm1, line 1060 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add comment-based help

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 70 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Now I'm out of my depth here, but it seems that you could just return $this after setting the public variables (the parameters) to the actual values. In this case the DatabaseName to the actual databasenames and MatchDatabaseOwner to $false in the group does not exist.
Do you have an opinion on that?

That was my initial approach. Unfortunately what happens is it overwrites the configured properties on the object. So, after doing some digging, this is the preferred method of handling this.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 87 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You have switch the variables here.
This verbose message can be moved down to line 104, inside that if-block. Then you can remove this entire if-block

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 93 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You have switch the variables here.
This verbose message can be moved down to line 398, inside that if-block. Then you can remove this entire if-block. I guess you wanted to write out what will be done ("adding and removing this") at the start, but I think these verbose messages can just as well be in each block that actually do the action.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 139 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please make this a comment block
<#
    Text...
#>

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 171 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

FilestreamDirectoryName

Should this be FileStreamDirectoryName, upper 'S' in 'Stream'. If so, please  change all other occurrences.

Looks like it's lowercase according to the documentation
https://msdn.microsoft.com/en-us/library/microsoft.sqlserver.management.smo.database.aspx

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 175 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

$availbilityReplicaFilestreamLevel

Typo in 'availability'; should be $availabilityReplicaFilestreamLevel
Same for all other occurrences.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 178 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Always the default instance?

Connect-SQL accepts both 'ServerName' and 'ServerName\InstanceName' in the 'SQLServer' property. It worked in testing from what I can tell.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 184 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This will list all instances, not just the ones that are wrong. It should just list the ones that are wrong.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 191 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

$availbilityReplicaContainmentEnabled

Typo in 'availability'; should be $availbilityReplicaContainmentEnabled

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 194 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Always the default instance?

Connect-SQL accepts both 'ServerName' and 'ServerName\InstanceName' in the 'SQLServer' property. It worked in testing from what I can tell.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 200 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This will list all instances, not just the ones that are wrong. It should just list the ones that are wrong.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 206 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add -Process before the script block. Also, could we add the contents in the script block onto a new row, indented 4 spaces and the closing brace on yet another row? To shortening the row a bit.
Do you need Select-Object -Unique here? You do that on row 208.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 207 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add -Process before the script block. Also, could we add the contents in the script block onto a new row, indented 4 spaces and the closing brace on yet another row ? To shortening the row a bit.
Do you need Select-Object -Unique here? You do that on row 208.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/en-US/about_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.help.txt, line 39 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

PSDscRunAsAccount

Should be 'PsDscRunAsCredential'. In two locations.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/en-US/about_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.help.txt, line 43 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Need a new line at the end of the file.

Done.

Examples/Resources/xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/2-RemoveDatabasesFromAvailabilityGroup.ps1, line 100 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I think you could remove all the resource to create the AlwaysOn replicas here. If you should remove databases, then yu probably already have done that, and you have that example in the first example.

When I removed the other resources, the AppVeyor tests broke.

Tests/Unit/xSQLServerHelper.Tests.ps1, line 883 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Maybe 'Should return the actual server object...'?

Done.

Comments from Reviewable

Review status: 6 of 17 files reviewed at latest revision, 53 unresolved discussions.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 137 at r8 (raw file):

$database

Could we change this to $databaseObject? It's used far below and when I review that row I did not know where it came from or what it contained.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 213 at r8 (raw file):

                foreach ( $availabilityGroupReplica in $secondaryReplicas )
                {
                    $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name

Always the default instance?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 233 at r8 (raw file):

                }

                if ( $availabilityReplicaMissingDirectories.Count -gt 0 )

Can't you do this in the if-block above (if ($missingDirectories.Count -gt 0)? Then you don't need the variable $availabilityReplicaMissingDirectories at all (?).

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 250 at r8 (raw file):

                    foreach ( $availabilityGroupReplica in $secondaryReplicas )
                    {
                        $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name

Always the default instance?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 251 at r8 (raw file):

[array]

We tend to not use small letter in variable names, and also prefix them with the namespace, like [System.String]. Should we do the same in classes? Should we use the same guideline? Asking your opinion, because this is PowerShell classes, and might be considered different. We have no guidelines specific for classes (yet).
I seen it before, like in the parameters but skipped past that, but when I saw this I thought I raise the question.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 251 at r8 (raw file):

                    {
                        $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name
                        [array]$installedCertificateThumbprints = $currentAvailabilityGroupReplicaServerObject.Databases['master'].Certificates | ForEach-Object { [System.BitConverter]::ToString($_.Thumbprint) }

Please add -Process before the script block. And also split the contents of the script block to a new row.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 263 at r8 (raw file):

                        foreach ( $availabilityReplicaMissingCertificate in $availabilityReplicaMissingCertificates.GetEnumerator() )
                        {
                            $prerequisiteCheckFailures += "The instance '$($availabilityReplicaMissingCertificate.Key)' is missing the following certificates: $($availabilityReplicaMissingCertificate.Value)"

Couldn't you add this line in the if-block above instead?  In the if-blockif ( $installedCertificateThumbprints -notcontains $databaseCertificateThumbprint )
Then you don't need the variable $availabilityReplicaMissingCertificates at all (?).

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 273 at r8 (raw file):

                    $databaseLogBackupFile = Join-Path -Path $this.BackupPath -ChildPath "$($database.Name)_Log_$(Get-Date -Format 'yyyyMMddhhmmss').trn"

                    $backupSqlDatabaseParams = @{

Could you add a comment  here that this is doing either a full backup or a copy only backup.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 273 at r8 (raw file):

$backupSqlDatabaseParams

Could we rename this $backupSqlDatabaseParameters? Also any other "...Params' if there are more occurrences :)

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 293 at r8 (raw file):

                    {
                        # Log the failure
                        $databasesToAddFailures.Add($databaseName, $_.Exception)

Will it say in the error message that it was a Full-backup, CopyOnly-backup or transactional-backup that failed?  Just if there is a possibility to tell the different for debugging,

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 299 at r8 (raw file):

                    }

                    $backupSqlDatabaseLogParams = @{

Could you add a comment  here that this is doing a transactional log backup

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 313 at r8 (raw file):

                    {
                        # Log the failure
                        $databasesToAddFailures.Add($databaseName, $_.Exception)

Will it say in the error message that it was a Full-backup, CopyOnly-backup or transactional-backup that failed? Just if there is a possibility to tell the different for debugging,

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 334 at r8 (raw file):

                    # Need to restore the database with a query in order to impersonate the correct login
                    $restoreDatabaseQueryStringBuilder = New-Object -TypeName System.Text.StringBuilder

Curious. Do you need to use a StringBuilder? Can't you just do this with semi-colon separating each command?
$restoreDatabaseQueryString  = "EXECUTE AS LOGIN = '$($database.Owner)'; " + `
                               "RESTORE DATABASE [$($databaseName)] ... and so on... ;"

StringBuilder is okay, was just curious.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 352 at r8 (raw file):

$restoreSqlDatabaseLogParams

This is not used anywhere?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 364 at r8 (raw file):

                        {
                            # Connect to the replica
                            $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name

Always the default instance?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 371 at r8 (raw file):

AG

Please write out AG' to 'Availability Group'

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 414 at r8 (raw file):

        # Combine the failures into one error message and throw it here. Doing this will allow all the databases that can be processes to be processed and will still show that applying the configuration failed
        $allFailures = $databasesToAddFailures + $databasesToRemoveFailures

See next two comments

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 416 at r8 (raw file):

        $allFailures = $databasesToAddFailures + $databasesToRemoveFailures

        if ( $allFailures.Count -gt 0 )

See next commnet, change this to check both $databasesToAddFailures and $databasesToRemoveFailures instead.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 420 at r8 (raw file):

            $newTerminatingErrorParams = @{
                ErrorType = 'AlterAvailabilityGroupDatabaseMembershipFailure'
                FormatArgs = ( $allFailures.GetEnumerator() | ForEach-Object { "The operation on the database '$( $_.Key )' failed with the following errors: $( $_.Value -join "`r`n" )" } )

This row seems unnecessary complex.
Instead of this row, could you do a foreach () for both $databasesToAddFailures and $databasesToRemoveFailures to build a string for a new variable $newTerminatingErrorFormatArgs. Instead of this row, and the row 414 above ($allFailures = $databasesToAddFailures + $databasesToRemoveFailures)
And move that inside the if-block when there are an actual error to throw.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 428 at r8 (raw file):

    }

    [bool] Test()

Mark from which to continue the review.

Comments from Reviewable

Reviewed 1 of 13 files at r4.
Review status: 6 of 17 files reviewed at latest revision, 66 unresolved discussions.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 70 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
That was my initial approach. Unfortunately what happens is it overwrites the configured properties on the object. So, after doing some digging, this is the preferred method of handling this.

Yes, I figured as much when I came down to the Test() method. It calls the Get() method, and if this was to return $this, then the parameter values would be gone.
This one looks good to me! :)

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 481 at r8 (raw file):

    }

    hidden [string[]] GetDatabasesToAddToAvailabilityGroup (

Can we get comment-based help on this help method?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 491 at r8 (raw file):

    )
    {
        if ( ( $ServerObject -eq $null ) -or ( [string]::IsNullOrEmpty($ServerObject) ) )

Isn't there a variant [ValidateNullOrEmpty()] in PowerShell classes? If so, this check should not be necessary.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 496 at r8 (raw file):

        }

        if ( ( $AvailabilityGroup -eq $null ) -or ( [string]::IsNullOrEmpty($AvailabilityGroup) ) )

Isn't there a variant [ValidateNullOrEmpty()] in PowerShell classes? If so, this check should not be necessary.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 501 at r8 (raw file):

pstypenames

Please change to PSTypeNames

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 506 at r8 (raw file):

pstypenames

Please change to PSTypeNames

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 525 at r8 (raw file):

    }

    hidden [string[]] GetDatabasesToRemoveFromAvailabilityGroup (

Can we get comment-based help on this help method?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 535 at r8 (raw file):

    )
    {
        if ( ( $ServerObject -eq $null ) -or ( [string]::IsNullOrEmpty($ServerObject) ) )

Isn't there a variant [ValidateNullOrEmpty()] in PowerShell classes? If so, this check should not be necessary.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 540 at r8 (raw file):

        }

        if ( ( $AvailabilityGroup -eq $null ) -or ( [string]::IsNullOrEmpty($AvailabilityGroup) ) )

Isn't there a variant [ValidateNullOrEmpty()] in PowerShell classes? If so, this check should not be necessary.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 545 at r8 (raw file):

pstypenames

Please change to PSTypeNames

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 550 at r8 (raw file):

pstypenames

Please change to PSTypeNames

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 573 at r8 (raw file):

    }

    hidden [string[]] GetMatchingDatabaseNames (

Can we get comment-based help on this help method?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 588 at r8 (raw file):

        foreach ( $dbName in $this.DatabaseName )
        {
            $matchingDatabaseNames += $ServerObject.Databases | Where-Object { $_.Name -like $dbName } | Select-Object -ExpandProperty Name

Please use -FilterScript on Where-Object.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 594 at r8 (raw file):

    }

    hidden [string[]] GetDatabaseNamesNotFoundOnTheInstance (

Can we get comment-based help on this help method?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 617 at r8 (raw file):

        }

        $result = $databasesNotFoundOnTheInstance.GetEnumerator() | Where-Object { $_.Value } | Select-Object -ExpandProperty Key

Please use -FilterScript on Where-Object.

Comments from Reviewable

Thanks. And back at you, nice work on this code, very well written. It is easy to review! :smile:

Reviewed 1 of 8 files at r11.
Review status: 7 of 17 files reviewed at latest revision, 56 unresolved discussions.

xSQLServerHelper.psm1, line 978 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
The comments got slightly out of line. Which function is this in reference to?

This was in reference to Get-PrimaryReplicaServerObject. It seems that could be reused in the other AlwaysOn resources? if so create an issue for that. :smile:
If the other new helper functions can be used in the other resources, then add an issue for each helper function that can be reused.

Comments from Reviewable

That'll do it for me today. I'll pick back up tomorrow.

Review status: 7 of 17 files reviewed at latest revision, 56 unresolved discussions.

xSQLServerHelper.psm1, line 978 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This was in reference to Get-PrimaryReplicaServerObject. It seems that could be reused in the other AlwaysOn resources? if so create an issue for that. :smile:
If the other new helper functions can be used in the other resources, then add an issue for each helper function that can be reused.

Created issue #631
Done.

xSQLServerHelper.psm1, line 1036 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This helper function, should/can it preferably be used by the other AlwaysOn resource? If so, can you create an issue for changing the resource to use this helper functions?

Issue #630 created
Done.

xSQLServerHelper.psm1, line 1060 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This helper function, should/can it preferably be used by the other AlwaysOn resource? If so, can you create an issue for changing the resource to use this helper functions?

Issue #632 created
Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 137 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

$database

Could we change this to $databaseObject? It's used far below and when I review that row I did not know where it came from or what it contained.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 213 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Always the default instance?

Connect-SQL accepts 'ServerName' or 'ServerName\InstanceName'  in ServerName property. It worked in testing from what I could tell.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 233 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Can't you do this in the if-block above (if ($missingDirectories.Count -gt 0)? Then you don't need the variable $availabilityReplicaMissingDirectories at all (?).

I don't want to wrap that in the foreach loop that starts on line 205 foreach ( $availabilityGroupReplica in $secondaryReplicas ). I think this will keep the  overall error cleaner.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 250 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Always the default instance?

Connect-SQL accepts 'ServerName' or 'ServerName\InstanceName'  in ServerName property. It worked in testing from what I could tell.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 251 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

[array]

We tend to not use small letter in variable names, and also prefix them with the namespace, like [System.String]. Should we do the same in classes? Should we use the same guideline? Asking your opinion, because this is PowerShell classes, and might be considered different. We have no guidelines specific for classes (yet).
I seen it before, like in the parameters but skipped past that, but when I saw this I thought I raise the question.

It's not C#, so case sensitivity shouldn't matter. I'll update as I find them.
Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 251 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add -Process before the script block. And also split the contents of the script block to a new row.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 263 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Couldn't you add this line in the if-block above instead?  In the if-blockif ( $installedCertificateThumbprints -notcontains $databaseCertificateThumbprint )
Then you don't need the variable $availabilityReplicaMissingCertificates at all (?).

Same as above. I want to build the full message outside of the foreach loop that starts on line 242 foreach ( $availabilityGroupReplica in $secondaryReplicas ).

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 273 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you add a comment  here that this is doing either a full backup or a copy only backup.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 273 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

$backupSqlDatabaseParams

Could we rename this $backupSqlDatabaseParameters? Also any other "...Params' if there are more occurrences :)

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 293 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Will it say in the error message that it was a Full-backup, CopyOnly-backup or transactional-backup that failed?  Just if there is a possibility to tell the different for debugging,

If I recall correctly, it will present the T-SQL error through if the backup fails. Those messages generally state what the backup command was.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 299 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you add a comment  here that this is doing a transactional log backup

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 313 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Will it say in the error message that it was a Full-backup, CopyOnly-backup or transactional-backup that failed? Just if there is a possibility to tell the different for debugging,

If I recall correctly, it will present the T-SQL error through if the backup fails. Those messages generally state what the backup command was.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 334 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Curious. Do you need to use a StringBuilder? Can't you just do this with semi-colon separating each command?
$restoreDatabaseQueryString  = "EXECUTE AS LOGIN = '$($database.Owner)'; " + `
                               "RESTORE DATABASE [$($databaseName)] ... and so on... ;"

StringBuilder is okay, was just curious.

I thought it was cleaner to do it this way. I prefer not to use the 'tick' at the end of the line as sometimes that is hard to read.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 352 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

$restoreSqlDatabaseLogParams

This is not used anywhere?

Used on line 366 Restore-SqlDatabase -InputObject $currentAvailabilityGroupReplicaServerObject @restoreSqlDatabaseLogParameters
Also updated to Parameters

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 364 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Always the default instance?

Connect-SQL accepts 'ServerName' or 'ServerName\InstanceName'  in ServerName property. It worked in testing from what I could tell.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 371 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

AG

Please write out AG' to 'Availability Group'

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 414 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
See next two comments

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 416 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
See next commnet, change this to check both $databasesToAddFailures and $databasesToRemoveFailures instead.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 420 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This row seems unnecessary complex.
Instead of this row, could you do a foreach () for both $databasesToAddFailures and $databasesToRemoveFailures to build a string for a new variable $newTerminatingErrorFormatArgs. Instead of this row, and the row 414 above ($allFailures = $databasesToAddFailures + $databasesToRemoveFailures)
And move that inside the if-block when there are an actual error to throw.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 481 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Can we get comment-based help on this help method?

Added the help, but it won't be presented through anywhere. The only place it can be seen is by developers looking at the code.
Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 491 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Isn't there a variant [ValidateNullOrEmpty()] in PowerShell classes? If so, this check should not be necessary.

Not that I was able to find. But I'm still new at this!

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 496 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Isn't there a variant [ValidateNullOrEmpty()] in PowerShell classes? If so, this check should not be necessary.

Not that I was able to find.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 501 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

pstypenames

Please change to PSTypeNames

Intellisense changes it to lowercase.
Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 506 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

pstypenames

Please change to PSTypeNames

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 525 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Can we get comment-based help on this help method?

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 535 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Isn't there a variant [ValidateNullOrEmpty()] in PowerShell classes? If so, this check should not be necessary.

Not that I know of

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 540 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Isn't there a variant [ValidateNullOrEmpty()] in PowerShell classes? If so, this check should not be necessary.

Not that I know of

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 545 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

pstypenames

Please change to PSTypeNames

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 550 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

pstypenames

Please change to PSTypeNames

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 573 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Can we get comment-based help on this help method?

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 588 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please use -FilterScript on Where-Object.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 594 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Can we get comment-based help on this help method?

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 617 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please use -FilterScript on Where-Object.

Done.

Comments from Reviewable

Review status: 7 of 17 files reviewed at latest revision, 58 unresolved discussions.

xSQLServer.psd1, line 37 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
That probably doesn't need to be there anymore. I will check on that.

I tested on a machine without SQL bits, and I was able to compile a MOF

Comments from Reviewable

Reviewed 1 of 5 files at r10.
Review status: 8 of 17 files reviewed at latest revision, 58 unresolved discussions.

CHANGELOG.md, line 30 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Done.

Great that you moved breaking change to the top, but now you have 'Added new resource' in two locations.  You can keep one of the entries in either location. :)

Comments from Reviewable

Review status: 7 of 17 files reviewed at latest revision, 10 unresolved discussions.

CHANGELOG.md, line 30 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Great that you moved breaking change to the top, but now you have 'Added new resource' in two locations.  You can keep one of the entries in either location. :)

The rebase got me again!
Done.

Comments from Reviewable

Reviewed 1 of 8 files at r6, 1 of 5 files at r10, 6 of 8 files at r11, 1 of 1 files at r12.
Review status: 16 of 17 files reviewed at latest revision, 56 unresolved discussions.

README.md, line 173 at r12 (raw file):

* **[String[]] DatabaseName** _(Write)_: The name of the database(s) that should be members of the availability group. The availability group member database(s) are added and removed until they match the defined databases. This accepts wildcards.
* **[String[]] DatabaseNameToInclude** _(Write)_: The name of the database(s) that should be added to the availability group. The defined database(s) are added to the availability group. All other databases that may be a member of the availability group are ignored. This accepts wildcards.

Please add something like (after looking at the code); 'This parameter can be used at the same time as the parameter DatabaseName.'

README.md, line 174 at r12 (raw file):

* **[String[]] DatabaseName** _(Write)_: The name of the database(s) that should be members of the availability group. The availability group member database(s) are added and removed until they match the defined databases. This accepts wildcards.
* **[String[]] DatabaseNameToInclude** _(Write)_: The name of the database(s) that should be added to the availability group. The defined database(s) are added to the availability group. All other databases that may be a member of the availability group are ignored. This accepts wildcards.
* **[String[]] DatabaseNameToExclude** _(Write)_: The name of the database(s) that should be removed from the availability group. The defined database(s) are removed to the availability group. All other databases that may be a member of the availability group are ignored. This accepts wildcards.

Please add something like (after looking at the code); 'If parameter DatabaseName is assign a value, then this parameter will be ignored.'

xSQLServer.psd1, line 37 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
I tested on a machine without SQL bits, and I was able to compile a MOF

You mean they are required to be here, and you were able to compile a mof on a node not having the SQL bits installed? You could compile a mof, but did it work to install SQL on the target node?
So another question then, if deploying a SQL Server, configure AlwaysOn and adding databases in one go, will this resource break because the module did not load the assemblies at the start when there were no SQL bits installed? :)

xSQLServerHelper.psm1, line 1060 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Issue #632 created
Done.

This Test-ImpersonatePermissions did not get an issue. Issue 632 is another issue. :)

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 178 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Connect-SQL accepts both 'ServerName' and 'ServerName\InstanceName' in the 'SQLServer' property. It worked in testing from what I can tell.

Yes it seems it does work, Connect-SQL sets the default value 'MSSQLSERVER' to instance name if it is not provided, so it then checks if SQLInstanceName is 'MSSQLSERVER' and sets $connectSql = $SQLServer which contains 'ServerName\InstanceName'.
https://github.com/PowerShell/xSQLServer/blob/dev/xSQLServerHelper.psm1#L39-L46
My point is, if that logic changes, that will break this resource, because any changes will be expecting that instance name goes into SQLInstanceName parameter, not SQLServer parameter. So I would rather that you explicitly set the correct instance name if it is an instance in the call to Connect-SQL.
Also, it will be much more clear to the next coder.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 194 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Connect-SQL accepts both 'ServerName' and 'ServerName\InstanceName' in the 'SQLServer' property. It worked in testing from what I can tell.

See comment above.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 213 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Connect-SQL accepts 'ServerName' or 'ServerName\InstanceName'  in ServerName property. It worked in testing from what I could tell.

See comment above.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 250 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Connect-SQL accepts 'ServerName' or 'ServerName\InstanceName'  in ServerName property. It worked in testing from what I could tell.

See comment above.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 364 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Connect-SQL accepts 'ServerName' or 'ServerName\InstanceName'  in ServerName property. It worked in testing from what I could tell.

See comment above.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 491 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Not that I was able to find. But I'm still new at this!

Me too. Curious, what happens if you just stick [ValidateNullOrEmpty()] to the parameter declaration? It is PowerShell after all, shuld hopefully use the same things ;)

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 617 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Done.

You missed this one. :)

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 176 at r12 (raw file):

                    if ( $availabilityReplicaFilestreamLevel.Values -contains 'Disabled' )
                    {
                        $availabilityReplicaFilestreamLevelDisabled = $availabilityReplicaFilestreamLevel.GetEnumerator() | Where-Object { $_.Value -eq 'Disabled' } | Select-Object -ExpandProperty Key

Please add -FilterScript to Where-Object

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 510 at r12 (raw file):

        # Using psobject here rather than [Microsoft.SqlServer.Management.Smo.AvailabilityGroup] so that Get-DSCResource will work properly
        [psobject]
        $AvailabilityGroup

Why do you not reference this by $this.AvailabilityGroup instead?

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 595 at r12 (raw file):

AG

Please change 'AG' to 'Availability Group'.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 605 at r12 (raw file):

AG

Please change 'AG' to 'Availability Group'.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/en-US/about_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.help.txt, line 9 at r12 (raw file):

    accepts wildcards.

PARAMETER DatabaseNameToInclude

The review comments for the descriptions in the README.md should be added here as well.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 363 at r12 (raw file):

            Context 'When the Get method is called' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 383 at r12 (raw file):

                It 'Should not return any databases if there are no databases in the availability group' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 401 at r12 (raw file):

                It 'Should return databases when there are databases in the availability group' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 428 at r12 (raw file):

        Describe 'xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership\Set()' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 442 at r12 (raw file):

            BeforeEach {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 467 at r12 (raw file):

            Context 'When the DatabaseNameToInclude property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 473 at r12 (raw file):

                It 'Should add the specified databases to the availability group' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 500 at r12 (raw file):

                It 'Should add the specified databases to the availability group when the primary replica is on another server' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 529 at r12 (raw file):

                It 'Should not do anything if no databases were found to add' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 558 at r12 (raw file):

                It 'Should add the specified databases to the availability group when "MatchDatabaseOwner" is $false' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 586 at r12 (raw file):

                }

                It 'Should throw the correct error "ImpersonatePermissionsMissing" when "MatchDatabaseOwner" is $true and the current login does not have impersonate permissions' {

A tip. You can use more than one Context-block if you like (in Pester 4.x), to shorten the It-block descriptions. But the way you have done it is okay too! :smile:
Context 'When the DatabaseNameToInclude property is set' {
    Context 'When "MatchDatabaseOwner" is $true and the current login does not have impersonate permissions' {
        It 'Should throw the correct error "ImpersonatePermissionsMissing" ' {
        }
    }
}

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 587 at r12 (raw file):

                It 'Should throw the correct error "ImpersonatePermissionsMissing" when "MatchDatabaseOwner" is $true and the current login does not have impersonate permissions' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 672 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when the database property "ID" is less than "4"' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 742 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when the database property "ContainmentType" is not "Partial"' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 774 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when the database file path does not exist on the secondary replica' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 807 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when the log file path does not exist on the secondary replica' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 840 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when TDE is enabled on the database but the certificate is not present on the replica instances' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 875 at r12 (raw file):

                It 'Should add the specified databases to the availability group when the database has not been previously backed up' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 904 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when it fails to perform a full backup' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 933 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when it fails to perform a log backup' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 962 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when it fails to add the database to the primary replica' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 991 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when it fails to add the database to the primary replica' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1021 at r12 (raw file):

            Context 'When the DatabaseNameToExclude property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1027 at r12 (raw file):

                It 'Should remove the specified databases to the availability group(s)' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1054 at r12 (raw file):

                It 'Should remove the specified databases to the availability group(s) when the primary replica is on another server' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1083 at r12 (raw file):

                It 'Should throw the correct error "AlterAvailabilityGroupDatabaseMembershipFailure" when it fails to remove the database from the availability group' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1113 at r12 (raw file):

            Context 'When the DatabaseName property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1165 at r12 (raw file):

            Context 'When the DatabaseNameToInclude property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1210 at r12 (raw file):

            Context 'When the DatabaseNameToExclude property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1254 at r12 (raw file):

            Context 'When the DatabaseName property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1320 at r12 (raw file):

            Context 'When invalid objects are passed to the method' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1349 at r12 (raw file):

            Context 'When the DatabaseNameToInclude property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1372 at r12 (raw file):

            Context 'When the DatabaseName property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1413 at r12 (raw file):

            Context 'When invalid objects are passed to the method' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1442 at r12 (raw file):

            Context 'When the DatabaseNameToExclude property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1471 at r12 (raw file):

            Context 'When the DatabaseName property is set' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1487 at r12 (raw file):

        Describe 'xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership\GetDatabaseNamesNotFoundOnTheInstance()' {

Please remove blank row

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1489 at r12 (raw file):

            Context 'When the GetDatabaseNamesNotFoundOnTheInstance method is called' {

Please remove blank row

Comments from Reviewable

@johlju, I realized after reading some the updated reviewable items that switching to the three DatabaseName arrays resulted in some undesired results. The biggest issue is that I am unable to enforce uniqueness at compile time.
Here's what I'm proposing.

Bring back the "Ensure" property with the standard values of "Absent" and "Present"
Add a "Force" property that is a switch. This will be used in conjunction with Ensure = 'Present'

This will allow me to perform validation and ensure the end user follows supported methods at compile time.
What do you think?  :/ What do you mean by uniqueness during compile time? You already say that instance name and Availability Group is unique. Do you have an example of the problem, and how Ensure and Force would solve that?
If you see that the only way to solve it is to do it according to what you suggest, then I have no problem with it.

In order to ensure that only one of the three 'DatabaseName' properties (DatabaseName, DatabaseNameToInclude, DatabaseNameToExclude) is set, I would have to write a method to have it checked in the get/set/test methods on runtime. This could result in some frustration that the resource compiles properly with the specified values, but then errors at run time.
I talked with some other folks about this issue, so I'll move forward with the Force option.
I can't wait until DSC supports parameter sets! This would be a non-issue if those existed.

DatabaseNameToInclude and DatabaseNameToExclude could be used at the same time if we wanted to. But they should not be used at the same time as DatabaseName.
Most resources throws an error message if the parameters are not correctly used, so this could be considered normal behaviour (until we have better functionality), and would only happen if one did not follow the documentation. ;)
I fail to se how a Force parameter make this different? Could you elaborate? 😄
I will wait for your next push :)

You're correct...using Force wouldn't throw an error at compilation. It will force the user into the desired code path so it wouldn't ever need to error on compilation. I'm probably not doing a great job of explaining myself, so I'll let the code speak for me.
I thought about using both the Include and Exclude parameters, but using force aligns more with how the user rights assignment resource works.

Okay :) Let see what you come up with, maybe it's a good approach that should be used in more places :smile:
Great work! :)

Ok, got everything done (I think) except splitting the server/instance name whenever I call Connect-SQL.

Review status: 7 of 17 files reviewed at latest revision, 56 unresolved discussions.

README.md, line 173 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add something like (after looking at the code); 'This parameter can be used at the same time as the parameter DatabaseName.'

No longer relevant.
Done.

README.md, line 174 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add something like (after looking at the code); 'If parameter DatabaseName is assign a value, then this parameter will be ignored.'

No longer relevant.
Done.

xSQLServer.psd1, line 37 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You mean they are required to be here, and you were able to compile a mof on a node not having the SQL bits installed? You could compile a mof, but did it work to install SQL on the target node?
So another question then, if deploying a SQL Server, configure AlwaysOn and adding databases in one go, will this resource break because the module did not load the assemblies at the start when there were no SQL bits installed? :)

I tested compiling a mof on a computer where the SQL bits were not installed. I didn't think through the rest of that!
I need to test if I can remove this line. I'm pretty sure it can be removed.

xSQLServerHelper.psm1, line 1060 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This Test-ImpersonatePermissions did not get an issue. Issue 632 is another issue. :)

Well....that's odd.
Issue #637 created.
Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 491 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Me too. Curious, what happens if you just stick [ValidateNullOrEmpty()] to the parameter declaration? It is PowerShell after all, shuld hopefully use the same things ;)

[PowerShell] Cannot find the type for custom attribute 'ValidateNullOrEmpty'. Make sure that the assembly that contains this type is loaded.
Not the same as passing a parameter into a function :). More along the lines of C# here.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 617 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You missed this one. :)

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 176 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add -FilterScript to Where-Object

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 510 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Why do you not reference this by $this.AvailabilityGroup instead?

AvailabilityGroup is not a property on the class. The availability group object could also change depending which instance DSC connects to.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 595 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

AG

Please change 'AG' to 'Availability Group'.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 605 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

AG

Please change 'AG' to 'Availability Group'.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/en-US/about_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.help.txt, line 9 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
The review comments for the descriptions in the README.md should be added here as well.

No longer relevant.
Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 363 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 383 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 401 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 428 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 442 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 467 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 473 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 500 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 529 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 558 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 586 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
A tip. You can use more than one Context-block if you like (in Pester 4.x), to shorten the It-block descriptions. But the way you have done it is okay too! :smile:
Context 'When the DatabaseNameToInclude property is set' {
    Context 'When "MatchDatabaseOwner" is $true and the current login does not have impersonate permissions' {
        It 'Should throw the correct error "ImpersonatePermissionsMissing" ' {
        }
    }
}

Thanks! I'll be sure to use that in the future.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 587 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 672 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 742 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 774 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 807 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 840 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 875 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 904 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 933 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 962 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 991 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1021 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1027 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1054 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1083 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1113 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1165 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1210 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1254 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1320 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1349 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1372 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1413 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1442 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1471 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1487 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.Tests.ps1, line 1489 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove blank row

Done.

Comments from Reviewable

Looks like I lost some changes when I did this. I'll work on re-adding them.

Review status: 7 of 17 files reviewed at latest revision, 56 unresolved discussions.

Comments from Reviewable

Review status: 7 of 17 files reviewed at latest revision, 56 unresolved discussions.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 178 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Yes it seems it does work, Connect-SQL sets the default value 'MSSQLSERVER' to instance name if it is not provided, so it then checks if SQLInstanceName is 'MSSQLSERVER' and sets $connectSql = $SQLServer which contains 'ServerName\InstanceName'.
https://github.com/PowerShell/xSQLServer/blob/dev/xSQLServerHelper.psm1#L39-L46
My point is, if that logic changes, that will break this resource, because any changes will be expecting that instance name goes into SQLInstanceName parameter, not SQLServer parameter. So I would rather that you explicitly set the correct instance name if it is an instance in the call to Connect-SQL.
Also, it will be much more clear to the next coder.

I'm with ya!
Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 194 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
See comment above.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 213 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
See comment above.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 250 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
See comment above.

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 364 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
See comment above.

Done.

Comments from Reviewable

I will review tomorrow :) It's very late here now :)

Review status: 6 of 17 files reviewed at latest revision, 56 unresolved discussions.

xSQLServer.psd1, line 37 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
I tested compiling a mof on a computer where the SQL bits were not installed. I didn't think through the rest of that!
I need to test if I can remove this line. I'm pretty sure it can be removed.

Removed the required assemblies.

Comments from Reviewable

Just a few small comments :)

Reviewed 5 of 10 files at r13, 2 of 4 files at r14, 4 of 4 files at r15.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

xSQLServerHelper.psm1, line 1036 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Issue #630 created
Done.

Can acknowledge this for some reason :/ Can you try writing Done again?

xSQLServerHelper.psm1, line 1000 at r15 (raw file):

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]

Change to [System.String]. Please do search and replace for this. This was removed when you reverted to an older commit :)

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 491 at r8 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
[PowerShell] Cannot find the type for custom attribute 'ValidateNullOrEmpty'. Make sure that the assembly that contains this type is loaded.
Not the same as passing a parameter into a function :). More along the lines of C# here.

Ah... To bad ;) Yes it more like coding C#. :) Too bad there is no equivalent to [ValidateNullOrEmpty()] (as far as we know at least) :smile:
All good like it is.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 176 at r12 (raw file):

$availabilityReplicaFilestreamLevelDisabled = $availabilityReplicaFilestreamLevel.GetEnumerator() | Where-Object -FilterScript { $_.Value -eq 'Disabled' } | Select-Object -ExpandProperty Key

$prerequisiteCheckFailures += ( 'Filestream is disabled on the following instances: {0}' -f ( $availabilityReplicaFilestreamLevelDisabled -join ', ' ) )

This code was lost I think. Only show those that actually failed.

Comments from Reviewable

Got 'em. :smile:

Review status: 15 of 17 files reviewed at latest revision, 2 unresolved discussions.

xSQLServerHelper.psm1, line 1036 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Can acknowledge this for some reason :/ Can you try writing Done again?

Done.

xSQLServerHelper.psm1, line 1000 at r15 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Change to [System.String]. Please do search and replace for this. This was removed when you reverted to an older commit :)

Done.

DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 176 at r12 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

$availabilityReplicaFilestreamLevelDisabled = $availabilityReplicaFilestreamLevel.GetEnumerator() | Where-Object -FilterScript { $_.Value -eq 'Disabled' } | Select-Object -ExpandProperty Key

$prerequisiteCheckFailures += ( 'Filestream is disabled on the following instances: {0}' -f ( $availabilityReplicaFilestreamLevelDisabled -join ', ' ) )

This code was lost I think. Only show those that actually failed.

Done.

Comments from Reviewable

This looks good to me know. Personally I would have liked to kept the parameters DatabaseName, DatabaseNameToInclude and DatabaseNameToExclude instead of using the Force parameter. I think the Force parameter should be used to Force in a change, like force overwrite a file, which in this case could be argued that it does, it force to overwrite all databases in the Availability Group. :smile:
I will ask Katie to glance over this to see if there anything else that might be missed or anything else that need to be changed since this is the first class-based resource.

Reviewed 2 of 2 files at r16.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@kwirkykat or @mbreakey3 Could you help me with the "classes side" of things in this one? This is the first class-based resource so I just want another to glance thru this to see if everything is in place and all looks in general okay. Is something wrong or is something missing?

Yeah, I really struggled with how to quickly indicate to the end user that use cases of the resource. My intention is to have three options:

Add the specified databases to the availability group.
Remove the specified databases from the availability group.
Add and remove databases to/from the availability group until the only members of the availability group are the databases specified.

The first two options fit neatly unto the buckets of Ensure = Absent or Present. However, this falls apart when option 3 is introduced.
I don't like using multiple arrays to define the databases because it implies that one or more of the arrays can be used in tandem, especially when adding the in the 3rd option. I think if we only had options 1 and 2, it would be more realistic, but I'm still not a fan.
The other thing that could make using multiple arrays more palatable is parameter groups (overloads in class world). This way the end user could see the error when the MOF is compiled rather than when the MOF is applied if they attempt to define more than one of the arrays.
It would be nice if we could add a 3rd property for Ensure (maybe "Exactly") to indicate the resource will be applied exactly as configured rather than the "fuzzy" logic of adding/removing only the defined objects.
No matter what, I think it'll be an imperfect solution. Thanks for the feedback!

In regards to 'Exactly'. I understand what you mean. But, if you would imagine that this resource also created the Availability Group (like xSQLServerRole, or Group in PSDscResources, which uses the three arrays) then Ensure is used to either create ('Present') or delete ('Absent') the Availability Group, the three arrays would then be used to add members. It that scenario 'Exactly' would be very confusing. Would 'Exactly' mean create or delete. In that sense 'EnsureExactly' would be better.
But I know the discussion was up changing Ensure adding more options, but it was a design decision that Ensure should only contain 'Present' and 'Absent'. This was a while back, can't find that discussion.
Thanks for the discussion! 😄

@randomnote1 If we don't hear anything from Mariah or Katie during this week (they are quite busy), then I will merge this one on Saturday.

@randomnote1 Could you maybe rebase this one, and resolve the merge conflicts before Saturday?

Reviewed 4 of 5 files at r17, 1 of 1 files at r18.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Reviewed 1 of 1 files at r19.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable


b7259878-436c-4274-bfff-ca76f4cb7892 not showing species, organ/part, disease in the data browser

Description of the issue
The project Fate-mapping within human iPSC-derived kidney organoids reveals conserved mammalian nephron progenitor lineage relationships. (b7259878-436c-4274-bfff-ca76f4cb7892) is not showing species, organ/part, disease in the data browser.
Ingest
Fate-mapping within human iPSC-derived kidney organoids reveals conserved mammalian nephron progenitor lineage relationships.
Data browser
Fate-mapping within human iPSC-derived kidney organoids reveals conserved mammalian nephron progenitor lineage relationships.
Primary wrangler
@ami-day
Other useful information

Conversation here

I need to look into Hannes response about this

Latest update: no species or disease status https://data.humancellatlas.org/explore/projects/b7259878-436c-4274-bfff-ca76f4cb7892

Added a collection protocol, donor and skin tissue specimen for the human fibroblast cell line, and linked them together in 2 new processes. Re-graph validating.

Re-submitted.

@ami-day is this Done? Last comment says you re-submitted but the ticket is still In Progress

I need to update the normal ontology label and then re-submit

Graph validating

This project is stuck in "graph validation requested" status.

Graph valid

Re-opening the ticket as from what I can see Ami never exported the update after Alexie helped to unblock it. It should be ready for a wrangler to just push the export I guess. Moving to the To Do list

Just submitted the metadata update in Ingest. Will complete the import form once the submission is Exported

The submission has been stuck in "Exporting" for the last 24h. No idea if that's to be expected or not. Will check in Stand-Up

@MightyAx just sorted the issues on https://app.zenhub.com/workspaces/operations-5fa2d8f2df78bb000f7fb2b5/issues/ebi-ait/hca-ebi-wrangler-central/985
Project updates have now been exported. I will fill the Import form and add to wrangling board as well to track that we check the Updates on Release 23

Import form submitted

Looks good in the browser. So closing it


Please mark angel as discontinued

In short - angel is a deprecated entry point, and its repo is now just used to hold application boilerplates, and the project overview. It will not be updated further.
The URL is: https://pub.dartlang.org/packages/angel
Thanks!

Done,