-
Notifications
You must be signed in to change notification settings - Fork 82
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
Adding Options parameter to Computer resource #381
Adding Options parameter to Computer resource #381
Conversation
Hi @nickgw - it looks like the pipelines are broken. I'll fix these this weekend so that the CI can run on your build. But in the meantime I'll do a quick review of your PR to get you some suggestions. Thank you for submitting! |
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.
Although the Set
has been updated, it looks like there would need to be code added to the Test
. In this case I'd recommend using the Compare-Object
command to compare the two array of strings.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @nickgw)
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 90 at r1 (raw file):
[Parameter()] [System.String()]
I think this should be an array of strings. That way you can also specify ValidateSet()
to ensure the values are valid.
Code snippet:
[ValidateSet('AccountCreate','Win9XUpgrade','UnsecuredJoin','PasswordPass','JoinWithNewName','JoinReadOnly','InstallInvoke')]
[System.String[]]
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 198 at r1 (raw file):
[Parameter()] [System.String()]
I think this should be an array of strings. That way you can also specify ValidateSet()
to ensure the values are valid.
Code snippet:
[ValidateSet('AccountCreate','Win9XUpgrade','UnsecuredJoin','PasswordPass','JoinWithNewName','JoinReadOnly','InstallInvoke')]
[System.String[]]
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 263 at r1 (raw file):
#> $joinOptions = New-Object System.Collections.Generic.List[string] $available_options = @("AccountCreate","Win9XUpgrade","UnsecuredJoin","PasswordPass","JoinWithNewName","JoinReadOnly","InstallInvoke")
I think this whole block can be simplified a lot if the parameter is changed to an array of strings.
The validation for the PasswordPass
is still needed, but you could move this whole block out into an a new Assert-ResourceProperty
function that validates the combination. Like we do here: https://github.com/dsccommunity/NetworkingDsc/blob/main/source/DSCResources/DSC_Route/DSC_Route.psm1#L571
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 273 at r1 (raw file):
} } else { New-InvalidOperationException -Message "PasswordPass is valid only when teh UnsecuredJoin option is specified"
Can this message be defined in the localization file for en-us and referenced (e.g. $script:localizedData.InvalidOptionPasswordPassUnsecuredJoin
) ? Also, regarding the wording, it is helpful to specify the parameter first e.g.: Domain Join option "PasswordPass" may not be specified if "UnsecuredJoin" is specified.
You would probably use New-InvalidArgumentOperation
here and can then specify the parameter name: https://github.com/dsccommunity/DscResource.Common#new-invalidargumentexception
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 273 at r2 (raw file):
Previously, nickgw (Nick G) wrote…
I'm not 100% on how I should be using these error functions if you've got a quick doc on it.
You're using the error functions correctly - just need to use a localization string. I've added comments and links to docs above.
source/DSCResources/DSC_Computer/DSC_Computer.schema.mof, line 12 at r2 (raw file):
[Write, Description("The value assigned here will be set as the local computer description.")] String Description; [Write, Description("The Active Directory Domain Controller to use to join the domain")] String Server; [Write, Description("Specifies advanced options for the Add-Computer join operation")] String Options;
I think this should be an array of strings too. That way you can also add ValueMap{""AccountCreate","Win9XUpgrade","UnsecuredJoin","PasswordPass","JoinWithNewName","JoinReadOnly","InstallInvoke"}, Values{"AccountCreate","Win9XUpgrade","UnsecuredJoin","PasswordPass","JoinWithNewName","JoinReadOnly","InstallInvoke"}
to the MOF.
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 4 files reviewed, 5 unresolved discussions (waiting on @nickgw and @PlagueHO)
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 90 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think this should be an array of strings. That way you can also specify
ValidateSet()
to ensure the values are valid.
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 198 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think this should be an array of strings. That way you can also specify
ValidateSet()
to ensure the values are valid.
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 273 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can this message be defined in the localization file for en-us and referenced (e.g.
$script:localizedData.InvalidOptionPasswordPassUnsecuredJoin
) ? Also, regarding the wording, it is helpful to specify the parameter first e.g.:Domain Join option "PasswordPass" may not be specified if "UnsecuredJoin" is specified.
You would probably use
New-InvalidArgumentOperation
here and can then specify the parameter name: https://github.com/dsccommunity/DscResource.Common#new-invalidargumentexception
Done.
source/DSCResources/DSC_Computer/DSC_Computer.schema.mof, line 12 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think this should be an array of strings too. That way you can also add
ValueMap{""AccountCreate","Win9XUpgrade","UnsecuredJoin","PasswordPass","JoinWithNewName","JoinReadOnly","InstallInvoke"}, Values{"AccountCreate","Win9XUpgrade","UnsecuredJoin","PasswordPass","JoinWithNewName","JoinReadOnly","InstallInvoke"}
to the MOF.
Done.
Once this PR (#384) is merged you should be able to rebase and the pipeline will work. |
Codecov Report
@@ Coverage Diff @@
## main #381 +/- ##
===================================
Coverage 90% 90%
===================================
Files 17 17
Lines 1682 1690 +8
===================================
+ Hits 1520 1528 +8
Misses 162 162
|
Hey @nickgw - your unit tests are failing because this |
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: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @nickgw and @PlagueHO)
tests/Unit/DSC_Computer.Tests.ps1, line 1099 at r6 (raw file):
It 'Should throw if PasswordPass is present in options without UnsecuredJoin' { $errorRecord = Get-InvalidArgumentException `
Should be:
Code snippet:
Get-InvalidArgumentRecord
tests/Unit/DSC_Computer.Tests.ps1, line 1122 at r6 (raw file):
It 'Should throw if PasswordPass and UnsecuredJoin is present but credential username is not null' { $errorRecord = Get-InvalidArgumentException `
Should be:
Code snippet:
Get-InvalidArgumentRecord
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.
Hey, I've made this change but the Unit tests for Computer continue to fail. I'm not certain what I'm doing wrong here, and the test output is not the most helpful.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @nickgw and @PlagueHO)
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: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @PlagueHO)
source/DSCResources/DSC_Computer/DSC_Computer.psm1, line 263 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think this whole block can be simplified a lot if the parameter is changed to an array of strings.
The validation for the
PasswordPass
is still needed, but you could move this whole block out into an a newAssert-ResourceProperty
function that validates the combination. Like we do here: https://github.com/dsccommunity/NetworkingDsc/blob/main/source/DSCResources/DSC_Route/DSC_Route.psm1#L571
I've added an Assert-ResourceProperty
function to handle this validation.
tests/Unit/DSC_Computer.Tests.ps1, line 1099 at r6 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should be:
Resolved, but tests are still failing
tests/Unit/DSC_Computer.Tests.ps1, line 1122 at r6 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should be:
Resolved, but tests are still failing
I'll have a look at test failures tomorrow night. |
Sorry about the delay in looking at this @nickgw - got a bit snowed under. Will try again tomorrow night. |
Thanks, appreciate your time! |
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.
Ok @nickgw - figured out what was wrong. Have added comments below. Can you also add some tests to cover passing the new parameter to Test-TargetResource
and Set-TargetResource
?
Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @nickgw and @PlagueHO)
tests/Unit/DSC_Computer.Tests.ps1, line 1193 at r8 (raw file):
-ArgumentName 'Credential' Assert-ResourceProperty `
Doh - Not sure how I missed this. Should be:
{
Assert-ResourceProperty `
-Name $env:COMPUTERNAME `
-Options @('PasswordPass', 'UnsecuredJoin') `
-Credential $machinePassword `
-Verbose
} | Should -Throw $errorRecord
tests/Unit/DSC_Computer.Tests.ps1, line 1205 at r8 (raw file):
-ArgumentName 'PasswordPass' Assert-ResourceProperty `
Should be:
{
Assert-ResourceProperty `
-Name $env:COMPUTERNAME `
-Options @('PasswordPass') `
-Verbose
} | Should -Throw $errorRecord
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.
I'm not 100% sure on how I would add tests to Test. The parameter set will validate all the passed options, but test shouldn't do anything else with the Options param otherwise.
Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @PlagueHO)
tests/Unit/DSC_Computer.Tests.ps1, line 1122 at r6 (raw file):
Previously, nickgw (Nick G) wrote…
Resolved, but tests are still failing
Done
tests/Unit/DSC_Computer.Tests.ps1, line 1193 at r8 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Doh - Not sure how I missed this. Should be:
{ Assert-ResourceProperty ` -Name $env:COMPUTERNAME ` -Options @('PasswordPass', 'UnsecuredJoin') ` -Credential $machinePassword ` -Verbose } | Should -Throw $errorRecord
Done. Tests are passing now.
tests/Unit/DSC_Computer.Tests.ps1, line 1205 at r8 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should be:
{ Assert-ResourceProperty ` -Name $env:COMPUTERNAME ` -Options @('PasswordPass') ` -Verbose } | Should -Throw $errorRecord
Done. Tests are passing now.
Hey @PlagueHO, please let me know when you get a chance to review this again! |
Hi @nickgw - sorry about the delay. I'll get onto this this weekend (snowed under with day job). I do notice that the code coverage has dropped ag bit - I think it just needs a unit test that covers these two lines by calling Set with the |
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.
Reviewed 1 of 2 files at r4, 1 of 1 files at r5, 1 of 3 files at r6, 1 of 1 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nickgw)
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 272 at r11 (raw file):
#> Assert-ResourceProperty @PSBoundParameters $addComputerParameters.Add("Options", $Options)
Can you convert the "Options" to single quotes? E.g. 'Options'
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 761 at r11 (raw file):
if ( ($options -contains 'PasswordPass') -and
The inner () aren't needed. This should be formatted like this:
if ($options -contains 'PasswordPass' -and
$options -notcontains 'UnsecuredJoin')
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 765 at r11 (raw file):
) { New-InvalidArgumentException -Message $script:localizedData.InvalidOptionPasswordPassUnsecuredJoin -ArgumentName 'PasswordPass'
Can you shorten this line by back-ticking? We try to stay below 100 chars:
New-InvalidArgumentException `
-Message $script:localizedData.InvalidOptionPasswordPassUnsecuredJoin `
-ArgumentName 'PasswordPass'
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 768 at r11 (raw file):
} if (
The inner () aren't needed. This should be formatted like this:
if ($options -contains 'PasswordPass' -and
$options -contains 'UnsecuredJoin')
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 773 at r11 (raw file):
) { if ( -not [System.String]::IsNullOrEmpty($Credential.UserName) )
Could this if
condition be added to the previous if? E.g.
if ($options -contains 'PasswordPass' -and
$options -contains 'UnsecuredJoin' -and
-not [System.String]::IsNullOrEmpty($Credential.UserName))
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 775 at r11 (raw file):
if ( -not [System.String]::IsNullOrEmpty($Credential.UserName) ) { New-InvalidArgumentException -Message $script:localizedData.InvalidOptionCredentialUnsecuredJoinNullUsername -ArgumentName 'Credential'
Can you shorten this line by back-ticking? We try to stay below 100 chars.
source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1
line 19 at r11 (raw file):
DomainNameAndWorkgroupNameError = Only DomainName or WorkGroupName can be specified at once. ComputerNotInDomainMessage = This machine is not a domain member. InvalidOptionPasswordPassUnsecuredJoin = Domain Join option "PasswordPass" may not be specified if "UnsecuredJoin" is specified.
Can we use single quotes instead of double quotes here?
source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1
line 20 at r11 (raw file):
ComputerNotInDomainMessage = This machine is not a domain member. InvalidOptionPasswordPassUnsecuredJoin = Domain Join option "PasswordPass" may not be specified if "UnsecuredJoin" is specified. InvalidOptionCredentialUnsecuredJoinNullUsername = "Credential" username must be null if "UnsecuredJoin" is specified.
Can we use single quotes instead of double quotes here?
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.
Looking really good @nickgw - just some minor tweaks and unit tests and should be good to go.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nickgw)
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: 2 of 5 files reviewed, 8 unresolved discussions (waiting on @PlagueHO)
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 272 at r11 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you convert the "Options" to single quotes? E.g.
'Options'
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 761 at r11 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
The inner () aren't needed. This should be formatted like this:
if ($options -contains 'PasswordPass' -and $options -notcontains 'UnsecuredJoin')
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 765 at r11 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you shorten this line by back-ticking? We try to stay below 100 chars:
New-InvalidArgumentException ` -Message $script:localizedData.InvalidOptionPasswordPassUnsecuredJoin ` -ArgumentName 'PasswordPass'
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 768 at r11 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
The inner () aren't needed. This should be formatted like this:
if ($options -contains 'PasswordPass' -and $options -contains 'UnsecuredJoin')
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 773 at r11 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Could this
if
condition be added to the previous if? E.g.if ($options -contains 'PasswordPass' -and $options -contains 'UnsecuredJoin' -and -not [System.String]::IsNullOrEmpty($Credential.UserName))
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 775 at r11 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you shorten this line by back-ticking? We try to stay below 100 chars.
Done.
source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1
line 19 at r11 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can we use single quotes instead of double quotes here?
Done.
source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1
line 20 at r11 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can we use single quotes instead of double quotes here?
Done.
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.
Hey @PlagueHO , really appreciate the time and comments. I think this is all set now!
Reviewable status: 2 of 5 files reviewed, 8 unresolved discussions (waiting on @PlagueHO)
Cool - thanks @nickgw - I'll finish review this weekend! |
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.
Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickgw)
Looks like an intermittent build failure (integration tests on WS2022). So, I've kicked the build again and hopefully will deploy the preview release. |
Pull Request (PR) description
Adds Options parameter to Computer resource, as defined in https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.management/add-computer?view=powershell-5.1#parameters
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