Closed jmos5156 closed 5 years ago
Merging #162 into dev will decrease coverage by
9%
. The diff coverage is2%
.
@@ Coverage Diff @@
## dev #162 +/- ##
====================================
- Coverage 100% 90% -10%
====================================
Files 7 8 +1
Lines 413 458 +45
====================================
+ Hits 413 414 +1
- Misses 0 44 +44
@jmos5156 Could you please add unit tests to this? Also add this resource to README.md.
@jmos5156 Let me know if you need any help with this PR.
Hi johlju Any help greatly appreciated. A bit new to this whole process... I do have some updates
No worries. Happy to have you here! 😄 Thanks for contributing! Let's start with this PR, it's seems as the smallest of them you have sent in.
For me to be able to merge this need to be fulfilled
What do you say, should we take it from the below steps (see review comments below), and then build from there? I will help you along the way. Let me know if you want help with any of these step. Happy to help!
And please do push more commits (code changes) if you have an update to this PR. And if you don't know how it works, then it's easy. Just push commits to the same working branch as this PR is based on (xClusterRoleProperties in your fork of xFailOverCluster), and this PR will update.
Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.
a discussion (no related file): Can you add an entry to the Unreleased section of the CHANGELOG.md?
a discussion (no related file): If there is no existing issue that cover this new resource, then please create a new issue for this PR? Please reference the issue in the PR description text under "This Pull Request (PR) fixes the following issues".
This is because discussions around a subject are normally made in issues.
a discussion (no related file): Can you add an example in the Examples folder (create a new folder for the new resource)? See an existing resource for example.
a discussion (no related file): Can you document the resource in the README.md? See an existing resource for example.
a discussion (no related file): Please change the parameters in the code to match the style guidline. See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block
There are a few more style changes that need to resolved, but lets start with just this one, and I review again once these review comments are resolved.
a discussion (no related file): Add unit tests that tests this resource. See other unit tests for example, and the test guidelines.
DSCResources/MSFT_xClusterRoleProperties/MSFT_xClusterRoleProperties.psm1, line 13 at r1 (raw file):
[Parameter(Mandatory = $false)][string] $Priority, [Parameter(Mandatory = $false)][UInt32] $FailoverThreshold, [Parameter(Mandatory = $false)][UInt32] $FailoverPeriod,
Seems you are using tabs. Please change your editor to use 4 white spaces instead of tabs.
DSCResources/MSFT_xClusterRoleProperties/MSFT_xClusterRoleProperties.schema.mof, line 4 at r1 (raw file):
key
Please change to 'Key' (upper 'K'). Same for the next parameter that uses key.
DSCResources/MSFT_xClusterRoleProperties/MSFT_xClusterRoleProperties.schema.mof, line 5 at r1 (raw file):
{ [key, Description("Name of the Role to configure")] string Name;
Please move the type and name to the previous row.
[Key, Description("Name of the Role to configure")] string Name;
Throughout this file.
Comments from Reviewable
Labeling this PR as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on the PR is taken up again.
Closing this PR due to inactivity. If there are someone who wants to work on this issue or PR please open a new PR.
Allows the configuration of Cluster Role Properties
Pull Request (PR) description Allows the configuration of Cluster Role Properties
This Pull Request (PR) fixes the following issues:
Task list:
This change is