xADObjectEnabledState: New resource #277

johlju · 2019-05-17T18:06:04Z

Pull Request (PR) description

Changes to xActiveDirectory
- Added new resource xADObjectEnabledState. This resource should be
  used to enforce the Enabled property of computer accounts. This
  resource replaces the deprecated Enabled property in the resource
  xADComputer.

The verbose output of the integration tests are in this gist:
https://gist.github.com/johlju/fdb402beb503529d9265f934e4ec6781

This Pull Request (PR) fixes the following issues

From the result of the discussion in PR #170 and fixes one part of the outcome in option #1 in the #170 (comment).

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 README.md.
Resource parameter descriptions added/updated in README.md, 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

codecov-io · 2019-05-17T18:14:23Z

Codecov Report

Merging #277 into dev will increase coverage by <1%.
The diff coverage is 98%.

@@         Coverage Diff          @@
##            dev   #277    +/-   ##
====================================
+ Coverage    91%    91%   +<1%     
====================================
  Files        19     20     +1     
  Lines      2317   2383    +66     
  Branches     10     10            
====================================
+ Hits       2124   2190    +66     
  Misses      183    183            
  Partials     10     10

johlju · 2019-05-22T12:47:06Z

@nyanhp, @devopsjesus or @rchristman89 could anyone of you review this PR when you have time? It's a larger PR (new resource).

johlju

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion

DSCResources/MSFT_xADObjectEnabledState/MSFT_xADObjectEnabledState.psm1, line 368 at r1 (raw file):

function Set-DscADComputer

This helper function exist both in xADComputer and here, it should be moved out to a common helper function.

johlju

Reviewable status: 0 of 14 files reviewed, all discussions resolved

DSCResources/MSFT_xADObjectEnabledState/MSFT_xADObjectEnabledState.psm1, line 368 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

function Set-DscADComputer
This helper function exist both in xADComputer and here, it should be moved out to a common helper function.

Done

johlju · 2019-05-26T16:53:47Z

@PlagueHO World you mind review this one when you have a chance. We need to have this merged before next release (~month away) 😊

johlju · 2019-06-04T12:44:08Z

@PlagueHO know your busy. Just pinging for a reminder. 🙂

devopsjesus

Reviewed 1 of 6 files at r2.
Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @devopsjesus and @johlju)

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

#xadobjectenabledstate

is this supposed to be here still? I've not seen this schema used before.

DSCResources/MSFT_xADObjectEnabledState/MSFT_xADObjectEnabledState.psm1, line 90 at r2 (raw file):

'Computer'

Is the idea to add ValidationSets and switch cases here with the addition of objects that are supported?

DSCResources/MSFT_xADObjectEnabledState/MSFT_xADObjectEnabledState.psm1, line 319 at r2 (raw file):

Quoted 25 lines of code…

    $getTargetResourceParameters = @{
        Identity         = $Identity
        ObjectClass      = $ObjectClass
        Enabled          = $Enabled
        DomainController = $DomainController
        Credential       = $Credential
    }

    # Need the @() around this to get a new array to enumerate.
    @($getTargetResourceParameters.Keys) | ForEach-Object {
        if (-not $PSBoundParameters.ContainsKey($_))
        {
            $getTargetResourceParameters.Remove($_)
        }
    }

    $getTargetResourceResult = Get-TargetResource @getTargetResourceParameters

    $compareTargetResourceStateParameters = @{
        CurrentValues = $getTargetResourceResult
        DesiredValues = $PSBoundParameters
        Properties    = @('Enabled')
    }

    $compareTargetResourceStateResult = Compare-ResourcePropertyState @compareTargetResourceStateParameters

This code block is repeated throughout each function (get/set/test), so could you put it in a helper function to reduce repeated code?

johlju

Will fix the new helper function tomorrow.

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @devopsjesus and @johlju)

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

Previously, devopsjesus (Jason Ryberg) wrote…

#xadobjectenabledstate
is this supposed to be here still? I've not seen this schema used before.

I think this was fixed in the rebase?

DSCResources/MSFT_xADObjectEnabledState/MSFT_xADObjectEnabledState.psm1, line 90 at r2 (raw file):

Previously, devopsjesus (Jason Ryberg) wrote…

'Computer'
Is the idea to add ValidationSets and switch cases here with the addition of objects that are supported?

Yes, that was my thought.

johlju

I fixed the review comment today. Thanks for the review @devopsjesus! Ready for review again.

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @devopsjesus)

DSCResources/MSFT_xADObjectEnabledState/MSFT_xADObjectEnabledState.psm1, line 319 at r2 (raw file):

Previously, devopsjesus (Jason Ryberg) wrote…

    $getTargetResourceParameters = @{
        Identity         = $Identity
        ObjectClass      = $ObjectClass
        Enabled          = $Enabled
        DomainController = $DomainController
        Credential       = $Credential
    }

    # Need the @() around this to get a new array to enumerate.
    @($getTargetResourceParameters.Keys) | ForEach-Object {
        if (-not $PSBoundParameters.ContainsKey($_))
        {
            $getTargetResourceParameters.Remove($_)
        }
    }

    $getTargetResourceResult = Get-TargetResource @getTargetResourceParameters

    $compareTargetResourceStateParameters = @{
        CurrentValues = $getTargetResourceResult
        DesiredValues = $PSBoundParameters
        Properties    = @('Enabled')
    }

    $compareTargetResourceStateResult = Compare-ResourcePropertyState @compareTargetResourceStateParameters

This code block is repeated throughout each function (get/set/test), so could you put it in a helper function to reduce repeated code?

Done.

devopsjesus

Reviewable status: 0 of 11 files reviewed, 2 unresolved discussions (waiting on @devopsjesus and @johlju)

Tests/Integration/MSFT_xADObjectEnableState.Integration.Tests.ps1, line 2 at r3 (raw file):

[Microsoft.DscResourceKit.IntegrationTest(OrderNumber = 3)]

There seem to only be two integration tests - am I missing a test, or should OrderNumber = 3 be changed to OrderNumber = 2? Also, it was hard to get this to run with this script attribute, since the module it is downloading has the attribute definition. Should we load the module for the attribute to be available in a initialization script first so the attribute will also load, or am I missing something?

johlju

Reviewable status: 0 of 12 files reviewed, 2 unresolved discussions (waiting on @devopsjesus)

Tests/Integration/MSFT_xADObjectEnableState.Integration.Tests.ps1, line 2 at r3 (raw file):
Done. I have removed this attribute from both tests for now.

Neither were not starting at OrderNumber = 1 since that integration tests should be the one setting up the domain, which does not yet exist. Read more here about the attribute. https://github.com/PowerShell/DscResource.Tests#run-integration-tests-in-order

Also, it was hard to get this to run with this script attribute, since the module it is downloading has the attribute definition. Should we load the module for the attribute to be available in a initialization script first so the attribute will also load, or am I missing something?

I working on getting this working to have a guide on how to run the integration tests. WIP: https://github.com/johlju/xActiveDirectory/tree/add-integ-documentation/Tests
Uses Assert-TestEnvironment.ps1 to load the necessary types
Not sure it is possible to have a Pester integration tests that sets up the domain because of the necessary restart. I have the config ready, must test out the best way to set up domain controllers and verify the result. Once the domain controllers are up it is a small thing running the other integration tests. Thats when the order number comes in. For example the integration tests that set's up a user can later be used by the group integration tests to add that user. THis is to simplify the tests so each test doesn't have to set up a lot of prerequisites.
See for example in SqlServerDsc where we use dependent tests: https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Integration/README.md

johlju · 2019-06-07T07:25:56Z

@devopsjesus this one is ready for review again.

johlju · 2019-06-11T18:47:23Z

@devopsjesus do you have time to look through this again, would love to fix any more potential issues so we can get this merged. 🙂

devopsjesus · 2019-06-11T21:58:34Z

I will try to get to this in the next few days :)

…

On Tue, Jun 11, 2019 at 11:47 AM Johan Ljunggren ***@***.***> wrote: @devopsjesus <https://github.com/devopsjesus> do you have time to look through this again, would love to fix any more potential issues so we can get this merged. 🙂 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#277>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH25XGS6MQYV2RMZ2KREBSDPZ7XL5ANCNFSM4HNXKKWA> .

johlju · 2019-06-12T15:36:24Z

Thank you! 🙇

devopsjesus

Reviewed 4 of 11 files at r1, 4 of 10 files at r3, 6 of 8 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

Tests/Integration/MSFT_xADObjectEnableState.Integration.Tests.ps1, line 2 at r3 (raw file):
I was able to get these tests to run on an already promoted DC.

devopsjesus

Reviewable status: complete! all files reviewed, all discussions resolved

devopsjesus

Reviewable status: complete! all files reviewed, all discussions resolved

johlju · 2019-06-15T10:55:00Z

@devopsjesus thank you so much for the review! 😃

I will merge this as soon as I have merged in another PR.

johlju added the needs review The pull request needs a code review. label May 17, 2019

johlju force-pushed the add-xADObjectEnabledState branch 2 times, most recently from 380050e to 3f496ec Compare May 22, 2019 12:39

johlju commented May 22, 2019

View reviewed changes

johlju force-pushed the add-xADObjectEnabledState branch 2 times, most recently from 8d22b45 to 8229604 Compare May 25, 2019 10:00

johlju commented May 26, 2019

View reviewed changes

johlju force-pushed the add-xADObjectEnabledState branch from 8229604 to d16a1c3 Compare June 4, 2019 12:42

johlju force-pushed the add-xADObjectEnabledState branch from d16a1c3 to df8a0e6 Compare June 4, 2019 16:05

devopsjesus reviewed Jun 4, 2019

View reviewed changes

johlju commented Jun 4, 2019

View reviewed changes

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 Jun 4, 2019

johlju commented Jun 4, 2019

View reviewed changes

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 4, 2019

devopsjesus reviewed Jun 4, 2019

View reviewed changes

johlju commented Jun 5, 2019

View reviewed changes

johlju force-pushed the add-xADObjectEnabledState branch 2 times, most recently from 7dc6099 to 3324b49 Compare June 8, 2019 19:16

Add new resource xADObjectEnabledState

2cd797a

johlju added 7 commits June 13, 2019 18:40

Add resource README.md

2e41fe4

Fix examples

69cf005

Fix typo in example

66bbe93

Remove unnecessary localized string

f639855

Add Compare-TargetResourceState

d19db0e

Remove OrderNumber from integration tests

4b5e7b5

Move Set-DscADComputer as a helper function

6a72208

johlju force-pushed the add-xADObjectEnabledState branch from 3324b49 to 6a72208 Compare June 13, 2019 16:40

devopsjesus approved these changes Jun 14, 2019

View reviewed changes

Merge branch 'dev' into add-xADObjectEnabledState

fc7931e

johlju added ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. and removed needs review The pull request needs a code review. labels Jun 15, 2019

johlju merged commit ec1f69e into dsccommunity:dev Jun 15, 2019

johlju deleted the add-xADObjectEnabledState branch June 15, 2019 12:23

johlju removed the ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. label Jun 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xADObjectEnabledState: New resource #277

xADObjectEnabledState: New resource #277

johlju commented May 17, 2019 •

edited

Loading

codecov-io commented May 17, 2019 •

edited

Loading

johlju commented May 22, 2019

johlju left a comment

johlju left a comment

johlju commented May 26, 2019

johlju commented Jun 4, 2019

devopsjesus left a comment

johlju left a comment

johlju left a comment

devopsjesus left a comment

johlju left a comment

johlju commented Jun 7, 2019

johlju commented Jun 11, 2019

devopsjesus commented Jun 11, 2019 via email

johlju commented Jun 12, 2019

devopsjesus left a comment •

edited

Loading

devopsjesus left a comment

devopsjesus left a comment

johlju commented Jun 15, 2019

xADObjectEnabledState: New resource #277

xADObjectEnabledState: New resource #277

Conversation

johlju commented May 17, 2019 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov-io commented May 17, 2019 • edited Loading

Codecov Report

johlju commented May 22, 2019

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented May 26, 2019

johlju commented Jun 4, 2019

devopsjesus left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

devopsjesus left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Jun 7, 2019

johlju commented Jun 11, 2019

devopsjesus commented Jun 11, 2019 via email

johlju commented Jun 12, 2019

devopsjesus left a comment • edited Loading

Choose a reason for hiding this comment

devopsjesus left a comment

Choose a reason for hiding this comment

devopsjesus left a comment

Choose a reason for hiding this comment

johlju commented Jun 15, 2019

johlju commented May 17, 2019 •

edited

Loading

codecov-io commented May 17, 2019 •

edited

Loading

devopsjesus left a comment •

edited

Loading