-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AdcsAuthorityInformationAccess: Always makes a change #128, #138 #141
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On tests, I'd need to possibly have mocks with only single AIA and OCSP values in to begin with. Thoughts?
I think we should test that no value (null
?), single value, and multiple values are returned. 🤔
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @dan-hughes and @PlagueHO)
source/DSCResources/DSC_AdcsAuthorityInformationAccess/DSC_AdcsAuthorityInformationAccess.psm1
line 37 at r2 (raw file):
IsSingleInstance = 'Yes' AiaUri = Get-CaAiaUriList -ExtensionType 'AddToCertificateAia' OcspUri = Get-CaAiaUriList -ExtensionType 'AddToCertificateOcsp'
We should check if Get-CaAiaUriList
is null or empty, if it is not we should cast the value to an string array - otherwise we return $null.
Same for both values. Also see other comment.
source/DSCResources/DSC_AdcsAuthorityInformationAccess/DSC_AdcsAuthorityInformationAccess.psm1
line 209 at r2 (raw file):
$null = $PSBoundParameters.Remove('IsSingleInstance') $null = $PSBoundParameters.Remove('AllowRestartService')
We should tell Test-DscParameterState
to exclude the parameter from testing instead of removing it (as is also added to the return value of Get-TargetResource). There should be a parameter for that in the command.
source/DSCResources/DSC_AdcsAuthorityInformationAccess/DSC_AdcsAuthorityInformationAccess.psm1
line 239 at r2 (raw file):
$UriList = [System.String[]] (Get-CAAuthorityInformationAccess | Where-Object -Property $ExtensionType -Eq $true).Uri return ,$UriList
I think we should avoid the unary operator. I think it is better to cast the returned value in the next step instead to make sure we actually get a string array unless it returns $null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @johlju and @PlagueHO)
source/DSCResources/DSC_AdcsAuthorityInformationAccess/DSC_AdcsAuthorityInformationAccess.psm1
line 209 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We should tell
Test-DscParameterState
to exclude the parameter from testing instead of removing it (as is also added to the return value of Get-TargetResource). There should be a parameter for that in the command.
That's a nicer solution. I'm assuming the IsSingleInstance
parameter needs to go into the exclude too?
source/DSCResources/DSC_AdcsAuthorityInformationAccess/DSC_AdcsAuthorityInformationAccess.psm1
line 239 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I think we should avoid the unary operator. I think it is better to cast the returned value in the next step instead to make sure we actually get a string array unless it returns
$null
.
This function returns $null
, System.String[]
or System.String
if the call to Get-CAAuthorityInformationAccess
is a single value.
Would it be 'cleaner' to ensure that this function only returns $null
or System.String[]
?
Or is the checking logic explicitly required to work around this behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @johlju and @PlagueHO)
source/DSCResources/DSC_AdcsAuthorityInformationAccess/DSC_AdcsAuthorityInformationAccess.psm1
line 37 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We should check if
Get-CaAiaUriList
is null or empty, if it is not we should cast the value to an string array - otherwise we return $null.Same for both values. Also see other comment.
Taking a fresh look at this Get-CaAiaUriList
will never return System.String[]
on it's own as PowerShell will do it's thing and unwrap the values.
If the result is cast outside as you suggested, is a $null
check required as PowerShell should explicitly convert $null
into an empty string in this case?
source/DSCResources/DSC_AdcsAuthorityInformationAccess/DSC_AdcsAuthorityInformationAccess.psm1
line 239 at r2 (raw file):
Previously, dan-hughes (Daniel Hughes) wrote…
This function returns
$null
,System.String[]
orSystem.String
if the call toGet-CAAuthorityInformationAccess
is a single value.Would it be 'cleaner' to ensure that this function only returns
$null
orSystem.String[]
?Or is the checking logic explicitly required to work around this behaviour?
As per above I'm not sure it will ever return a System.String[]
. Changed just to System.String
, is this a valid approach?
If you rebase this one I continue with this one. 🙂 |
…UriList, add exclude properties parameter instead of removing
I really messed up when trying to merge. I think it's all back to where it should be now. What's going to be the optimal solution for this? The |
I will try to get back to this soon. |
Pull Request (PR) description
Removed the
AllowRestartService
parameter from being tested/compared.Updated
Get-CaAiaUriList
to force aSystem.String[]
to be returned regardless of single or multiple values.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.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)