Closed marciorschneider closed 6 years ago
johlju
commented
6 years ago @PlagueHO I will review this, but do you have time to take a glance at these four new resources to see if you see anything that jumps out? I think you have more knowledge of DHCP than me.
johlju
commented
6 years ago I will continue the review in a bit.
Review status: 0 of 18 files reviewed, 2 unresolved discussions (waiting on @marciorschneider)
README.md, line 19 at r4 (raw file):
xDhcpServerOptions
Are you seeing these resource deprecating MSFT_xDhcpServerOption? In that case I think we should prefix this description with (DEPRECATED).
README.md, line 50 at r4 (raw file):
xDhcpServerOption
Are you seeing these resource deprecating MSFT_xDhcpServerOption? In that case I think we should suffix this section with (DEPRECATED), e.g. xDhcpServerOption (DEPRECATED) .
Comments from Reviewable
PlagueHO
commented
6 years ago Awesome @marciorschneider - and @johlju will do. Just got to finish the big Language one over in ComputerManagementDsc
johlju
commented
6 years ago @PlagueHO Great! I know, it take a while to review these big PR's :)
johlju
commented
6 years ago @marciorschneider Awesome work on this! Code looks great! Review comments are for the most part just minor style changes.
Reviewed 7 of 22 files at r2, 1 of 5 files at r3, 10 of 10 files at r4. Review status: all files reviewed, 89 unresolved discussions (waiting on @marciorschneider)
a discussion (no related file):
I don't see the need to add the x prefix on these new resources, I think we should remove that so we are closer to renaming this resource module. I suggest we call it DhcpPolicyOptionValue, MSFT_DhcpPolicyOptionValue etc. What do you think?
From Naming.md in DSCResource:
The "x" prefix is no longer required. Resources that include the prefix are free to deprecate the convention going forward. https://github.com/PowerShell/DscResources/blob/master/Naming.md#deprecated-experimental
DSCResources/OptionValueHelper.psm1, line 2 at r4 (raw file):
# Localized messages data LocalizedData
Could we move these strings to a .strings.psd1 under an en-US folder? See https://github.com/PowerShell/DscResources/blob/master/BestPractices.md#localization
When doing this, it would also be more appropriate to move the module to a new folder location; /Modules/DhcpServerDsc.OptionValueHelper
DSCResources/OptionValueHelper.psm1, line 25 at r4 (raw file):
$policyWithScopeRemoveValueMessage
I think this should not have a $ in front of the key.
DSCResources/OptionValueHelper.psm1, line 42 at r4 (raw file):
}
Extra blank row here.
DSCResources/OptionValueHelper.psm1, line 43 at r4 (raw file):
<#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/OptionValueHelper.psm1, line 53 at r4 (raw file):
The option ID. .PARAMETER OptionValue
This is not among the functions parameters. It can be removed from here.
DSCResources/OptionValueHelper.psm1, line 127 at r4 (raw file):
[ValidateSet('Present','Absent')] [String] $Ensure = 'Present'
This parameter is not needed here? It's not used in the code (and can then also be removed from the Get-TargetResource in the resources).
DSCResources/OptionValueHelper.psm1, line 137 at r4 (raw file):
#endregion Input Validation # Checking if option needs to be configured for server, DHCP scope, Policy or reservedIP
We should indent this comment.
DSCResources/OptionValueHelper.psm1, line 146 at r4 (raw file):
$serverGettingValueMessage = $localizedData.ServerGettingValueMessage -f $OptionId, $VendorClass, $UserClass Write-Verbose $serverGettingValueMessage $currentConfiguration = Get-DhcpServerv4OptionValue -OptionId $OptionId -VendorClass $VendorClass -UserClass $UserClass -ErrorAction SilentlyContinue
Can we use splatting here (and throughout) to shorten the rows, some are very long?
DSCResources/OptionValueHelper.psm1, line 163 at r4 (raw file):
Write-Verbose $policyGettingValueMessage # Policy can exist on server or scope level, so we need to address both cases
We should use comment block for these comments that is more than one row.
<#
Text ...
#>DSCResources/OptionValueHelper.psm1, line 164 at r4 (raw file):
If ScopeId is $null
Do we need this comment? there are two code paths and comment is only mentioning one.
DSCResources/OptionValueHelper.psm1, line 219 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/OptionValueHelper.psm1, line 306 at r4 (raw file):
[ValidateSet('Present','Absent')] [String] $Ensure = 'Present'
Mandatory parameter should not have default values.
DSCResources/OptionValueHelper.psm1, line 315 at r4 (raw file):
{ # Trying to get the option value $currentConfiguration = Get-TargetResourceHelper -ApplyTo 'Server' -OptionId $OptionId -VendorClass $VendorClass -UserClass $UserClass -AddressFamily $AddressFamily -Ensure $Ensure
Can we use splatting here (and throughout) to shorten the rows, some are very long?
DSCResources/OptionValueHelper.psm1, line 335 at r4 (raw file):
} }
Remove extra blank line
DSCResources/OptionValueHelper.psm1, line 490 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/OptionValueHelper.psm1, line 578 at r4 (raw file):
[ValidateSet('Present','Absent')] [String] $Ensure = 'Present'
Mandatory parameters should not have default values.
DSCResources/OptionValueHelper.psm1, line 586 at r4 (raw file):
'Server' { $currentConfiguration = Get-TargetResourceHelper -ApplyTo 'Server' -OptionId $OptionId -VendorClass $VendorClass -UserClass $UserClass -AddressFamily $AddressFamily -Ensure $Ensure
Can we use splatting here (and throughout) to shorten the rows, some are very long?
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 9 at r4 (raw file):
Import-Module -Name $modulePathOptionValueHelper <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 19 at r4 (raw file):
The ID of the option. .PARAMETER Value
This is not among the functions parameters. It can be removed from here.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 41 at r4 (raw file):
[parameter(
Parameter (upper 'P'). Throughout.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 51 at r4 (raw file):
[parameter(Mandatory = $false)]
We should write these as [Parameter()]. Throughout.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 69 at r4 (raw file):
[ValidateSet('Present','Absent')] [String] $Ensure = 'Present'
This parameter is not needed here? We should not add unused non-mandatory parameters. Also remove from comment-based-help
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 72 at r4 (raw file):
) $result = Get-TargetResourceHelper -ApplyTo 'Policy' -PolicyName $PolicyName -OptionId $OptionId -ScopeId $ScopeId -VendorClass $VendorClass -UserClass '' -AddressFamily $AddressFamily -Ensure $Ensure
Can we use splatting here (and throughout) to shorten the row?
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 77 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 145 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.schema.mof, line 4 at r4 (raw file):
uint32
UInt32 (upper 'UI').
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 9 at r4 (raw file):
Import-Module -Name $modulePathOptionValueHelper <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 19 at r4 (raw file):
The ID of the option. .PARAMETER Value
This is not among the functions parameters. It can be removed from here.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 35 at r4 (raw file):
When set to 'Absent', the option will be removed. #>
Remove extra line
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 42 at r4 (raw file):
[parameter(
Parameter (upper 'P'). Throughout.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 70 at r4 (raw file):
[ValidateSet('Present','Absent')] [String] $Ensure = 'Present'
This parameter is not needed here? We should not add unused non-mandatory parameters. Also remove from comment-based-help
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 73 at r4 (raw file):
) $result = Get-TargetResourceHelper -ApplyTo 'ReservedIP' -ReservedIP $ReservedIP -OptionId $OptionId -VendorClass $VendorClass -UserClass $UserClass -AddressFamily $AddressFamily -Ensure $Ensure
Can we use splatting here (and throughout) to shorten the row?
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 78 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 104 at r4 (raw file):
When set to 'Absent', the option will be removed. #>
Remove extra line
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 148 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.schema.mof, line 5 at r4 (raw file):
uint32
UInt32 (upper 'UI').
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 9 at r4 (raw file):
Import-Module -Name $modulePathOptionValueHelper <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 19 at r4 (raw file):
The ID of the option. .PARAMETER Value
This is not among the functions parameters. It can be removed from here.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 35 at r4 (raw file):
When set to 'Absent', the option will be removed. #>
Remove extra blank line.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 42 at r4 (raw file):
[parameter(
Parameter (upper 'P'). Throughout.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 70 at r4 (raw file):
[ValidateSet('Present','Absent')] [String] $Ensure = 'Present'
This parameter is not needed here? We should not add unused non-mandatory parameters. Also remove from comment-based-help
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 73 at r4 (raw file):
) $result = Get-TargetResourceHelper -ApplyTo 'Scope' -ScopeId $ScopeId -OptionId $OptionId -VendorClass $VendorClass -UserClass $UserClass -AddressFamily $AddressFamily -Ensure $Ensure
Can we use splatting here (and throughout) to shorten the row?
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 78 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 104 at r4 (raw file):
When set to 'Absent', the option will be removed. #>
Remove extra line
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 148 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.schema.mof, line 5 at r4 (raw file):
uint32
UInt32 (upper 'UI').
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 9 at r4 (raw file):
Import-Module -Name $modulePathOptionValueHelper -Force <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 16 at r4 (raw file):
The ID of the option. .PARAMETER Value
This is not among the functions parameters. It can be removed from here.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 32 at r4 (raw file):
When set to 'Absent', the option will be removed. #>
Remove extra line
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 39 at r4 (raw file):
[parameter(
Parameter (upper 'P'). Throughout.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 62 at r4 (raw file):
[ValidateSet('Present','Absent')] [String] $Ensure = 'Present'
This parameter is not needed here? We should not add unused non-mandatory parameters. Also remove from comment-based-help
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 65 at r4 (raw file):
) $result = Get-TargetResourceHelper -ApplyTo 'Server' -OptionId $OptionId -VendorClass $VendorClass -UserClass $UserClass -AddressFamily $AddressFamily -Ensure $Ensure
Can we use splatting here (and throughout) to shorten the row?
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 70 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 93 at r4 (raw file):
When set to 'Absent', the option will be removed. #>
Remove extra line
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 132 at r4 (raw file):
} <#
We should move this back one indent. And .SYNOPSISon the next row seems out of place by one space too.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.schema.mof, line 4 at r4 (raw file):
uint32
UInt32 (upper 'UI').
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 1 at r4 (raw file):
configuration Sample_xDhcpsServerScope_NewScope
Could we add comment-based to the new examples, at least .SYNOPSIS (add a .DESCRIPTION if the description gets to long).
Throughout all the new examples.
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 1 at r4 (raw file):
configuration Sample_xDhcpsServerScope_NewScope
We should name it Example, that way test framework can validate the examples (once op-in). E.g configuration Example.
Throughout all the new examples.
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 3 at r4 (raw file):
xDHCpServer
xDhcpServer (lower-case 'hcp').
Throughout all the new examples.
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 3 at r4 (raw file):
configuration Sample_xDhcpsServerScope_NewScope { Import-DscResource -module xDHCpServer
Should also import PSDscResources (for WindowsFeature).
Throughout all the new examples.
Samples/xDhcpReservedIPOptionValue/xDhcpReservedIPOptionValue.ps1, line 3 at r4 (raw file):
-ModuleVersion
We can remove this.
Samples/xDhcpScopeOptionValue/xDhcpScopeOptionValue.ps1, line 10 at r4 (raw file):
#set scope gateway
We should have space between # and text. Throughout.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 19 at r4 (raw file):
#endregion HEADER
Remove extra line.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 86 at r4 (raw file):
-ParameterFilter
One extra space before this parameter.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 87 at r4 (raw file):
-ModuleName OptionValueHelper
Non-blocking: Curious, do you need to to use -ModuleName (throughout), it doesn't work without? (it should I think, but can be wrong).
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 98 at r4 (raw file):
It 'Returns a "System.Collections.Hashtable" object type' { $result = Get-TargetResource @testParams -Ensure 'Present'
We should also tests so that Get-TargetResource returns the correct values.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 98 at r4 (raw file):
It 'Returns a "System.Collections.Hashtable" object type' { $result = Get-TargetResource @testParams -Ensure 'Present'
We should also have a test for when Get-TargetResource returns 'Absent'
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 99 at r4 (raw file):
$result = Get-TargetResource @testParams -Ensure 'Present' $result -is [System.Collections.Hashtable] | Should Be $true;
We can use BeOfType here as well (as thew previous test does).
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 112 at r4 (raw file):
$result |
An extra space here
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 19 at r4 (raw file):
#endregion HEADER
Remove extra line
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 83 at r4 (raw file):
-ParameterFilter
One extra space before this parameter.
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 95 at r4 (raw file):
It 'Returns a "System.Collections.Hashtable" object type' { $result = Get-TargetResource @testParams -Ensure 'Present'
We should also tests so that Get-TargetResource returns the correct values.
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 95 at r4 (raw file):
It 'Returns a "System.Collections.Hashtable" object type' { $result = Get-TargetResource @testParams -Ensure 'Present'
We should also have a test for when Get-TargetResource returns 'Absent'
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 96 at r4 (raw file):
$result = Get-TargetResource @testParams -Ensure 'Present' $result -is [System.Collections.Hashtable] | Should Be $true;
We can use BeOfType here as well (as thew previous test does).
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 109 at r4 (raw file):
$result = Test-TargetResource @testParams -Ensure 'Present' -Value $value $result | Should BeOfType [System.Boolean]
An extra space here
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 19 at r4 (raw file):
#endregion HEADER
Remove extra line
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 86 at r4 (raw file):
-ParameterFilter
One extra space before this parameter.
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 98 at r4 (raw file):
It 'Returns a "System.Collections.Hashtable" object type' { $result = Get-TargetResource @testParams -Ensure 'Present'
We should also tests so that Get-TargetResource returns the correct values.
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 98 at r4 (raw file):
It 'Returns a "System.Collections.Hashtable" object type' { $result = Get-TargetResource @testParams -Ensure 'Present'
We should also have a test for when Get-TargetResource returns 'Absent'
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 99 at r4 (raw file):
$result = Get-TargetResource @testParams -Ensure 'Present' $result -is [System.Collections.Hashtable] | Should Be $true;
We can use BeOfType here as well (as thew previous test does).
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 112 at r4 (raw file):
$result = Test-TargetResource @testParams -Ensure 'Present' -Value $value $result | Should BeOfType [System.Boolean]
An extra space here
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 19 at r4 (raw file):
#endregion HEADER
Remove extra line
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 81 at r4 (raw file):
-ParameterFilter
One extra space before this parameter.
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 93 at r4 (raw file):
It 'Returns a "System.Collections.Hashtable" object type' { $result = Get-TargetResource @testParams -Ensure 'Present'
We should also tests so that Get-TargetResource returns the correct values.
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 93 at r4 (raw file):
It 'Returns a "System.Collections.Hashtable" object type' { $result = Get-TargetResource @testParams -Ensure 'Present'
We should also have a test for when Get-TargetResource returns 'Absent'
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 94 at r4 (raw file):
$result = Get-TargetResource @testParams -Ensure 'Present' $result -is [System.Collections.Hashtable] | Should Be $true;
We can use BeOfType here as well (as thew previous test does).
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 107 at r4 (raw file):
$result = Test-TargetResource @testParams -Ensure 'Present' -Value $value $result | Should BeOfType [System.Boolean]
An extra space here
Comments from Reviewable
codecov-io
commented
6 years ago :exclamation: No coverage uploaded for pull request base (
dev@f06becc). Click here to learn what that means. The diff coverage is100%.
@@ Coverage Diff @@
## dev #43 +/- ##
======================================
Coverage ? 66.61%
======================================
Files ? 11
Lines ? 608
Branches ? 4
======================================
Hits ? 405
Misses ? 199
Partials ? 4| Impacted Files | Coverage Δ | |
|---|---|---|
| ...pServerOptionValue/MSFT_DhcpServerOptionValue.psm1 | 100% <100%> (ø) |
|
| ...dIPOptionValue/MSFT_DhcpReservedIPOptionValue.psm1 | 100% <100%> (ø) |
|
| ...hcpScopeOptionValue/MSFT_DhcpScopeOptionValue.psm1 | 100% <100%> (ø) |
|
| ...pPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1 | 100% <100%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update f06becc...824960c. Read the comment docs.
marciorschneider
commented
6 years ago Done with the fixes :)
Review status: 0 of 20 files reviewed, 89 unresolved discussions (waiting on @johlju and @marciorschneider)
a discussion (no related file):
I don't see the need to add the x prefix on these new resources, I think we should remove that so we are closer to renaming this resource module. I suggest we call it `DhcpPolicyOptionValue`, `MSFT_DhcpPolicyOptionValue` etc. What do you think? From Naming.md in DSCResource: >The "x" prefix is no longer required. Resources that include the prefix are free to deprecate the convention going forward. >https://github.com/PowerShell/DscResources/blob/master/Naming.md#deprecated-experimental
I agree, it's a step forward.
README.md, line 19 at r4 (raw file):
> ``` > xDhcpServerOptions > ``` Are you seeing these resource deprecating MSFT_xDhcpServerOption? In that case I think we should prefix this description with `(DEPRECATED)`.
Yes, I'll put an example on how use the MSFT_xDhcpScopeOptionValue to substitute it.
README.md, line 50 at r4 (raw file):
> ``` > xDhcpServerOption > ``` Are you seeing these resource deprecating MSFT_xDhcpServerOption? In that case I think we should suffix this section with `(DEPRECATED)`, e.g. `xDhcpServerOption (DEPRECATED)` .
Done.
DSCResources/OptionValueHelper.psm1, line 2 at r4 (raw file):
Could we move these strings to a .strings.psd1 under an en-US folder? See https://github.com/PowerShell/DscResources/blob/master/BestPractices.md#localization When doing this, it would also be more appropriate to move the module to a new folder location; `/Modules/DhcpServerDsc.OptionValueHelper`
Done, for importing the localization data I've used the "CommonResourceHelper.psm1" helper, like the "SqlServerDsc" module.
DSCResources/OptionValueHelper.psm1, line 25 at r4 (raw file):
> ``` > $policyWithScopeRemoveValueMessage > ``` I think this should not have a `$` in front of the key.
Done.
DSCResources/OptionValueHelper.psm1, line 42 at r4 (raw file):
Extra blank row here.
Done.
DSCResources/OptionValueHelper.psm1, line 43 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
done.
DSCResources/OptionValueHelper.psm1, line 53 at r4 (raw file):
This is not among the functions parameters. It can be removed from here.
Done.
DSCResources/OptionValueHelper.psm1, line 127 at r4 (raw file):
This parameter is not needed here? It's not used in the code (and can then also be removed from the Get-TargetResource in the resources).
Done.
DSCResources/OptionValueHelper.psm1, line 137 at r4 (raw file):
We should indent this comment.
Done.
DSCResources/OptionValueHelper.psm1, line 146 at r4 (raw file):
Can we use splatting here (and throughout) to shorten the rows, some are very long?
Done.
DSCResources/OptionValueHelper.psm1, line 163 at r4 (raw file):
We should use comment block for these comments that is more than one row. ``` <# Text ... #> ```
done.
DSCResources/OptionValueHelper.psm1, line 164 at r4 (raw file):
> ``` > If ScopeId is $null > ``` Do we need this comment? there are two code paths and comment is only mentioning one.
done.
DSCResources/OptionValueHelper.psm1, line 219 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/OptionValueHelper.psm1, line 306 at r4 (raw file):
Mandatory parameter should not have default values.
Done.
DSCResources/OptionValueHelper.psm1, line 315 at r4 (raw file):
Can we use splatting here (and throughout) to shorten the rows, some are very long?
Done.
DSCResources/OptionValueHelper.psm1, line 335 at r4 (raw file):
Remove extra blank line
Done.
DSCResources/OptionValueHelper.psm1, line 490 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/OptionValueHelper.psm1, line 578 at r4 (raw file):
Mandatory parameters should not have default values.
Done.
DSCResources/OptionValueHelper.psm1, line 586 at r4 (raw file):
Can we use splatting here (and throughout) to shorten the rows, some are very long?
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 9 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 19 at r4 (raw file):
This is not among the functions parameters. It can be removed from here.
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 41 at r4 (raw file):
> ``` > [parameter( > ``` Parameter (upper 'P'). Throughout.
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 51 at r4 (raw file):
> ``` > [parameter(Mandatory = $false)] > ``` We should write these as `[Parameter()]`. Throughout.
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 69 at r4 (raw file):
This parameter is not needed here? We should not add unused non-mandatory parameters. Also remove from comment-based-help
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 72 at r4 (raw file):
Can we use splatting here (and throughout) to shorten the row?
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 77 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 145 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.schema.mof, line 4 at r4 (raw file):
> ``` > uint32 > ``` UInt32 (upper 'UI').
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 9 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 19 at r4 (raw file):
This is not among the functions parameters. It can be removed from here.
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 35 at r4 (raw file):
Remove extra line
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 42 at r4 (raw file):
> ``` > [parameter( > ``` Parameter (upper 'P'). Throughout.
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 70 at r4 (raw file):
This parameter is not needed here? We should not add unused non-mandatory parameters. Also remove from comment-based-help
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 73 at r4 (raw file):
Can we use splatting here (and throughout) to shorten the row?
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 78 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 104 at r4 (raw file):
Remove extra line
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.psm1, line 148 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpReservedIPOptionValue/MSFT_xDhcpReservedIPOptionValue.schema.mof, line 5 at r4 (raw file):
> ``` > uint32 > ``` UInt32 (upper 'UI').
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 9 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 19 at r4 (raw file):
This is not among the functions parameters. It can be removed from here.
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 35 at r4 (raw file):
Remove extra blank line.
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 42 at r4 (raw file):
> ``` > [parameter( > ``` Parameter (upper 'P'). Throughout.
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 70 at r4 (raw file):
This parameter is not needed here? We should not add unused non-mandatory parameters. Also remove from comment-based-help
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 73 at r4 (raw file):
Can we use splatting here (and throughout) to shorten the row?
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 78 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 104 at r4 (raw file):
Remove extra line
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.psm1, line 148 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpScopeOptionValue/MSFT_xDhcpScopeOptionValue.schema.mof, line 5 at r4 (raw file):
> ``` > uint32 > ``` UInt32 (upper 'UI').
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 9 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 16 at r4 (raw file):
This is not among the functions parameters. It can be removed from here.
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 32 at r4 (raw file):
Remove extra line
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 39 at r4 (raw file):
> ``` > [parameter( > ``` Parameter (upper 'P'). Throughout.
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 62 at r4 (raw file):
This parameter is not needed here? We should not add unused non-mandatory parameters. Also remove from comment-based-help
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 65 at r4 (raw file):
Can we use splatting here (and throughout) to shorten the row?
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 70 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 93 at r4 (raw file):
Remove extra line
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.psm1, line 132 at r4 (raw file):
We should move this back one indent. And `.SYNOPSIS`on the next row seems out of place by one space too.
Done.
DSCResources/MSFT_xDhcpServerOptionValue/MSFT_xDhcpServerOptionValue.schema.mof, line 4 at r4 (raw file):
> ``` > uint32 > ``` UInt32 (upper 'UI').
Done.
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 1 at r4 (raw file):
Could we add comment-based to the new examples, at least `.SYNOPSIS` (add a `.DESCRIPTION` if the description gets to long). Throughout all the new examples.
Done.
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 1 at r4 (raw file):
> ``` > configuration Sample_xDhcpsServerScope_NewScope > ``` We should name it Example, that way test framework can validate the examples (once op-in). E.g `configuration Example`. Throughout all the new examples.
Done.
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 3 at r4 (raw file):
> ``` > xDHCpServer > ``` xDhcpServer (lower-case 'hcp'). Throughout all the new examples.
Done.
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 3 at r4 (raw file):
Should also import PSDscResources (for `WindowsFeature`). Throughout all the new examples.
Done.
Samples/xDhcpReservedIPOptionValue/xDhcpReservedIPOptionValue.ps1, line 3 at r4 (raw file):
> ``` > -ModuleVersion > ``` We can remove this.
Done.
Samples/xDhcpScopeOptionValue/xDhcpScopeOptionValue.ps1, line 10 at r4 (raw file):
> ``` > #set scope gateway > ``` We should have space between # and text. Throughout.
Done.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 19 at r4 (raw file):
Remove extra line.
Done.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 86 at r4 (raw file):
> ``` > -ParameterFilter > ``` One extra space before this parameter.
Done.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 87 at r4 (raw file):
> ``` > -ModuleName OptionValueHelper > ``` Non-blocking: Curious, do you need to to use `-ModuleName` (throughout), it doesn't work without? (it should I think, but can be wrong).
Yes, I don't why but without the -Modulename the mocking doesn't work with the helper module.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 98 at r4 (raw file):
We should also tests so that Get-TargetResource returns the correct values.
Done.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 98 at r4 (raw file):
We should also have a test for when Get-TargetResource returns 'Absent'
Done.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 99 at r4 (raw file):
We can use BeOfType here as well (as thew previous test does).
Done.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 112 at r4 (raw file):
> ``` > $result | > ``` An extra space here
Done.
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 19 at r4 (raw file):
Remove extra line
Done.
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 83 at r4 (raw file):
> ``` > -ParameterFilter > ``` One extra space before this parameter.
Done.
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 95 at r4 (raw file):
We should also tests so that Get-TargetResource returns the correct values.
Done.
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 95 at r4 (raw file):
We should also have a test for when Get-TargetResource returns 'Absent'
Done.
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 96 at r4 (raw file):
We can use BeOfType here as well (as thew previous test does).
Done.
Tests/Unit/MSFT_xDhcpReservedIPOptionValue.Tests.ps1, line 109 at r4 (raw file):
An extra space here
Done.
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 19 at r4 (raw file):
Remove extra line
Done.
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 86 at r4 (raw file):
> ``` > -ParameterFilter > ``` One extra space before this parameter.
Done.
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 98 at r4 (raw file):
We should also tests so that Get-TargetResource returns the correct values.
Done.
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 98 at r4 (raw file):
We should also have a test for when Get-TargetResource returns 'Absent'
Done.
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 99 at r4 (raw file):
We can use BeOfType here as well (as thew previous test does).
Done.
Tests/Unit/MSFT_xDhcpScopeOptionValue.Tests.ps1, line 112 at r4 (raw file):
An extra space here
Done.
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 19 at r4 (raw file):
Remove extra line
Done.
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 81 at r4 (raw file):
> ``` > -ParameterFilter > ``` One extra space before this parameter.
Done.
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 93 at r4 (raw file):
We should also tests so that Get-TargetResource returns the correct values.
Done.
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 93 at r4 (raw file):
We should also have a test for when Get-TargetResource returns 'Absent'
Done.
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 94 at r4 (raw file):
We can use BeOfType here as well (as thew previous test does).
Done.
Tests/Unit/MSFT_xDhcpServerOptionValue.Tests.ps1, line 107 at r4 (raw file):
An extra space here
Done.
Comments from Reviewable
johlju
commented
6 years ago Awesome work resolving those - just a few new minor ones. :smiley:
Reviewed 20 of 20 files at r5. Review status: all files reviewed, 9 unresolved discussions (waiting on @johlju and @marciorschneider)
README.md, line 19 at r5 (raw file):
xDhcpServerOption
Could you please submit an issue to track removing this deprecated resource in a future release?
en-US/OptionValueHelper.strings.psd1, line 1 at r5 (raw file):
# Localized resources for helper module OptionValueHelper.
This files should be in the folder /Modules/en-US instead ('en-US' should be in the same folder as the module it belongs to).
Modules/CommonResourceHelper.psm1, line 1 at r5 (raw file):
<#
Can you bring in the unit tests for CommonresourceHelper.psm1 as well? There are unit test in the SqlServerDsc repo too (might need to change the path in the tests where it find the module though).
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 29 at r5 (raw file):
The option definition address family (IPv4 or IPv6). Currently only the IPv4 is supported. .PARAMETER Ensure
We should remove this parameter from the comment-based help as it was removed.
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 65 at r5 (raw file):
$Params
Can we write this out as $parameters (also with lower-case 'p), to avoid abbreviations. Throughout, and throughout the new resources.
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 68 at r5 (raw file):
$Params.Remove('Ensure') $result = Get-TargetResourceHelper -ApplyTo 'Policy' -UserClass '' @Params $result
We should only return (all) the properties in the schema.mof. The helper function returns more properties.
Throughout the new resources.
Tests/Unit/MSFT_DhcpPolicyOptionValue.Tests.ps1, line 106 at r5 (raw file):
$result = Get-TargetResource @testParams $result.Ensure | Should -Be 'Absent'
We should test so that the rest of the hash table properties is returned with the correct values when Ensure -eq 'Absent.
Throughout the unit tests for the new resources.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 99 at r4 (raw file):
The option definition address family (IPv4 or IPv6). Currently only the IPv4 is supported. .PARAMETER Ensure
Parameter Ensure should have been kept in the comment-based help for this function.
Samples/xDhcpPolicyOptionValue/xDhcpPolicyOptionValue.ps1, line 3 at r4 (raw file):
Done.
xDhcpServer (upper'D'). :slightly_smiling_face:
Throughout the new examples.
Tests/Unit/MSFT_xDhcpPolicyOptionValue.Tests.ps1, line 87 at r4 (raw file):
Yes, I don't why but without the -Modulename the mocking doesn't work with the helper module.
Yes, I see now, it's needed! It's needed because it's the module that calls the helper function, not the resource. Cool! :smiley:
Comments from Reviewable
marciorschneider
commented
6 years ago Done :)
Review status: 6 of 21 files reviewed, 9 unresolved discussions (waiting on @johlju and @marciorschneider)
README.md, line 19 at r5 (raw file):
> ``` > xDhcpServerOption > ``` Could you please submit an issue to track removing this deprecated resource in a future release?
Sure
Modules/CommonResourceHelper.psm1, line 1 at r5 (raw file):
Can you bring in the unit tests for CommonresourceHelper.psm1 as well? There are unit test in the SqlServerDsc repo too (might need to change the path in the tests where it find the module though).
Done.
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 29 at r5 (raw file):
We should remove this parameter from the comment-based help as it was removed.
Done.
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 65 at r5 (raw file):
> ``` > $Params > ``` Can we write this out as `$parameters` (also with lower-case 'p), to avoid abbreviations. Throughout, and throughout the new resources.
Done.
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 68 at r5 (raw file):
We should only return (all) the properties in the schema.mof. The helper function returns more properties. Throughout the new resources.
ok, I'll remote the extra properties from the hash table object before get-targetresource returns it.
Tests/Unit/MSFT_DhcpPolicyOptionValue.Tests.ps1, line 106 at r5 (raw file):
We should test so that the rest of the hash table properties is returned with the correct values when Ensure -eq 'Absent. Throughout the unit tests for the new resources.
Done.
DSCResources/MSFT_xDhcpPolicyOptionValue/MSFT_xDhcpPolicyOptionValue.psm1, line 99 at r4 (raw file):
Parameter `Ensure` should have been kept in the comment-based help for this function.
Done.
en-US/OptionValueHelper.strings.psd1, line 1 at r5 (raw file):
This files should be in the folder /Modules/en-US instead ('en-US' should be in the same folder as the module it belongs to).
Done.
Comments from Reviewable
johlju
commented
6 years ago Just three minor comments left. Then I think this looks good to me. :smiley:
Reviewed 15 of 15 files at r6. Review status: all files reviewed, 3 unresolved discussions (waiting on @johlju and @marciorschneider)
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 29 at r5 (raw file):
Done.
Still seeing this parameter in the comment-based help :)
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 8 at r6 (raw file):
-force
-Force (uppercase 'F')
en-US/OptionValueHelper.strings.psd1, line 1 at r5 (raw file):
Done.
You moved the module file to /Modules/DhcpServerDsc.OptionValueHelper (very good, I like it better! :+1: ), but now the en-US folder should be moved to /Modules/DhcpServerDsc.OptionValueHelper/en-US. That way the entire module folder is standalone.
Sorry for being pain :/
Comments from Reviewable
marciorschneider
commented
6 years ago Review status: all files reviewed, 3 unresolved discussions (waiting on @johlju and @marciorschneider)
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 29 at r5 (raw file):
Still seeing this parameter in the comment-based help :)
ops, sorry
DSCResources/MSFT_DhcpPolicyOptionValue/MSFT_DhcpPolicyOptionValue.psm1, line 8 at r6 (raw file):
> ``` > -force > ``` `-Force` (uppercase 'F')
Ops, it was a leftover from my testing.
en-US/OptionValueHelper.strings.psd1, line 1 at r5 (raw file):
You moved the module file to `/Modules/DhcpServerDsc.OptionValueHelper` (very good, I like it better! :+1: ), but now the en-US folder should be moved to `/Modules/DhcpServerDsc.OptionValueHelper/en-US`. That way the entire module folder is standalone. Sorry for being pain :/
Done, no problem ;)
Comments from Reviewable
johlju
commented
6 years ago Just one tiny comment left, after that I think this is good to go!
Reviewed 4 of 4 files at r7. Review status: all files reviewed, 1 unresolved discussion (waiting on @marciorschneider)
README.md, line 129 at r7 (raw file):
* Added xDhcpServerOptionDefinition * Added xDhcpScopeOptionValue
We should remove 'x' from the names in the change log too.
Comments from Reviewable
marciorschneider
commented
6 years ago Done :D
Review status: 20 of 21 files reviewed, 1 unresolved discussion (waiting on @johlju)
README.md, line 129 at r7 (raw file):
We should remove 'x' from the names in the change log too.
Done.
Comments from Reviewable
johlju
commented
6 years ago 
Reviewed 1 of 1 files at r8. Review status: :shipit: complete! all files reviewed, all discussions resolved
Comments from Reviewable
johlju
commented
6 years ago @marciorschneider Awesome work on this! 👍 Thanks!
Hello, here is the resource for managing DHCP option values. This could substitute the MSFT_xDhcpServerOption resource.
Fixes #11
This change is