WebAdministrationDsc icon indicating copy to clipboard operation
WebAdministrationDsc copied to clipboard

DSC_WebSite: Change CertificateStoreName value in the schema

Open webalexeu opened this issue 1 year ago • 1 comments

Pull Request (PR) description

Change CertificateStoreName value in the schema to match the value returned by DSC This is aligned with the testing defined https://github.com/dsccommunity/WebAdministrationDsc/blob/3db41e47fad389e2f05e6a61709a3c93ef3be450/tests/Unit/DSC_Website.Tests.ps1#L2252

This Pull Request (PR) fixes the following issues

Fixes https://github.com/dsccommunity/WebAdministrationDsc/issues/642

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).
  • [x] Resource documentation added/updated in README.md.
  • [x] Resource parameter descriptions added/updated in README.md, schema.mof and comment-based help.
  • [x] Comment-based help added/updated.
  • [x] Localization strings added/updated in all localization files as appropriate.
  • [x] Examples appropriately added/updated.
  • [x] Unit tests added/updated. See DSC Community Testing Guidelines.
  • [x] Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • [x] New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

webalexeu avatar Oct 17 '24 17:10 webalexeu

Codecov Report

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

Project coverage is 88%. Comparing base (04d3bd3) to head (6c66430). Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #641   +/-   ##
===================================
  Coverage    88%    88%           
===================================
  Files        15     15           
  Lines      1952   1952           
===================================
  Hits       1726   1726           
  Misses      226    226           
Files with missing lines Coverage Δ
source/DSCResources/DSC_WebSite/DSC_WebSite.psm1 97% <100%> (ø)

codecov[bot] avatar Oct 17 '24 17:10 codecov[bot]

Suggest we instead change the value in the code to My instead of MY:

https://github.com/dsccommunity/WebAdministrationDsc/blob/04d3bd3de3977f242c9642263fd169da6abb5c34/source/DSCResources/DSC_WebSite/DSC_WebSite.psm1#L1463

johlju avatar Oct 27 '24 12:10 johlju

Suggest we instead change the value in the code to My instead of MY:

https://github.com/dsccommunity/WebAdministrationDsc/blob/04d3bd3de3977f242c9642263fd169da6abb5c34/source/DSCResources/DSC_WebSite/DSC_WebSite.psm1#L1463

I've updated my PR accordingly

webalexeu avatar Oct 28 '24 10:10 webalexeu

Hello @johlju, I've double checked but the get is returning the info in upperscale

protocol             : https
bindingInformation   : *:443:*
sslFlags             : 0
isDsMapperEnabled    : False
certificateHash      : E2CD2BB7A408F069D4C48E4B6ACE5DCEFD9FE94A
certificateStoreName : MY
Attributes           : {protocol, bindingInformation, sslFlags, isDsMapperEnabled...}
ChildElements        : {}
ElementTagName       : binding
Methods              : {EnableDsMapper, DisableDsMapper, AddSslCertificate, RemoveSslCertificate...}
Schema               : Microsoft.IIs.PowerShell.Framework.ConfigurationElementSchema

So I guess it's why all the code has been done to use 'MY' instead of 'My'

Do you want me to revert the schema defintion to 'MY' or add in the get to transform it to 'My'

webalexeu avatar Oct 28 '24 11:10 webalexeu

Do you want me to revert the schema definition to 'MY' or add in the get to transform it to 'My'

Maybe we need to control what is passes to the *-TargetResource and what is returned. It is more PowerShell-ish to use My, so let us try converting the value to 'My'.

Also, PowerShell allows us to pass 'MY' even if the schema says 'My', so maybe we should also convert the input value to My in all *-TargetResource that uses it?

Also make the tests use -BeExactly to tests case sensetive.

johlju avatar Oct 28 '24 12:10 johlju

Do you want me to revert the schema definition to 'MY' or add in the get to transform it to 'My'

Maybe we need to control what is passes to the *-TargetResource and what is returned. It is more PowerShell-ish to use My, so let us try converting the value to 'My'.

Also, PowerShell allows us to pass 'MY' even if the schema says 'My', so maybe we should also convert the input value to My in all *-TargetResource that uses it?

Also make the tests use -BeExactly to tests case sensetive.

PR updated

webalexeu avatar Oct 28 '24 13:10 webalexeu

I resolved conflicts in the hopes that we could have merged it, so you might need to pull in changes too.

johlju avatar Oct 28 '24 16:10 johlju

Reviewed 8 of 11 files at r3, 2 of 2 files at r4, 1 of 1 files at r5, all commit messages. Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @webalexeu)

source/DSCResources/DSC_WebSite/DSC_WebSite.psm1 line 1313 at r5 (raw file):

            $cimProperties.Add('CertificateThumbprint', [String]$binding.certificateHash)
            $cimProperties.Add('CertificateStoreName',  [String]($binding.certificateStoreName) -replace ('MY','My'))

This should only replace 'MY' if the name is exactly 'MY', not 'MYStore' or 'StoreMY'. 🤔

source/DSCResources/DSC_WebSite/DSC_WebSite.psm1 line 1470 at r5 (raw file):

                    else
                    {
                        $certificateStoreName = $binding.CertificateStoreName -replace ('MY','My')

Same as previous comment.

Indeed, thanks for the review So I made it case sensitive and match only the whole word

webalexeu avatar Oct 29 '24 11:10 webalexeu

Reviewed all commit messages. Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @webalexeu)

source/DSCResources/DSC_WebSite/DSC_WebSite.psm1 line 1313 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should only replace 'MY' if the name is exactly 'MY', not 'MYStore' or 'StoreMY'. 🤔

The new change will also convert 'MY store' (although spaces might not be allowed). But just to make sure MY is the only word you need to do: -replace '^MY$','My'

source/DSCResources/DSC_WebSite/DSC_WebSite.psm1 line 1470 at r5 (raw file):

Previously, webalexeu wrote… See previous comment.

Updated Thank you

webalexeu avatar Oct 29 '24 12:10 webalexeu