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

xActiveDirectory: Requirements section #399

Merged
merged 47 commits into from
Jun 27, 2019
Merged

Conversation

JamesFrierson1
Copy link
Contributor

@JamesFrierson1 JamesFrierson1 commented Jun 26, 2019

Pull Request (PR) description

Added a Requirements section to every DSCResource README with the bullet point stating "Target machine must be running Windows Server 2008 R2 or later".

This Pull Request (PR) fixes the following issues

Fixes #394

Task list

  • Added an entry under the Unreleased section in the CHANGELOG.md.

This change is Reviewable

@johlju
Copy link
Member

johlju commented Jun 26, 2019

@JamesFrierson1 Could you please resolve the merge conflicts? It seems your working branch is not using the latest changes merged into the dev branch.

See here how to resolve merge conflicts
https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts

@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jun 26, 2019
@codecov-io
Copy link

codecov-io commented Jun 26, 2019

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #399   +/-   ##
===================================
  Coverage    92%    92%           
===================================
  Files        20     20           
  Lines      2538   2538           
  Branches     10     10           
===================================
  Hits       2345   2345           
  Misses      183    183           
  Partials     10     10

Copy link
Contributor Author

@JamesFrierson1 JamesFrierson1 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: 8 of 18 files reviewed, 22 unresolved discussions (waiting on @JamesFrierson1 and @johlju)


CHANGELOG.md, line 32 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
  - Deleted the obsolete xActiveDirectory_TechNetDocumentation.html file

I think this row should be removed since it is already present two rows down.

Done.


CHANGELOG.md, line 33 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
DSCResource

I think we should write it like this "...DSC resource...", also please add a full stop .at he end of the sentence.

Done.


CHANGELOG.md, line 33 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Added a Requirements section to every DSCResource README with the bullet point stating "Target machine must be running Windows Server 2008 R2 or later"

Please add a reference to the issue this change closes, see other entry for example.

Done.


DSCResources/MSFT_xADDomainTrust/README.md, line 2 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add back the empty row after this section.

Done.


DSCResources/MSFT_xADDomainTrust/README.md, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADKDSKey/README.md, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Description

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADKDSKey/README.md, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADManagedServiceAccount/README.md, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Description

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADManagedServiceAccount/README.md, line 5 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADOrganizationalUnit/README.md, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
# Description

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADOrganizationalUnit/README.md, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADReplicationSite/README.md, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
# Description

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADReplicationSite/README.md, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADReplicationSiteLink/README.md, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
# Description

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADReplicationSiteLink/README.md, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADServicePrincipalName/README.md, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
# Description

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADServicePrincipalName/README.md, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADUser/README.md, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
# Description

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xADUser/README.md, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xWaitForADDomain/README.md, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
# Description

Please add an the empty row after this section.

Done.


DSCResources/MSFT_xWaitForADDomain/README.md, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Please add an the empty row after this section.

Done.

Copy link
Contributor Author

@JamesFrierson1 JamesFrierson1 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: 8 of 18 files reviewed, 22 unresolved discussions (waiting on @johlju)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Can you please resolve the failing tests here https://ci.appveyor.com/project/PowerShell/xactivedirectory/builds/25563778?fullLog=true#L307

Done.


@JamesFrierson1
Copy link
Contributor Author

Thank you for your patience, @johlju ! I added the proper spacing per your instructions and updated my changlog entry with a link to the issue

@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 Jun 26, 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:

No worries, this was nothing. 🙂 Thank you for this contribution! Great work! 🙂

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

@johlju johlju added ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. 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. ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. labels Jun 26, 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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JamesFrierson1)


CHANGELOG.md, line 34 at r5 (raw file):

      are not in desired state when verbose output is enabled.
  - Update all unit tests to latest unit test template.
  - Added a Requirements section to every DSC resource README with the bullet point stating "Target machine must be running Windows Server 2008 R2 or later" ([issue #399](https://github.com/PowerShell/xActiveDirectory/pull/399)).

@JamesFrierson1 Actually Katie just released a new version of this module, so we need to movie this entry back up to the unreleased section.
Add a - Changes to xActiveDirectory entry too and put it under. 🙂

I was hoping to get this one in before she did that. Either way, once this PR is merged in the dev branch I will update the wiki with this change.

Copy link
Contributor Author

@JamesFrierson1 JamesFrierson1 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: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @johlju)


CHANGELOG.md, line 34 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

@JamesFrierson1 Actually Katie just released a new version of this module, so we need to movie this entry back up to the unreleased section.
Add a - Changes to xActiveDirectory entry too and put it under. 🙂

I was hoping to get this one in before she did that. Either way, once this PR is merged in the dev branch I will update the wiki with this change.

Done.

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.

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JamesFrierson1)

a discussion (no related file):
There are some failing tests here, could yo look at those?
https://ci.appveyor.com/project/PowerShell/xactivedirectory/builds/25567306?fullLog=true#L1163



DSCResources/MSFT_xADManagedServiceAccount/README.md, line 6 at r6 (raw file):

## Requirements

Can we add a requirement that "Group Managed Service Accounts need at least one Windows Server 2012 Domain Controller.".

Copy link
Contributor Author

@JamesFrierson1 JamesFrierson1 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: 15 of 18 files reviewed, 2 unresolved discussions (waiting on @JamesFrierson1 and @johlju)


DSCResources/MSFT_xADManagedServiceAccount/README.md, line 6 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## Requirements

Can we add a requirement that "Group Managed Service Accounts need at least one Windows Server 2012 Domain Controller.".

Done.

Copy link
Contributor Author

@JamesFrierson1 JamesFrierson1 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: 14 of 18 files reviewed, 2 unresolved discussions (waiting on @johlju)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

There are some failing tests here, could yo look at those?
https://ci.appveyor.com/project/PowerShell/xactivedirectory/builds/25567306?fullLog=true#L1163

Done.


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.

Just one more tiny comment, then I think this is ready to be merged. :)

Reviewed 4 of 4 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JamesFrierson1)


CHANGELOG.md, line 7 at r7 (raw file):

- Changes to xActiveDirectory
  - Added a Requirements section to every DSC resource README with the bullet point stating "Target machine must be running Windows Server 2008 R2 or later" ([issue #399](https://github.com/PowerShell/xActiveDirectory/pull/399)).
  - Added a line to the xADManagedServiceAccount DSC resource README reading "Group Managed Service Accounts need at least one Windows Server 2012 Domain Controller" ([issue #399](https://github.com/PowerShell/xActiveDirectory/pull/399)).

Can we add this second entry under a Changes to xADManagedServiceAccount bullet item so we it is easier to read the changes for a particular resource? The first entry is good where it is.

Copy link
Contributor Author

@JamesFrierson1 JamesFrierson1 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: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @johlju)


CHANGELOG.md, line 7 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we add this second entry under a Changes to xADManagedServiceAccount bullet item so we it is easier to read the changes for a particular resource? The first entry is good where it is.

Done.

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 r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju merged commit c5dd281 into dsccommunity:dev Jun 27, 2019
@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jun 27, 2019
@johlju
Copy link
Member

johlju commented Jun 27, 2019

@JamesFrierson1 Thank you for this! 😃

@JamesFrierson1
Copy link
Contributor Author

You're very welcome @joeyaiello ! My pleasure 😄

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.

xActiveDirectory: Resource have a requirements section (in wiki README.md)
3 participants