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

xWaitForADDomain: Fix Issue #341 #435

Merged
merged 5 commits into from
Jul 15, 2019
Merged

Conversation

RobertCGouge
Copy link
Contributor

@RobertCGouge RobertCGouge commented Jul 15, 2019

Pull Request (PR) description

This PR addresses issue #341. It adds comment based help to the xWaitForADDomain resource.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry under the Unreleased section in the CHANGELOG.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in resource directory README.md.
  • Resource parameter descriptions added/updated in schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

@msftclas
Copy link

msftclas commented Jul 15, 2019

CLA assistant check
All CLA requirements met.

@codecov-io
Copy link

Codecov Report

Merging #435 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #435   +/-   ##
===================================
  Coverage    96%    96%           
===================================
  Files        20     20           
  Lines      2599   2599           
  Branches     10     10           
===================================
  Hits       2502   2502           
  Misses       87     87           
  Partials     10     10

@johlju johlju added the needs review The pull request needs a code review. label Jul 15, 2019
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work on this, thank you! 😃 Just one comment.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RobertCGouge)


DSCResources/MSFT_xWaitForADDomain/MSFT_xWaitForADDomain.psm1, line 34 at r1 (raw file):

# The name of the Active Directory domain to wait for.

We should not have these comments above each parameter. Throughout.
These should also be removed/moved if they exist when adding the comment-based help.

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 15, 2019
@johlju
Copy link
Member

johlju commented Jul 15, 2019

@RobertCGouge another thing. if you see the comment lines go over ~80 chars, then please line break them around ~80 chars (not a hard limit), it helps when reviewing. Don't think that was an issue in this resource though, but there will be others, as I'm hoping you want to resolve the others issues as well eventually. 😁
If you use VSCode there is a tool that can be enabled, just add this to your VS Code user settings.

    "editor.rulers": [
        80
    ],

@RobertCGouge
Copy link
Contributor Author

Thank you for the guidance. I have always thought the best practice was to have the help in both places. I've removed the comments from inside the function and left them in the comment blocks.

@johlju johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jul 15, 2019
@johlju
Copy link
Member

johlju commented Jul 15, 2019

@RobertCGouge No worries, happy to help out. Looks like the descriptions above the parameters i Get-TargetResource was not removed. 🙂

Also, when you have resolved the review comments, or if you need to discuss a comment. Could you please go into Reviewable and write 'Done' (or click the done-button) on each of the review comments. This allows me to acknowledge (resolve) the review comments and I know that you are done with each change. After you written 'Done' on each review comment, or comment on the review comment if it needs further discussion, then click the big green 'Publish' button on top of the page. That will send all your comments back as one big comment to this PR.
You find Reviewable if you click on the big purple Reviewable-button in the PR description. 🙂

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 15, 2019
Copy link
Contributor Author

@RobertCGouge RobertCGouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @johlju)


DSCResources/MSFT_xWaitForADDomain/MSFT_xWaitForADDomain.psm1, line 34 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
# The name of the Active Directory domain to wait for.

We should not have these comments above each parameter. Throughout.
These should also be removed/moved if they exist when adding the comment-based help.

Done.

@johlju johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jul 15, 2019
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju merged commit 12f6215 into dsccommunity:dev Jul 15, 2019
@johlju
Copy link
Member

johlju commented Jul 15, 2019

@RobertCGouge merged! Thank you so much!

@RobertCGouge RobertCGouge deleted the Fix-341 branch July 15, 2019 18:23
@johlju johlju removed the needs review The pull request needs a code review. label Aug 3, 2019
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.

xWaitForADDomain: This resource is missing comment-based help
4 participants