SqlServerDsc: Resources work with automatic seeding

[Sidoran]

Pull Request (PR) description

• SqlAg
  • Added optional parameter SeedingMode that will set the SeedingMode for the SQL Server 2016 and higher. This parameter can only be used together with the module SqlServer installed (tested v21.0.17099). The parameter will be ignored if SQLPS module will be used.
• SqlAgReplica
  • Added optional parameter SeedingMode that will set the SeedingMode for the SQL Server 2016 and higher. This parameter can only be used together with the module SqlServer installed (tested v21.0.17099). The parameter will be ignored if SQLPS module will be used.

This Pull Request (PR) fixes the following issues

• Fixes #487

Task list

• [x] Added an entry to the change log under the Unreleased section of the file CHANGELOG.md. Entry should say what was changed and how that affects users (if applicable), and reference the issue being resolved (if applicable).
• [ ] Resource documentation updated in the resource's README.md.
• [x] Resource parameter descriptions updated in schema.mof.
• [x] Comment-based help updated, including parameter descriptions.
• [ ] Localization strings updated.
• [ ] Examples updated.
• [x] Unit tests updated. See DSC Community Testing Guidelines.
• [ ] Integration tests updated (where possible). See DSC Community Testing Guidelines.
• [x] Code changes adheres to DSC Community Style Guidelines.

---

This change is 

[codecov[bot]]

Codecov Report

Merging #1915 (b78a8b2) into main (be2f562) will decrease coverage by 1%. The diff coverage is 55%.

@@         Coverage Diff          @@
##           main   #1915   +/-   ##
====================================
- Coverage    92%     91%   -1%     
====================================
  Files        91      91           
  Lines      7761    7788   +27     
====================================
+ Hits       7149    7164   +15     
- Misses      612     624   +12     

Flag	Coverage Δ
unit	91% <55%> (-1%)	:arrow_down:

Impacted Files	Coverage Δ
source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1	0% <0%> (ø)
...SCResources/DSC_SqlAGReplica/DSC_SqlAGReplica.psm1	99% <93%> (-1%)	:arrow_down:

[Sidoran]

Hi @johlju, can you please look at PR? Can you please also rerun the verification pipeline because two agents stopped to respond, and the whole pipeline is marked as failed?

[Sidoran]

@johlju, I made the changes according to your comments. The only thing confusing me is the code responsible for the Get, Update (inside Set), and Test using Microsoft.SqlServer.Management.Smo.Server object which supports SeedingMode for all SQL 2016 and newer. The only issue I have is the New-SQLAvailabilityReplica command in SQLPS module does not support SeedingMode parameters. And that's why I have to use the if ( (Get-Command -Name 'New-SqlAvailabilityReplica').Parameters.ContainsKey('SeedingMode') ) in all places. If you will have some idea how to get rid of this check I would be happy to rewrite this part of the code.

[johlju]

If you will have some idea how to get rid of this check I would be happy to rewrite this part of the code.

Only way is to connect to the replica (e.g. Connect-SqlDscDatabaseEngine to get the Server object. The set the SeedingMode property on the right object. But that might be a bigger (too big?) change?

[Sidoran]

Previously, johlju (Johan Ljunggren) wrote…

> > _[Reviewable](https://reviewable.io/reviews/dsccommunity/SqlServerDsc/1915)_ status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @Sidoran) > > _[`source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1` line 359 at r5](https://reviewable.io/reviews/dsccommunity/SqlServerDsc/1915#-NU1t98r3KLNkNzP-D2k:-NU1t98s1eSkCjWBeXwV:b-thyll) ([raw file](https://github.com/dsccommunity/SqlServerDsc/blob/57012763e6e84677b456becaf6dde582f9dda736/source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1#L359)):_ > > > ```powershell > > } > > > > if ( ( $sqlMajorVersion -ge 13 ) -and (Get-Command -Name 'New-SqlAvailabilityReplica').Parameters.ContainsKey('SeedingMode') ) > > ``` > > Instead of having this check, I thing we should have this check one time at the beginning of the Set-method. If the user passes SeedingMode and version is lower than 13 then it should throw an exception saying that "using SeedingMode for verison lower than 13 is not supported". > > _Code quote:_ > > ```powershell > $sqlMajorVersion -ge 13 > ```

The Major version check at the beginning will break the current behavior for the BasicAvailabilityGroup, DatabaseHealthTrigger and DtcSupportEnabled availability group options.

Previously, johlju (Johan Ljunggren) wrote…

> Is it possible for you to add unit tests to cover the changed (added) code paths?

I've already made some unit test modifications for the SqlAGReplica resource. The SqlAg, as I see, is uncovered by pester because of migration to Pester 5.x

Previously, johlju (Johan Ljunggren) wrote…

> Only way is to connect to the replica (e.g. `Connect-SqlDscDatabaseEngine` to get the `Server` object. The set the `SeedingMode` property on the right object. But that might be a bigger (too big?) change?

Sorry, I didn't get your Idea. The Connect-SqlDscDatabaseEngine will work with the SeedingMode because it uses the Connect-SQL, which uses the SMO object. IMHO, the problem is in the New-SqlAvailabilityReplica command that has the SeedingMode parameter in the SQLServer module and doesn't have it in the case of SQLPS(which is the default).

[johlju]

The Major version check at the beginning will break the current behavior for the BasicAvailabilityGroup, DatabaseHealthTrigger and DtcSupportEnabled availability group options.

I haven’t look at the code so not sure, but I meant: “if major version is less than 13 and PSBoundParameters contain SeedingMode then throw an exception telling the user to not specify SeedingMode as it is not supported”. I might be wrong but I thought you had that check because it was supported in lower major versions?

[johlju]

Sorry, I didn't get your Idea. The Connect-SqlDscDatabaseEngine will work with the SeedingMode because it uses the Connect-SQL, which uses the SMO object. IMHO, the problem is in the New-SqlAvailabilityReplica command that has the SeedingMode parameter in the SQLServer module and doesn't have it in the case of SQLPS(which is the default).

I meant set all the settings using the SMO objects directly (if possible) and not use any commands from either module. It was the only way I could see around that the Microsoft SQL Server Team has not added the parameter to the command in the module SQLPS.

But I’m good with the current way of checking for the parameter you added. Though maybe we should have outputted a warning message telling the user that they should use the module SqlServer if they pass the parameter SeedingMode. 🤔

[johlju]

I've already made some unit test modifications for the SqlAGReplica resource. The SqlAg, as I see, is uncovered by pester because of migration to Pester 5.x

Yes, no need to add unit tests for SqlAg if it hasn’t been converted. It probably need to be heavily refactored then anyway.

[Sidoran]

I haven’t look at the code so not sure, but I meant: “if major version is less than 13 and PSBoundParameters contain SeedingMode then throw an exception telling the user to not specify SeedingMode as it is not supported”. I might be wrong but I thought you had that check because it was supported in lower major versions?

Auto seeding was introduced in SQL 2016. When I did changes, I was based on existing behavior in a code, as you may see here https://github.com/dsccommunity/SqlServerDsc/blob/a86039c2ae0bdde6b7e479d529918adc11f3296c/source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1#L367-L372. As far as I understand, this was a conscious decision not to throw an exception in case you try to use a property from SQL 2016 in an older version.

[johlju]

When I did changes, I was based on existing behavior in a code

Ah, I see. Then we continue on the same path.

[johlju]

@Sidoran this is ready to merge but got an issue with the pipeline unrelated to this PR. Trying to fix it in PR #1926 so I can merge this.