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: Added CodeCov plus Opt-in for common tests #65

Merged
merged 15 commits into from
Jun 19, 2017

Conversation

johlju
Copy link
Member

@johlju johlju commented Jun 17, 2017

Pull Request (PR) description

  • Changes to xFailOverCluster
  • Changes to xCluster
    • Added examples
      • 1-CreateFirstNodeOfAFailoverCluster.ps1
      • 2-JoinAdditionalNodeToFailoverCluster.ps1
      • 3-CreateFailoverClusterWithTwoNodes.ps1 (this is the example from README.md)
    • Fixed typo in xCluster parameter description.
    • Added links to examples from README.md
  • Changes to xWaitForCluster
    • Added example
      • 1-WaitForFailoverClusterToBePresent.ps1
    • Added link to example from README.md

This Pull Request (PR) fixes the following issues:
Fixes #41
Fixes #42

Task list:

  • Change details added to Unreleased section of CHANGELOG.md?
  • Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
  • Examples appropriately updated?
  • New/changed code adheres to Style Guidelines?
  • Unit and (optional) Integration tests created/updated where possible?

This change is Reviewable

johlju added 14 commits June 17, 2017 10:24
- Added CodeCov and opt-in for all common tests (issue dsccommunity#41).
- Fixed typo in README.md.
 - Fixed lint rule MD013 in CHANGELOG.md.
- Added CodeCov badge to README.md
- Changes to xFailOverCluster
  - Removed example from README.md (issue dsccommunity#42).
Changes to xCluster
  - Added examples
    - 1-CreateFirstNodeOfAFailoverCluster.ps1
    - 2-JoinAdditionalNodeToFailoverCluster.ps1
    - 3-CreateFailoverClusterWithTwoNodes.ps1 (this is the example from README.md)
- Changes to xWaitForCluster
  - Added example
    - 1-WaitForFailoverClusterToBePresent.ps1
- Opt-in for example test
Fixed failing test for the example '3-CreateFailoverClusterWithTwoNodes' by commented the line 'CertificateFile'.
Fixed failing test for the example '3-CreateFailoverClusterWithTwoNodes' by adding parameter 'PSDscAllowPlainTextPassword'.
- Fixed typo in filename for ISSUE\_TEMPLATE. Was 'ISSUE\_TEMPLATE', now it is correctly 'ISSUE\_TEMPLATE.md'.
- Fixed lint rule MD013 in README.md.
Fixed typo in xCluster parameter description.
- Changes to xCluster
  - Added links to examples from README.md
- Changes to xWaitForCluster
  - Added link to example from README.md
- Fixed lint rule MD024 in README.md.
- Fixed lint rule MD032 in README.md.
- Opt-in for markdown test
@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 johlju added the needs review The pull request needs a code review. label Jun 17, 2017
@codecov-io
Copy link

codecov-io commented Jun 17, 2017

Codecov Report

❗ No coverage uploaded for pull request base (dev@71849b1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##             dev   #65   +/-   ##
===================================
  Coverage       ?   57%           
===================================
  Files          ?     6           
  Lines          ?   293           
  Branches       ?     0           
===================================
  Hits           ?   168           
  Misses         ?   125           
  Partials       ?     0

@johlju
Copy link
Member Author

johlju commented Jun 17, 2017

@bgelens Would you mind reviewing this one when you got a chance. 😄

@bgelens
Copy link
Contributor

bgelens commented Jun 19, 2017

Done. Sorry for the delay! Not had a lot of time this weekend :)


Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


.gitignore, line 3 at r1 (raw file):

DSCResource.Tests
.vs
.vscode

Won't this make adding the vscode style rules difficult in the future?


appveyor.yml, line 7 at r1 (raw file):

install:
    - git clone https://github.com/PowerShell/DscResource.Tests

This is not stated on your PR description is it?
Is this a new appveyor.yml default (i'm unfamiliar with it so sorry to ask)


CHANGELOG.md, line 51 at r1 (raw file):

  - Added example
    - 1-WaitForFailoverClusterToBePresent.ps1
  - Added link to example from README.md

Not sure if it's worth mentioning, but the changes to appveyor.yml are not in here


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jun 19, 2017

No worries :) I get this comments fixed.


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


.gitignore, line 3 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Won't this make adding the vscode style rules difficult in the future?

Not difficult. To change, you have to do the following

  1. Change for example .vscode\settings.json
  2. Remove .vscode from .gitignore
  3. Commit
  4. Add back .vscode to .gitignore.
  5. Commit
  6. Push

The downside not to have this in the .gitignore file is that we would get a lot of .vscode files in the PR's that we didn't want.


appveyor.yml, line 7 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

This is not stated on your PR description is it?
Is this a new appveyor.yml default (i'm unfamiliar with it so sorry to ask)

Fixing the PR description.

Yes, PSDscResources uses this, so this is the new default. All repos should change to this, or to the "Harness" model that for example SharePointDsc is using which also allows "auto" Wiki documentation. But then the folder structure need to change as well. "Harness" is not the default.
Read more about it here: https://github.com/PowerShell/DscResource.Tests#example-usage-of-dscresourcetests-in-appveyoryml
And Daniel and Brian has best knowledge of the "Harness" model.


CHANGELOG.md, line 51 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Not sure if it's worth mentioning, but the changes to appveyor.yml are not in here

Yes, absolutely. Fixing that 😄


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jun 19, 2017

Review status: 10 of 11 files reviewed at latest revision, 3 unresolved discussions.


.gitignore, line 3 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Not difficult. To change, you have to do the following

  1. Change for example .vscode\settings.json
  2. Remove .vscode from .gitignore
  3. Commit
  4. Add back .vscode to .gitignore.
  5. Commit
  6. Push

The downside not to have this in the .gitignore file is that we would get a lot of .vscode files in the PR's that we didn't want.

Done. Let me know if you want me to exclude it.


appveyor.yml, line 7 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Fixing the PR description.

Yes, PSDscResources uses this, so this is the new default. All repos should change to this, or to the "Harness" model that for example SharePointDsc is using which also allows "auto" Wiki documentation. But then the folder structure need to change as well. "Harness" is not the default.
Read more about it here: https://github.com/PowerShell/DscResource.Tests#example-usage-of-dscresourcetests-in-appveyoryml
And Daniel and Brian has best knowledge of the "Harness" model.

Done.


CHANGELOG.md, line 51 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Yes, absolutely. Fixing that 😄

Done.


Comments from Reviewable

@bgelens
Copy link
Contributor

bgelens commented Jun 19, 2017

:lgtm:


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


.gitignore, line 3 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done. Let me know if you want me to exclude it.

OK No I'm fine with the inclusion. Just wanted to check.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jun 19, 2017

@bgelens btw, don't be sorry for asking. Happy to answer any questions. 😄

@bgelens
Copy link
Contributor

bgelens commented Jun 19, 2017

Just being polite 😉

@johlju
Copy link
Member Author

johlju commented Jun 19, 2017

Okay good 😉

Merging this one.

@johlju johlju merged commit 343441c into dsccommunity:dev Jun 19, 2017
@joeyaiello joeyaiello removed the needs review The pull request needs a code review. label Jun 19, 2017
@johlju johlju deleted the add-codecov branch June 19, 2017 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants