BREAKING CHANGE: Add Active Directory Certificate Authority Settings Resource - Fixes #13 #94
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #94 +/- ##
=================================
- Coverage 98% 97% -1%
=================================
Files 7 8 +1
Lines 569 641 +72
=================================
+ Hits 560 626 +66
- Misses 9 15 +6 |
There was a problem hiding this comment.
Reviewed 18 of 18 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @PlagueHO)
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 53 at r1 (raw file):
Function
function (lower-case 'f'). Throughout.
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 192 at r1 (raw file):
Uint32
I think it should be UInt32 (upper-case 'I'). Throughout.
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 425 at r1 (raw file):
$null = $PSBoundParameters.Remove('IsSingleInstance')
Not sure $null is necessary? Remove() doesn't seem to return an object? 🤔
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.data.psd1, line 31 at r1 (raw file):
Uint32'
Same as previous comment.
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.schema.mof, line 7 at r1 (raw file):
Uint32
Same as previous comment
|
Clean coding as always. 😄 Just minor things. |
There was a problem hiding this comment.
Thankyou very much @johlju - should be done and ready to go now! I did correct the style issues you raised in all the resources. Didn't change the $PSBoundParameters.Remove() one because the function does return a bool.
Reviewable status: 13 of 24 files reviewed, 4 unresolved discussions (waiting on @johlju)
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 53 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Functionfunction (lower-case 'f'). Throughout.
Done.
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 192 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Uint32I think it should be
UInt32(upper-case 'I'). Throughout.
Done.
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 425 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
$null = $PSBoundParameters.Remove('IsSingleInstance')Not sure
$nullis necessary?Remove()doesn't seem to return an object? 🤔
$PSBoundParameters.Remove() returns a boolean value. It returns $true if the parameter exists and was removed or false if it doesn't. So it is always best to suppress the output from this (and Add aswell).
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.data.psd1, line 31 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Uint32'Same as previous comment.
Done.
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.schema.mof, line 7 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Uint32Same as previous comment
Done.
There was a problem hiding this comment.
Just a tiny one left 😃
Reviewed 11 of 11 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)
ActiveDirectoryCSDsc.psd1, line 27 at r2 (raw file):
functions
Accidentally changed this one, it shoulfd be upper-case 'F' here
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 425 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
$PSBoundParameters.Remove() returns a boolean value. It returns $true if the parameter exists and was removed or false if it doesn't. So it is always best to suppress the output from this (and Add aswell).
I did not know this! Learned something new today! 😄
There was a problem hiding this comment.
@johlju - thanks again! Should be good to go now.
Reviewable status: 23 of 24 files reviewed, 1 unresolved discussion (waiting on @johlju)
ActiveDirectoryCSDsc.psd1, line 27 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
functionsAccidentally changed this one, it shoulfd be upper-case 'F' here
Doh! Fixed!
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 425 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I did not know this! Learned something new today! 😄
I got caught out by it once (a difficult to track down bug) so I've remembered it ever since 😁 - nothing like a several hours of frustration to cement something in!
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
DSCResources/MSFT_AdcsCertificationAuthoritySettings/MSFT_AdcsCertificationAuthoritySettings.psm1, line 425 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I got caught out by it once (a difficult to track down bug) so I've remembered it ever since 😁 - nothing like a several hours of frustration to cement something in!
Haha yes, know the feeling! Saw a bunch in ActiveDirectoryDsc! will fix those, added an issue for that.
Pull Request (PR) description
This PR adds the AdcsCertificateAuthoritySettings resource. This resource uses a WMF 5.0 feature (enum) so the resource module is moving to require WMF 5.0. Older versions of the resource will still work with WMF 4.0.
Because this module is moving to require WMF5.0 it is considered a BREAKING CHANGE.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
@johlju - would you mind reviewing this one for me when you get a chance? I'm hoping to get this one in before next resource kit release. Hopefully tests pass as I've got to shoot out of the house before they complete.
This change is