Added New parameter ProductCoveredbySA that is introduced in SQL 2022

sara921-spec commented 2 months ago

This Pull Request (PR) fixes the following issues

Pull Request (PR) description

'ProductCoveredbySA'
- Added new parameter ProductCoveredbySA which is introduced in SQL 2022.
Adding parameter ProductCoveredbySA (introduced in sql 2022) into DSC_SqlSetup Specifies the license coverage for SQL Server. True indicates it's covered under Software Assurance or SQL Server subscription. False, or omitting the parameter, indicates it's covered under a SQL Server license. Default value is False.

This Pull Request (PR) fixes the following issues

Fixes #2040

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.
[x] Examples updated.
[x] Unit tests updated. See DSC Community Testing Guidelines.
[x] Integration tests updated (where possible). See DSC Community Testing Guidelines.
[x] Code changes adheres to DSC Community Style Guidelines.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94%. Comparing base (3c30929) to head (7de6c99). Report is 1 commits behind head on main.

Additional details and impacted files

[![Impacted file tree graph](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2044/graphs/tree.svg?width=650&height=150&src=pr&token=2L5l2Zcoqd&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity)](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) ```diff @@ Coverage Diff @@ ## main #2044 +/- ## ==================================== Coverage 94% 94% ==================================== Files 94 94 Lines 7922 7930 +8 ==================================== + Hits 7492 7500 +8 Misses 430 430 ``` | [Flag](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2044/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2044/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | `94% <100%> (+<1%)` | :arrow_up: | | [Files with missing lines](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2044?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | Coverage Δ | | |---|---|---| | [source/DSCResources/DSC\_SqlSetup/DSC\_SqlSetup.psm1](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2044?src=pr&el=tree&filepath=source%2FDSCResources%2FDSC_SqlSetup%2FDSC_SqlSetup.psm1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#diff-c291cmNlL0RTQ1Jlc291cmNlcy9EU0NfU3FsU2V0dXAvRFNDX1NxbFNldHVwLnBzbTE=) | `98% <100%> (+<1%)` | :arrow_up: |

johlju commented 2 months ago

Happy to take a look, but might take a few days before I have time. 😊

johlju commented 2 months ago

If the above command (in the review comment) is able to get the value, then it will should resolve the error in the tests. If the command does not work, then you need to handle the error being thrown another way. 🙂

sara921-spec commented 2 months ago

If the above command (in the review comment) is able to get the value, then it will should resolve the error in the tests. If the command does not work, then you need to handle the error being thrown another way. 🙂

@johlju This update to use "Get-RegistryPropertyValue" as suggested worked to pass the HKLM path errors, we have only 2 check failures now.

error - 'Conn' is not a valid value for setting 'FEATURES'.

johlju commented 2 months ago

The Feature CONN and some other features are not available in SQL Server 2022. I see you have enabled SQL Server 2022 (major version '16') for a bunch of tests which is good because that is really needed, but that is why it is failing. You need to make sure the correct values are passed to -TargetResource and then evaluated the correct values where returned when the major version is 16. For example not calling the -TargetResource with 'CONN' features parameter.

sara921-spec commented 2 months ago

Hi @johlju What are the available features for 2022 or how to get that? any idea?

https://github.com/dsccommunity/SqlServerDsc/issues/1566

Seems "RS" is not valid too

Error: Context When installing a default instance for major version 16 Context When installing all features

[error] [-] Should set the system in the desired state when feature is SQLENGINE 59ms (54ms|5ms)

[error] Expected no exception to be thrown, but an exception "System.InvalidOperationException: 'Rs' is not a valid value for setting 'FEATURES'. Refer to SQL Help for more information." was thrown from D:\a\1\s\output\builtModule\SqlServerDsc\17.0.0\Modules\DscResource.Common\0.17.2\DscResource.Common.psm1:3755 char:9

johlju commented 2 months ago

Hi @johlju What are the available features for 2022 or how to get that? any idea?

Can be determined here: https://learn.microsoft.com/en-us/sql/database-engine/install-windows/install-sql-server-from-the-command-prompt?view=sql-server-ver16#Feature

sara921-spec commented 2 months ago

Can be determined here: https://learn.microsoft.com/en-us/sql/database-engine/install-windows/install-sql-server-from-the-command-prompt?view=sql-server-ver16#Feature

hmm, RS is a valid feature for 2022

johlju commented 2 months ago

It says in the Description-field "Applies to: SQL Server 2016 (13.x) and earlier versions". So not part of 2022 unfortunately.

sara921-spec commented 2 months ago

Hi @johlju can U review this when u get a chance and let me know!

sara921-spec commented 2 months ago

Hi @johlju can U review this when u get a chance and let me know!

@johlju ? Just following up, if u have any thoughts on this PR? We are looking to automate the SQL 2022 installs and this parameter is something we need to make our installs configured properly with selecting the correct license

johlju commented 2 months ago

I will get back to this as soon as I have time. It is on my todo list but have had less time recently to do community work due to regular work and family obligations. 😊

dan-hughes commented 2 months ago

@sara921-spec, please can you add an example and an integration test.

sara921-spec commented 2 months ago

@sara921-spec, please can you add an example and an integration test.

Does this needs an example and an integration test? As this is not a new resource and only a setting (I was comparing the existing examples and could not figure out how to add one for this yet, so just wanted to check if I make sense or it is something mandatory) (new here😊)

dan-hughes commented 2 months ago

@sara921-spec, please can you add an example and an integration test.

Does this needs an example and an integration test? As this is not a new resource and only a setting (I was comparing the existing examples and could not figure out how to add one for this yet, so just wanted to check if I make sense or it is something mandatory) (new here😊)

Ideally yes.

Example you can copy this and add the extra parameter and update the 2016 references. It's so people know what a fully populated config looks like.

Looking at the integration tests it might be a little tricky as there is no 2022 specific test and it uses the Eval media, which I'm not sure if the new parameter will work with. I'll defer decision to @johlju on this one.

sara921-spec commented 2 months ago

Is .NET 3.5 and 4.5 still required on supported OSes?

No, It comes by default

johlju commented 2 months ago

.Net 3.5 It is required for some features in some SQL Server versions that the resource probably still can install. I suggest it can be updated in another PR to determine when legacy dependencies are needed. 🤔

johlju commented 2 months ago

It seems there is a syntax error here: https://dev.azure.com/dsccommunity/SqlServerDsc/_build/results?buildId=9394&view=logs&jobId=5d97f74b-71f9-5a7c-9686-bc75158bbbc4&j=18b1a044-2ebe-5d7a-c679-f09fca0b67aa&t=d83cb86e-a475-59d3-8144-5a4579d10adc

sara921-spec commented 2 months ago

Just the last few bits left and I can pass it off to someone with merge rights :-).

No use splatting Code snippet:
$getRegistryPropertyParams = @{
   Path = "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL$($SqlVersion).$($InstanceName)\Setup"
   Name = 'IsProductCoveredBySA'
}

@dan-hughes Does it not needed to be passed like below? I mean for ($getTargetResourceReturnValue?) or that line is not needed? Thought we need that gettargetrespurcereturnvalue

$getTargetResourceReturnValue.ProductCoveredBySA = Get-RegistryPropertyValue @getRegistryPropertyParams

dan-hughes commented 2 months ago

Just the last few bits left and I can pass it off to someone with merge rights :-).

No use splatting Code snippet:
$getRegistryPropertyParams = @{
   Path = "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL$($SqlVersion).$($InstanceName)\Setup"
   Name = 'IsProductCoveredBySA'
}
@dan-hughes Does it not needed to be passed like below? I mean for ($getTargetResourceReturnValue?) or that line is not needed? Thought we need that gettargetrespurcereturnvalue

$getTargetResourceReturnValue.ProductCoveredBySA = Get-RegistryPropertyValue @getRegistryPropertyParams

You are correct that is how you pass a splat to a command. The last commit you made looks to be good.

dan-hughes commented 2 months ago

@sara921-spec, run a format on the module file and commit again, it should fix the HQRM test.

sara921-spec commented 2 months ago

@dan-hughes It actually looked for single quotes as mentioned here https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-hashtables-or-objects).,

I believe we are looking good now! do u see any other updates required?

dan-hughes commented 2 months ago

@dan-hughes It actually looked for single quotes as mentioned here https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-hashtables-or-objects).,

I believe we are looking good now! do u see any other updates required?

The path string was not the issue, it was the formatting. I've just cloned your repo and opened it in VSCode.

If I put your double quotes back so the string has the correct values in and format it it looks like this.

sara921-spec commented 1 month ago

@dan-hughes does it looks good now?

dan-hughes commented 1 month ago

@sara921-spec, I'll look at this over the weekend.

sara921-spec commented 1 month ago

Unit Tests: ContextBlock 'When SQL Server version is and the system is not in the desired state for default instance'

It needs to add the check that the value is false.

@dan-hughes False for the parameter productcoveredbysa? In the past johan had the below thought (or am I not reading your request correctly?)

dan-hughes commented 1 month ago

Unit Tests: ContextBlock 'When SQL Server version is and the system is not in the desired state for default instance'

It needs to add the check that the value is false.

@dan-hughes False for the parameter productcoveredbysa? In the past johan had the below thought (or am I not reading your request correctly?)

Apologies for not being clear,

It was more of adding the $result.ProductCoveredBySA | Should -BeFalse check here for the as the resource should return the default value for the type.

It looks like you have resolved all the other issues so once this is done and it builds it's good to go.

dan-hughes commented 1 month ago

I'm not sure where I got that Context block title from though as I've just searched and cannot find it!

johlju commented 1 month ago

@sara921-spec awesome work on this, thank you! Also big thanks to @dan-hughes for the review! 🙇‍♂️

dsccommunity / SqlServerDsc