Skip to content
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

xFailOverCluster: Updating README.md, adding CHANGELOG.md, adding GitHub templates #60

Merged
merged 5 commits into from
Jun 14, 2017

Conversation

johlju
Copy link
Member

@johlju johlju commented Jun 13, 2017

Fixes #44
Fixes #43
Fixes #36
Fixes #45
Fixes #48
Fixes #51
Fixes #56
Fixes #62
Fixes #58


This change is Reviewable

@msftclas
Copy link

@johlju,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@johlju
Copy link
Member Author

johlju commented Jun 13, 2017

@bgelens or @RichardSiddaway Can anyone of you review this when you got a chance?

I can send in an other PR that fixes the tests before we merge this one. But it's gonna take a few days (probably this weekend).

@johlju johlju added the needs review The pull request needs a code review. label Jun 13, 2017
@bgelens
Copy link
Contributor

bgelens commented Jun 14, 2017

Nice! I think I'll adopt some of those templates pretty soon!


Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


README.md, line 28 at r2 (raw file):

## Change log

A full list of changes in each version can be found in the [change log](CHANGELOG.md).

Is this a general approach for all resources?


README.md, line 124 at r2 (raw file):

#### Parameters

* **IsSingleInstance** Always set to `Yes` to prevent multiple quorum settings per cluster.

Technically, this is actually to ensure this resource can only be used once in the configuration so ping/pong behavior is blocked.
A cluster can only have one quorum, so this is used to prevent on and on change behavior of the quorum.
(I do see it is a copy paste issue from your side so maybe an issue to change this later?)


README.md, line 125 at r2 (raw file):

* **IsSingleInstance** Always set to `Yes` to prevent multiple quorum settings per cluster.
* **Type** Quorum type to use: *NodeMajority*, *NodeAndDiskMajority*, *NodeAndFileShareMajority*, *DiskOnly*.

Again, probably a fix for another issue but the resources above have the property types specified and these don't (not repeating this remark for other occurrences )


README.md, line 169 at r2 (raw file):

        [ValidateNotNullorEmpty()]
        [PsCredential]
        $domainAdminCredential

Parameters should be PascalCase. https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-names-use-pascal-case


README.md, line 171 at r2 (raw file):

        $domainAdminCredential
    )

Missing an Import-DscResource of xFailOverCluster module. Example won't work


README.md, line 184 at r2 (raw file):

            Ensure = 'Present'
            Name   = 'RSAT-Clustering-PowerShell'

Unnecessary white space (not repeating for other occurrences)


README.md, line 188 at r2 (raw file):

        }

        WindowsFeature RSATClusteringCmdInterface

Casing consistency on resource names?


README.md, line 193 at r2 (raw file):

            Name   = 'RSAT-Clustering-CmdInterface'

            DependsOn = '[WindowsFeature]RSATClusteringPowerShell'

'=' alignment ? (not repeating for other occurrences)


README.md, line 196 at r2 (raw file):

        }

        xCluster ensureCreated

Casing consistency on resource names?


README.md, line 202 at r2 (raw file):

            DomainAdministratorCredential = $domainAdminCredential

           DependsOn = “[WindowsFeature]RSATClusteringCmdInterface”

Indentation issue? (I see a couple but could be Reviewable?)


Comments from Reviewable

@johlju johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Jun 14, 2017
@johlju
Copy link
Member Author

johlju commented Jun 14, 2017

I will get to this changes later tonight.


Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


README.md, line 28 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Is this a general approach for all resources?

It is not. The PSDscResource uses README.md https://github.com/PowerShell/PSDscResources#versions

But the README.md tend to be long, and the version history is at the end, so it not easy to find. Also, I feel it's easier to tell contributors to 'update Unreleased section in the CHANGELOG.md', than 'update the Unreleased section under the Version section at the bottom of the README.md' :)

If you feel we should keep it in README.md I happy to move it back.


README.md, line 124 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Technically, this is actually to ensure this resource can only be used once in the configuration so ping/pong behavior is blocked.
A cluster can only have one quorum, so this is used to prevent on and on change behavior of the quorum.
(I do see it is a copy paste issue from your side so maybe an issue to change this later?)

Actually I did not change that description, that was there prior to this PR. I did not even read it. 😄 But I will update the description!

Also added an issue yesterday to fix descriptions; #59. I will comment on that to verify so the descriptions are also correct. 😄


Comments from Reviewable

@bgelens
Copy link
Contributor

bgelens commented Jun 14, 2017

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


README.md, line 28 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It is not. The PSDscResource uses README.md https://github.com/PowerShell/PSDscResources#versions

But the README.md tend to be long, and the version history is at the end, so it not easy to find. Also, I feel it's easier to tell contributors to 'update Unreleased section in the CHANGELOG.md', than 'update the Unreleased section under the Version section at the bottom of the README.md' :)

If you feel we should keep it in README.md I happy to move it back.

I think it's a good idea to split up. Probably a good idea to raise an issue at DscResources to have this standardized?


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jun 14, 2017

I added a bunch to this commit. Added comments to the example. Sorry for the additions, but hard not to try to improve something when you see something. 😄


Review status: 3 of 5 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


README.md, line 28 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

I think it's a good idea to split up. Probably a good idea to raise an issue at DscResources to have this standardized?

Done. Brian has already done that; PowerShell/DscResources#167


README.md, line 124 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Actually I did not change that description, that was there prior to this PR. I did not even read it. 😄 But I will update the description!

Also added an issue yesterday to fix descriptions; #59. I will comment on that to verify so the descriptions are also correct. 😄

Done. Used the text from the best practice article, which is also used in other resources in other modules.
https://msdn.microsoft.com/en-us/powershell/dsc/singleinstance


README.md, line 125 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Again, probably a fix for another issue but the resources above have the property types specified and these don't (not repeating this remark for other occurrences )

Done. Got to late yesterday, so submitted an issue, but I fixed now.


README.md, line 169 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameters should be PascalCase. https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-names-use-pascal-case

Done.


README.md, line 171 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Missing an Import-DscResource of xFailOverCluster module. Example won't work

Done.


README.md, line 184 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Unnecessary white space (not repeating for other occurrences)

Done.


README.md, line 188 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Casing consistency on resource names?

Done. I over clarified them, let's try to avoid abbreviations.


README.md, line 193 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

'=' alignment ? (not repeating for other occurrences)

Done. I removed all aligned '=' since we don't have a guideline to either use or not use, and I feel most of the code in DSC Resource Kit does not align equal signs.


README.md, line 196 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Casing consistency on resource names?

Done.


README.md, line 202 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Indentation issue? (I see a couple but could be Reviewable?)

Done. Reviewable was correct.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jun 14, 2017

When PR #63 is merged, then we can merge this one (after I rebased this one). And after review is finished of course 😄

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jun 14, 2017
@bgelens
Copy link
Contributor

bgelens commented Jun 14, 2017

:lgtm: Great job!!


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

  - Added 'Code of Conduct' text to the README.md (issue dsccommunity#44).
  - Added TOC for all resources in the README.md (issue dsccommunity#43).
  - Fixed typos and lint errors in README.md.
  - Fixed style issue in example in README.md.
  - Removed 'Unreleased' "tag" from the resources xClusterQuorum and xClusterDisk (issue dsccommunity#36)
  - Added new sections to each resource (Requirements, Parameters and Examples) in the README.md. Some does not yet have any examples, so they are set to 'None.'.
  - Added GitHub templates PULL\_REQUEST\_TEMPLATE, ISSUE_TEMPLATE and CONTRIBUTING.md (issue dsccommunity#45).
  - Split the change log from README.md to a seperate file CHANGELOG.md.
  - Added the resource xClusterPreferredOwner to README.md (issue dsccommunity#51).
  - Added the resource xClusterNetwork to README.md (issue dsccommunity#56).
- Updating link to example for resource xCluster and xWaitForCluster.
Changes to xFailOverCluster
  - Removed Credential parameter from parameter list for xWaitForCluster. Parameter Credential does not exist in the schema.mof of the resource (issue dsccommunity#62).
  - Now all parameters in the README.md list their data type and type qualifier (issue dsccommunity#58.)
  - Added Import-DscResource to example in README.md.
@johlju johlju force-pushed the changes-to-xFailOverCluster branch from e043f4d to 83a663e Compare June 14, 2017 19:34
@johlju
Copy link
Member Author

johlju commented Jun 14, 2017

@bgelens Rebased. Please verify in Reviewable that nothing changed. I did change the CHANGELOG.md to move the entries from the recently merged PR.

@bgelens
Copy link
Contributor

bgelens commented Jun 14, 2017

:lgtm: Exactly as planned ;)


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bgelens bgelens merged commit 71849b1 into dsccommunity:dev Jun 14, 2017
@joeyaiello joeyaiello removed the needs review The pull request needs a code review. label Jun 14, 2017
@johlju johlju deleted the changes-to-xFailOverCluster branch June 15, 2017 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants