search

dsccommunity / FailoverClusterDsc

This module contains DSC resources for deployment and configuration of Windows Server Failover Cluster.
MIT License
60 stars 54 forks source link

xFailOverCluster: Added CodeCov plus Opt-in for common tests #65

Closed johlju closed 7 years ago

johlju commented 7 years ago

Pull Request (PR) description

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

Task list:

This change is Reviewable

msftclas commented 7 years ago

@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

codecov-io commented 7 years ago

Codecov Report

:exclamation: 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 commented 7 years ago

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

bgelens commented 7 years ago

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 commented 7 years ago

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 :smile:

Comments from Reviewable

johlju commented 7 years ago

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 1. Remove .vscode from .gitignore 1. Commit 1. Add back .vscode to .gitignore. 1. Commit 1. 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 :smile:

Done.

Comments from Reviewable

bgelens commented 7 years ago
: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 commented 7 years ago

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

bgelens commented 7 years ago

Just being polite :wink:

johlju commented 7 years ago

Okay good 😉

Merging this one.