Closed markgossa closed 6 years ago
codecov-io
commented
6 years ago Merging #171 into dev will not change coverage. The diff coverage is
100%.
@@ Coverage Diff @@
## dev #171 +/- ##
===================================
Coverage 100% 100%
===================================
Files 7 8 +1
Lines 437 486 +49
===================================
+ Hits 437 486 +49
johlju
commented
6 years ago Awesome work on this! Really impressed of your work here! :smile: Localization and tests and everything.
I made a few review comments, but nothing big really. Once these changes are made, I will review the tests.
When you are done with the changes, please go into Reviewable (big purple button in the PR description) and write 'Done' on each of the comments (or comment if you want to discuss a comment). Once you comment on each review comment, you click on the big Publish button at the top of the Reviewable page, then all comments you made will be sent back to GitHub (to this PR as one big comment).
Let me know if you need any help! Again, awesome work here! :smile:
Reviewed 6 of 8 files at r1. Review status: 6 of 8 files reviewed at latest revision, 33 unresolved discussions.
CHANGELOG.md, line 5 at r1 (raw file):
xFailoverCluster
Saw now that there is a typo here, could you please help correct it? Should be xFailOverCluster (upper 'O').
CHANGELOG.md, line 23 at r1 (raw file):
- Added cloud witness (Azure storage) functionality on Windows 2016 ([issue #37](https://github.com/PowerShell/xFailOverCluster/issues/37)). - Added new xClusterProperty ([issue #169](https://github.com/PowerShell/xFailOverCluster/issues/169)).
Maybe we could move this entry under - Changes to xFailOverCluster?
README.md, line 178 at r1 (raw file):
Configures cluster properties on a failover cluster.
Maybe we should use the same description as in the ToC? "Configures cluster properties on a failover cluster."
README.md, line 187 at r1 (raw file):
Uint32
Could we change this to UInt32 (upper 'I')? Throughout in the README.md, in the code and schema.mof.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 24 at r1 (raw file):
CheckingClusterProperties
Could we change this a separate message, like GettingClusterProperties with a text something like Returning current values from cluster '{0}'.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 26 at r1 (raw file):
Write-Verbose -Message ($script:localizedData.CheckingClusterProperties) $ClusterProperties = Get-ClusterPropertyList
This helper function seems unnecessary? Could we do it using the hash table instead? Thinking of something like this.
$returnValue = @{
AddEvictDelay = $null
ClusterLogLevel = $null
# ... and the rest of the properties
}
$clusterProperties = $returnValue.Clone()
foreach ($clusterProperty in $clusterProperties.Keys)
{
$returnValue[$clusterProperty] = $cluster.$clusterProperty
}
# This adds key 'Name' to the hash table and sets the value
$returnValue['Name'] = $NameDSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 28 at r1 (raw file):
$Cluster
Please use camelCase for variables. Throughout.
Except parameters that should use PascalCase.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 40 at r1 (raw file):
return $ReturnValue }
Please remove blank row.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 105 at r1 (raw file):
Specifies how many minutes after a system shutdown is initiated that the failover cluster service will wait for resources to go offline. #>
Please remove blank row.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 112 at r1 (raw file):
( [Parameter()] [Uint32]
Could we change this to System.UInt32 (full name of the type, and upper 'I' in Int). Throughout.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 168 at r1 (raw file):
[Parameter()] [String]
Could we change this to System.String (full name of the type). Throughout.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 194 at r1 (raw file):
$PSBoundParameters
I think you should .Clone() the hash table, otherwise you are removing the keys from both $PSBoundParameters and $Params.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 194 at r1 (raw file):
$Params
Could we change this to $boundParameters (let's avoid using abbreviations).
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 195 at r1 (raw file):
| Out-Null
Is Out-Null necessary, I don't see Remove() method returning anything? Non-blocking if it do return a value in certain scenarios.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 195 at r1 (raw file):
"Name"
Please use single-quotes. Same for the row below.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 200 at r1 (raw file):
"
"$($Param.Value)""
We can add for example '{0}' directly in the localized string in the psd1 file so you don't need to this. Please use single-quotes in the localized string, unless it's more appropriate to use double-quotes in this scenario.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 202 at r1 (raw file):
(Get-Cluster -Name $Name)
Could we run Get-Cluster once before the foreach-block so we get an object back that we can update instead?
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 202 at r1 (raw file):
($Param.Value)
Parenthesis around here should not be necessary.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 270 at r1 (raw file):
Specifies how many minutes after a system shutdown is initiated that the failover cluster service will wait for resources to go offline. #>
Please remove blank row.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 360 at r1 (raw file):
$Params
Could we change this to $boundParameters (let's avoid using abbreviations).
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 360 at r1 (raw file):
$PSBoundParameters
I think you should .Clone() the hash table, otherwise you are removing the keys from both $PSBoundParameters and $Params.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 361 at r1 (raw file):
| Out-Null
Is Out-Null necessary, I don't see Remove() method returning anything? Non-blocking if it do return a value in certain scenarios.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 361 at r1 (raw file):
"Name"
Please use single-quotes. Same for the three rows below.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 370 at r1 (raw file):
$Param
Could we change this to $parameter (let's avoid using abbreviations).
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 372 at r1 (raw file):
($Param.Value)
We should not need parenthesis here?
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 374 at r1 (raw file):
"
"$($Param.Value)""
We can add for example '{0}' directly in the localized string in the psd1 file so you don't need to this. Please use single-quotes in the localized string, unless it's more appropriate to use double-quotes in this scenario.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 375 at r1 (raw file):
Write-Debug
I think we should use Write-Verbose here as well. I think the user would be interested in that information in the verbose output.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 382 at r1 (raw file):
$result }
Remove duplicate blank row.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.schema.mof, line 5 at r1 (raw file):
class MSFT_xClusterProperty : OMI_BaseResource { [Key, Description("Cluster name")] string Name;
Please use the same description as in the README.md. Throughout.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.schema.mof, line 6 at r1 (raw file):
Uint32
Could we change to UInt32 (upper 'I'). Throughout.
DSCResources/MSFT_xClusterProperty/en-US/MSFT_xClusterProperty.strings.psd1, line 4 at r1 (raw file):
ConvertFrom-StringData @' CheckingClusterProperties = Checking cluster properties
Could we end each localized string with a full stop ... properties.. Throughout.
DSCResources/MSFT_xClusterProperty/en-US/MSFT_xClusterProperty.strings.psd1, line 7 at r1 (raw file):
Cluster property {0} is not equal to {1}
The word 'equal' feels a little wrong to me. Thinking that something like this is better; `Cluster property '{0}' has value '{1}', expected it to have value '{2}'. But this is my personal opinion, so leaving this as non-blocking. :smile:
Examples/Resources/xClusterProperty/1-SetClusterProperties.ps1, line 19 at r1 (raw file):
ClusterLogSize = 300 Description = '' Name = 'Cluster1'
Could we move Name to the top of the list (since that is the key)?
Comments from Reviewable
markgossa
commented
6 years ago Cool. Thanks for the feedback. I'll get working on the points in the code review and get back to you 😄
johlju
commented
6 years ago @markgossa Sounds good! Looking forward to it! 😄
markgossa
commented
6 years ago @johlju I've been away so only managed to do a bit of work on this but I'll send over my code in the next week or so.
johlju
commented
6 years ago @markgossa Awesome! No hurry, I know how busy the day job and life can be. 😄
markgossa
commented
6 years ago Review status: 6 of 8 files reviewed at latest revision, 33 unresolved discussions.
CHANGELOG.md, line 5 at r1 (raw file):
> xFailoverCluster Saw now that there is a typo here, could you please help correct it? Should be xFailOverCluster (upper 'O').
Done.
CHANGELOG.md, line 23 at r1 (raw file):
Maybe we could move this entry under `- Changes to xFailOverCluster`?
Done.
README.md, line 178 at r1 (raw file):
> Configures cluster properties on a > failover cluster. Maybe we should use the same description as in the ToC? "Configures cluster properties on a failover cluster."
Done.
README.md, line 187 at r1 (raw file):
> Uint32 Could we change this to UInt32 (upper 'I')? Throughout in the README.md, in the code and schema.mof.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 24 at r1 (raw file):
> CheckingClusterProperties Could we change this a separate message, like `GettingClusterProperties` with a text something like `Returning current values from cluster '{0}'.`
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 26 at r1 (raw file):
This helper function seems unnecessary? Could we do it using the hash table instead? Thinking of something like this. ``` $returnValue = @{ AddEvictDelay = $null ClusterLogLevel = $null # ... and the rest of the properties } $clusterProperties = $returnValue.Clone() foreach ($clusterProperty in $clusterProperties.Keys) { $returnValue[$clusterProperty] = $cluster.$clusterProperty } # This adds key 'Name' to the hash table and sets the value $returnValue['Name'] = $Name ```
It's simpler to just add values to the hashtable keys as it's being created rather than creating a hashtable with null values then setting the values. This is what I've done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 28 at r1 (raw file):
> $Cluster Please use [camelCase](https://msdn.microsoft.com/en-us/library/x2dbyw72(v=vs.71).aspx) for variables. Throughout. *Except parameters that should use PascalCase.*
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 40 at r1 (raw file):
Please remove blank row.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 105 at r1 (raw file):
Please remove blank row.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 112 at r1 (raw file):
Could we change this to `System.UInt32` (full name of the type, and upper 'I' in Int). Throughout.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 168 at r1 (raw file):
Could we change this to `System.String` (full name of the type). Throughout.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 194 at r1 (raw file):
> $PSBoundParameters I think you should `.Clone()` the hash table, otherwise you are removing the keys from both `$PSBoundParameters` and `$Params`.
$PSBoundParameters is actually not a hashtable so it doesn't have the .Clone() method. There is a .CopyTo() but this doesn't do the same as .Clone().
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 194 at r1 (raw file):
> $Params Could we change this to `$boundParameters` (let's avoid using abbreviations).
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 195 at r1 (raw file):
> | Out-Null Is `Out-Null` necessary, I don't see `Remove()` method returning anything? Non-blocking if it _do_ return a value in certain scenarios.
This does return an output in some scenarios so I've left it in.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 195 at r1 (raw file):
> "Name" Please use single-quotes. Same for the row below.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 200 at r1 (raw file):
> "`"$($Param.Value)`"" We can add for example `'{0}'` directly in the localized string in the psd1 file so you don't need to this. Please use single-quotes in the localized string, unless it's more appropriate to use double-quotes in this scenario.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 202 at r1 (raw file):
> ($Param.Value) Parenthesis around here should not be necessary.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 202 at r1 (raw file):
> (Get-Cluster -Name $Name) Could we run `Get-Cluster` once before the `foreach`-block so we get an object back that we can update instead?
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 270 at r1 (raw file):
Please remove blank row.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 360 at r1 (raw file):
> $Params Could we change this to `$boundParameters` (let's avoid using abbreviations).
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 360 at r1 (raw file):
> $PSBoundParameters I think you should `.Clone()` the hash table, otherwise you are removing the keys from both `$PSBoundParameters` and `$Params`.
$PSBoundParameters is actually not a hashtable so it doesn't have the .Clone() method. There is a .CopyTo() but this doesn't do the same as .Clone().
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 361 at r1 (raw file):
> | Out-Null Is `Out-Null` necessary, I don't see `Remove()` method returning anything? Non-blocking if it _do_ return a value in certain scenarios.
This does return an output in some scenarios so I've left it in.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 361 at r1 (raw file):
> "Name" Please use single-quotes. Same for the three rows below.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 370 at r1 (raw file):
> $Param Could we change this to `$parameter` (let's avoid using abbreviations).
Done. Changed to $boundParameter.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 372 at r1 (raw file):
> ($Param.Value) We should not need parenthesis here?
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 374 at r1 (raw file):
> "`"$($Param.Value)`"" We can add for example `'{0}'` directly in the localized string in the psd1 file so you don't need to this. Please use single-quotes in the localized string, unless it's more appropriate to use double-quotes in this scenario.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 375 at r1 (raw file):
> Write-Debug I think we should use `Write-Verbose` here as well. I think the user would be interested in that information in the verbose output.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 382 at r1 (raw file):
Remove duplicate blank row.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.schema.mof, line 5 at r1 (raw file):
Please use the same description as in the README.md. Throughout.
Done.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.schema.mof, line 6 at r1 (raw file):
> Uint32 Could we change to `UInt32` (upper 'I'). Throughout.
Done.
DSCResources/MSFT_xClusterProperty/en-US/MSFT_xClusterProperty.strings.psd1, line 4 at r1 (raw file):
Could we end each localized string with a full stop `... properties.`. Throughout.
Done.
DSCResources/MSFT_xClusterProperty/en-US/MSFT_xClusterProperty.strings.psd1, line 7 at r1 (raw file):
> Cluster property {0} is not equal to {1} The word 'equal' feels a little wrong to me. Thinking that something like this is better; `Cluster property '{0}' has value '{1}', expected it to have value '{2}'. But this is my personal opinion, so leaving this as non-blocking. :smile:
Done.
Examples/Resources/xClusterProperty/1-SetClusterProperties.ps1, line 19 at r1 (raw file):
Could we move `Name` to the top of the list (since that is the key)?
Done.
Comments from Reviewable
markgossa
commented
6 years ago @johlju - Thanks for your patience. I've made the reviewable changes and resolved the merge conflicts. Let me know what you need next.
johlju
commented
6 years ago @markgossa I will get to this tomorrow I hope, if not then, then during the weekend 😄
johlju
commented
6 years ago Awesome work! Saw that I messed up with the $PSBoundParameters vs Clone() :)
Just a couple of easy review comments left. After that I think this looks good to me!
Reviewed 7 of 8 files at r3. Review status: all files reviewed at latest revision, 3 unresolved discussions.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 26 at r1 (raw file):
It's simpler to just add values to the hashtable keys as it's being created rather than creating a hashtable with null values then setting the values. This is what I've done.
LGTM. I couldn't acknowledge this. Maybe you can now that I marked it as satisfied. If not, write Done on this so that I can acknowledge it. :smile:
Tests/Unit/MSFT_xClusterProperty.Tests.ps1, line 30 at r3 (raw file):
"Cluster1"
Please use single quotes. Throughout.
Tests/Unit/MSFT_xClusterProperty.Tests.ps1, line 124 at r3 (raw file):
} Context "$($script:DSCResourceName)\Test-TargetResource" {
There are also tests on line 50 for Test-TargetResource - could these be concatenated? Maybe by using another Context inside the first context block to tell the different tests apart?
Context "$($script:DSCResourceName)\Set-TargetResource" {
Context 'When something 1...' {
}
Context 'When something 2...' {
}
}Comments from Reviewable
markgossa
commented
6 years ago Review status: all files reviewed at latest revision, 2 unresolved discussions.
DSCResources/MSFT_xClusterProperty/MSFT_xClusterProperty.psm1, line 26 at r1 (raw file):
LGTM. I couldn't acknowledge this. Maybe you can now that I marked it as satisfied. If not, write Done on this so that I can acknowledge it. :smile:
Done.
Tests/Unit/MSFT_xClusterProperty.Tests.ps1, line 30 at r3 (raw file):
> "Cluster1" Please use single quotes. Throughout.
Done.
Tests/Unit/MSFT_xClusterProperty.Tests.ps1, line 124 at r3 (raw file):
There are also tests on line 50 for Test-TargetResource - could these be concatenated? Maybe by using another Context inside the first context block to tell the different tests apart? ``` Context "$($script:DSCResourceName)\Set-TargetResource" { Context 'When something 1...' { } Context 'When something 2...' { } }
Actually, I found that the tests at line 50 were duplicate tests, just worded differently so I've removed them. There's just the one context for Test-TargetResource now.
Comments from Reviewable
markgossa
commented
6 years ago @johlju - thanks for the good feedback. I've made the changes so please take a look when you get the chance. Thanks.
johlju
commented
6 years ago 
Reviewed 1 of 1 files at r4. Review status: all files reviewed at latest revision, all discussions resolved.
Comments from Reviewable
johlju
commented
6 years ago @markgossa it is now merged! Awesome work on this one! 😄 Thank you!
markgossa
commented
6 years ago @johlju - my pleasure. Have a good one and perhaps I'll bump into you again on another PR :smile:
markgossa
commented
6 years ago @johlju - and thanks for merging!
johlju
commented
6 years ago @markgossa hope we will, please send in more PR's 😄
Pull Request (PR) description New DSC resource to set cluster properties such as SameSubnetDelay, QuarantineThreshold etc.
This Pull Request (PR) fixes the following issues: Fixes #169
Task list:
This change is