BREAKING CHANGE: xADComputer: A computer account can now be created disabled

[johlju]

• Changes to xActiveDirectory
  • Added new helper functions in xADCommon, see each functions comment-based
    help for more information.
    • Convert-PropertyMapToObjectProperties
    • Compare-ResourcePropertyState
    • Test-DscPropertyState
• Changes to xADComputer
  • Refactored the resource and the unit tests.
  • BREAKING CHANGE: The Enabled property is DEPRECATED and is no
    longer set nor enforced with this resource. If this parameter is
    used in a configuration a warning message will be outputted saying
    that the Enabled parameter has been deprecated. The new resource
    xADObjectEnabledState
    can be used to enforce the Enabled property.
  • BREAKING CHANGE: The default value of the enabled property of the
    computer account will be set to the default value of the cmdlet
    New-ADComputer.
  • A new parameter was added called EnabledOnCreation that will control
    if the computer account is created enabled or disabled.
    This parameter does not enforce the property Enabled. To enforce the
    property Enabled see the resource xADObjectEnabledState.
  • Moved examples from the README.md to separate example files in the
    Examples folder.

Fixes #167

Test solved with a wrapper for Set-ADComputer. Not elegant, but it's working. A better approach would have been to create a stub module, see issue #169. But I think that is out of scope for this PR.

This PR cannot be merged until a new resource xADObjectEnabledState is merged (will be a separate PR).

---

This change is 

[codecov-io]

Codecov Report

Merging #170 into dev will increase coverage by 1%.
The diff coverage is 99%.

@@         Coverage Diff          @@
##            dev   #170    +/-   ##
====================================
+ Coverage    89%    90%    +1%     
====================================
  Files        20     20            
  Lines      2033   2208   +175     
  Branches     10     10            
====================================
+ Hits       1810   1995   +185     
+ Misses      213    203    -10     
  Partials     10     10

[johlju]

@PlagueHO would you mind reviewing this one when ever you have a chance?

[PlagueHO]

Sure thing @johlju - I'll get onto this tonight after work!

[PlagueHO]

Just a call out, not something you should fix: there is inconsistent use of ; for line termination in the file. I don't think you should worry about fixing it - and if you were my preference would be not to use ;. It looks like this module could use some work in general to bring it up to HQRM too... one day 😁

Anyway, looking great, only some minor grammatical stuff. Great work as always. Odd about the Pester issue mocking Set-ADComputer.

---

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 18 unresolved discussions.

---

README.md, line 254 at r1 (raw file):

Default a computer account will be created as enabled.
Should be: By default a computer account will be created as enabled.

---

README.md, line 271 at r1 (raw file):

regardless of Enabled parameter was used in a configuration. Now if
Should be: regardless of if theEnabled parameter was used in a configuration. Now if

---

README.md, line 273 at r1 (raw file):

computer account will not be evaluated if it enabled. So if a computer is
Should be: computer account will not be evaluated if it is enabled. So if a computer is

---

README.md, line 274 at r1 (raw file):

disabled, the resource will not enabled it unless Enabled parameter is
Should be: disabled, the resource will not enable it unless the Enabled parameter is

---

README.md, line 277 at r1 (raw file):

CreateDisabled to $true. Default a computer account will be created
Should be: CreateDisabled to $true. By default a computer account will be created

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 246 at r1 (raw file):

    else
    {
        ## Add ensure and enabled as they may not be explicitly passed and we want to enumerate them

No longer adding enabled in here so this should be removed from the comment I think.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 471 at r1 (raw file):

                        adding it as a parameter to the Set-ADComputer cmdlet.
                    #>
                    $setADComputerParams['Enabled'] = $PSBoundParameters.$parameter

To be consistent this line needs a ; (although not using these at all would be my pref). Non-blocking

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 534 at r1 (raw file):

            'Identity' by itself).
        #>
        if ($replaceComputerProperties.Count -or $removeComputerProperties.Count -or $setADComputerParams.Count -gt 1)

How come you're not using $replaceComputerProperties.Count -gt 0 and $removeComputerProperties.Count -gt 0 here? It just looks a bit odd? Non-blocking.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 547 at r1 (raw file):

            Write-Verbose -Message ($LocalizedData.UpdatingADComputer -f $ComputerName);
            Set-DscADComputer -SetADComputerParameters $setADComputerParams

To be consistent this line needs a ; (although not using these at all would be my pref). Non-blocking

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 579 at r1 (raw file):

    param
    (
        [Parameter()]

Shouldn't this be mandatory?

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.schema.mof, line 18 at r1 (raw file):

    [Write, Description("Specifies the full path to the Offline Domain Join Request file to create.")] String RequestFile;
    [Write, ValueMap{"Present", "Absent"},Values{"Present", "Absent"}] String Ensure;
    [Write, Description("Specifies if the the computer account should be created disabled ($true) or enabled ($false). Default a computer account will be created as enabled. If the parameter Enabled is used in the same configuration that will override this parameter.")] Boolean CreateDisabled;

See previous comments about wording of this.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 675 at r1 (raw file):

Should create a computer account that are enabled
Should create a computer account that is enabled

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 691 at r1 (raw file):

Should create a computer account that are enabled
Should create a computer account that is enabled

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 708 at r1 (raw file):

Should create a computer account that are enabled
Should create a computer account that is enabled

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 760 at r1 (raw file):

Should create a computer account that are disabled
Should create a computer account that is disabled

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 777 at r1 (raw file):

Should create a computer account that are disabled
Should create a computer account that is disabled

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 794 at r1 (raw file):

Should create a computer account that are disabled
Should create a computer account that is disabled

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 812 at r1 (raw file):

Should create a computer account that are disabled
Should create a computer account that is disabled

---

Comments from Reviewable

[johlju]

Yes I ignored all the style issue in there, but tried not to make it worse by adding more :)

I updated the issue #169 with more text around the problem I had with mocks.

Added new issue to bring this to HQRM, so someone can start work on it :)

Thanks for reviewing, and helping me with the proof reading! 😄

---

Review status: 1 of 5 files reviewed at latest revision, 18 unresolved discussions.

---

README.md, line 254 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Default a computer account will be created as enabled.
Should be: By default a computer account will be created as enabled.

Done.

---

README.md, line 273 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

computer account will not be evaluated if it enabled. So if a computer is
Should be: computer account will not be evaluated if it is enabled. So if a computer is

Done.

---

README.md, line 274 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

disabled, the resource will not enabled it unless Enabled parameter is
Should be: disabled, the resource will not enable it unless the Enabled parameter is

Done.

---

README.md, line 277 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

CreateDisabled to $true. Default a computer account will be created
Should be: CreateDisabled to $true. By default a computer account will be created

Done.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 246 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

No longer adding enabled in here so this should be removed from the comment I think.

Done.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 471 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

To be consistent this line needs a ; (although not using these at all would be my pref). Non-blocking

Done. Skipping this as I don't want to make it worse. :)

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 534 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

How come you're not using $replaceComputerProperties.Count -gt 0 and $removeComputerProperties.Count -gt 0 here? It just looks a bit odd? Non-blocking.

Done.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 547 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

To be consistent this line needs a ; (although not using these at all would be my pref). Non-blocking

Done. Skipping this as I don't want to make it worse. :)

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 579 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Shouldn't this be mandatory?

Done.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.schema.mof, line 18 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

See previous comments about wording of this.

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 675 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should create a computer account that are enabled
Should create a computer account that is enabled

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 691 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should create a computer account that are enabled
Should create a computer account that is enabled

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 708 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should create a computer account that are enabled
Should create a computer account that is enabled

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 760 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should create a computer account that are disabled
Should create a computer account that is disabled

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 777 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should create a computer account that are disabled
Should create a computer account that is disabled

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 794 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should create a computer account that are disabled
Should create a computer account that is disabled

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 812 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should create a computer account that are disabled
Should create a computer account that is disabled

Done.

---

Comments from Reviewable

[PlagueHO]

Great job @johlju! Once I'm done with the HQRM work on the other 6 repos I maintain I'll look at starting on these 😁

---

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 471 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done. Skipping this as I don't want to make it worse. :)

I completely understand and agree 😁

---

Comments from Reviewable

[johlju]

@PlagueHO That would be great if you had time to bring this to HQRM as well.

[johlju]

@mbreakey3 @kwirkykat do you mind glance over this and merge when you have a moment? Seems we don't have a community maintainer on this repo yet.

[kwirkykat]

@johlju I'm just a little confused on this one. Why not have Enabled = false mean that I want the account to be created as disabled? Is it you want it just created as disabled if it is not there and then not monitored after that? And then the enable/disable status is only monitored and optionally auto-corrected if you specify Enabled?

[johlju]

@kwirkykat and @PlagueHO Yes, exactly so. But starting to questioning my decision now, see below. What are your thoughts on this? Do you think the below would be better, and close this PR?

When creating a Failover Cluster using a prestaged computer account, that prestaged computer account must be disabled for the cluster to use it, if it is not disabled then the New-Cluster cmdlet won't allow to create the cluster.
Since Enabled property is used to monitor/autocorrect the desired state of the Enabled property we can't use it to disable the account, because once the cluster is created, and the account is enabled, it won't be in desired state any more (enabled = $false) and the account would be disabled again, breaking the cluster.

Using both CreateDisabled and Enabled in the same configuration would make Enabled override CreateDisabled. Meaning that in this scenario there is not an option to both create the account disabled, and after that, monitor that the account is enabled (once cluster is created).

Thinking about it now, if we want that last scenario to work as well, then a disabled computer account must be created using a new resource, in for example xFailoverCluster, let say xClusterCno. Then the configuration could look like this.

Configuration Example
{
    node localhost
    {
        xClusterCno CreateDisabledComputerAccount
        {
            Ensure  = 'Present'
            Name    = 'CLUSTER01'
            Enabled = $false # This property is not monitored, and only used during creation.
        }

        xCluster CreateCluster
        {
            Name            = 'CLUSTER01'
            StaticIPAddress = '192.168.100.20/24'

            DependsOn       = '[xClusterCno]CreateDisabledComputerAccount'
        }

        xADComputer MakeSureAccountIsEnabled
        {
            ComputerName = 'CLUSTER01'
            Enabled      = $true

            DependsOn    = '[xCluster]CreateCluster'
        }
    }
}

I'm I thinking correctly here? Happy to create a new resource and close this PR, if that is the better option.

[johlju]

@PlagueHO (and anyone else) can I have your thought on this one (so I'm moving in the right direction)? I think we need a new resource for support all scenarios in my comment above - but I don't think that resource belong in xFailoverCluster (my suggestion of 'xClusterCno')? Mostly because it would feel strange for a resource there needing ActiveDirectory PowerShell cmdlets. Maybe better to create a new resource here in this repo, maybe xADComputerDisabled that are only used to create (and remove) a disabled computer account. All other properties are handled by xADComputer.

Trying to make this better, not just fixing my current problem. 😄

[johlju]

Labeling as abandoned since it has gone 14 days until last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on the PR is taken up again.

[stale]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[brwilkinson]

Is someone able to confirm the desired actions here?

$setADComputerParams['Enabled'] = $PSBoundParameters.$parameter

If I have the following

xADComputer $computeraccount 
{
    ComputerName 			= $computeraccount
    Description  			= "Cluster pre-provision"
    Path         			= $ouname
    Enabled      			= $false
    PsDscRunAsCredential 	= $credlookup["domainjoin"]
}

On the first run, it will create the computer account, in the disabled state. which is desired.

Now if some other DSC code creates a cluster and then enables that account later in the script by the xCluster resource.

Then I run a -useexisitng what is the desired outcome of that execution ?

Do we want the account to stay enabled OR should it always follow the initial specification in the DSC and then disable the account ?

Option a) break the cluster
Option b) skip the test for the Enabled property and also only apply the Enabled setting on new-adcomputer, not set-adcomputer

[iainbrighton]

@brwilkinson this line $setADComputerParams['Enabled'] = $PSBoundParameters.$parameter sets the Enabled key on the $sertADComputerParams hashtable to whatever was passed in via the Enabled parameter. You could change it to $setADComputerParams['Enabled'] = $Enabled and it'd do the same thing.

Surely, we should change the functionality to only explicitly set the Enabled property using New-ADComputer/Set-ADComputer when it is explicitly passed? The Test-TargetResource function would then only fail if the current state doesn't match and we've explicitly asked for the account to be enabled or disabled. This is still a breaking change though.

I think this would resolve your issue? If you didn't specify the Enabled property in your configuration, the computer account would be created disabled -as you want it. When the computer account is enabled during the cluster creation, the Test-TargetResource function would pass still as it wouldn't be checking the Enabled property as it's not defined in your configuration.

I hope this makes sense?

[johlju]

@iainbrighton I think you suggestion only works if the New-ADComputer cmdlet creates the computer accounts disabled by default, but it does not. :/
This New-ADComputer -Name 'MY-COMP" -SamAccountName 'MY-COMP' will create the account enabled, but this New-ADComputer -Name "USER02-SRV3" -SamAccountName "USER02-SRV3" -Enabled:$false will create the account disabled.

• If the Enabled property is not used in the configuration it should not be evaluated.
• If Enabled property is used in the configuration it should be evaluated and be changed to the desired state if it transitions to a non desired state.

With the CreateDisabled property my thought was to handle the Enabled property on the New-ADComputer cmdlet. If CreateDisabled is set to $true it will create the account disabled. If the Enabled property is left out of the configuration, and only CreateDisabled is used, it should not evaluate the Enabled property of the computer account (not sure if the PR does this already).

<# 
    1. The account will be created disabled, and Enabled property will not
    be evaluated on second run.
#>
xADComputer 'CreateDisabledAndEnabledPropertyAreNotEvaluated'
{
    ComputerName   = 'SRV-DISABLED'
    CreateDisabled = $true
}

<# 
    2. The account will be created disabled, and 
    the account will always be disabled on next runs. 
#>
xADComputer 'CreateDisabledAndDesiredStateDisabled'
{
    ComputerName   = 'SRV-DISABLED'
    Enabled        = $false
    CreateDisabled = $true
}

<# 
    3. The account will be created disabled, and 
    the account will always be enabled on next runs.
#>
xADComputer 'CreateDisabledAndDesiredStateEnabled'
{
    ComputerName   = 'SRV-DISABLED'
    # The Enabled parameter is overridden by the parameter CreateDisabled when creating the account.
    Enabled        = $true 
    CreateDisabled = $true
}

<# 
    4. The account will be created enabled, and on the next runs
    it will be enabled (if disabled).
#>
xADComputer 'CreateEnabledAndDesiredStateEnabled'
{
    ComputerName   = 'SRV-DISABLED'
    Enabled        = $true 
}

<# 
    5. The account will be created enabled, and on the next runs
    it will be enabled (if disabled).
#>
xADComputer 'CreateEnabledAndDesiredStateEnabled'
{
    ComputerName   = 'SRV-DISABLED'
    Enabled        = $true 
    # Same as previous, the CreateDisabled parameter will be ignored.
    CreateDisabled = $false
}

[johlju]

The problem arises when a user want to both create an account disabled, but also evaluate the Enabled property if the account transitions to disabled.

If using the example 3 above, that will create the account disabled. But on the second run it will enable the account. If the cluster was not created before the second run, for example because of a restart of the node (but could be because of any number of reasons), the cluster creating will fail because the computer account has been enabled.

xADComputer 'CreateDisabledAndDesiredStateEnabled'
{
    ComputerName   = 'SRV-DISABLED'
    Enabled        = $true 
    # The CreateDisabled parameter is overridden by the parameter Enabled in this scenario.
    CreateDisabled = $true
}

Instead my thought is if there is a need for a new resource xADComputerDisabled. The scenario below the resource xADComputerDisabled creates the account disabled. But the Enabled property is not evaluated by xADComputer until all it's dependencies have been meet. That resource won't run until all other dependencies have been successfully configured. Meaning a number of reasons can happen between the account being created, until the Enabled property is evaluated.

# NEW RESOURCE xADComputerDisabled
xADComputerDisabled 'CreateDisabledComputerAccount'
{
    Name    = 'CLU_CNO01'
    # This property is not monitored, and only used during creation.
    Enabled = $false 
}

xCluster 'CreateCluster'
{
    Name            = 'CLU_CNO01'
    StaticIPAddress = '192.168.100.20/24'

    DependsOn       = '[xClusterCno]CreateDisabledComputerAccount'
}

xADComputer 'MakeSureAccountIsEnabled'
{
    ComputerName = 'CLU_CNO01'
    Enabled      = $true

    DependsOn    = '[xCluster]CreateCluster'
}

[brwilkinson]

@johlju @iainbrighton

Is there ever a time when you would want to Disable an active computer account in AD ?

I can't really think of one . . .

I feel like if you are running new-adcomputer then make it enabled by default and disabled if the user requested. However if you are running set-adcomputer, then never apply the enabled setting. . . remove it from the params when calling that cmdlet.

[johlju]

However if you are running set-adcomputer, then never apply the enabled setting. . . remove it from the params when calling that cmdlet.

Not sure we can do that, because if the computer account would be disabled by other means, the account would never be enabled again if the users intention is that the desired state should be enabled.

Is there ever a time when you would want to Disable an active computer account in AD ?
I can't really think of one . . .

Edge cases I guess. One scenario I can think of is that there is an old computer account that is inactive in AD but aren't allowed to be deleted, and it should never be enabled again. Should we remove the ability to disabled accounts? That's more than a breaking change, that is removal of functionality. 🤔

[johlju]

Is there a way to support all scenarios without a new resource xADComputerDisabled?

[PlagueHO]

Quite an epic adventure on this one!

So general notes: It would really be good to move this resource to automatic documentation.

In the interest of getting this one through I'll just omit commenting on any existing issues and assume we'll fix in future PRs.

Just some minor tweaks and some FYIs you can ignore. Once you've tweaked, let me know and I'll complete the review and we can merge.

Reviewed 22 of 22 files at r3.
Reviewable status: all files reviewed, 34 unresolved discussions (waiting on @johlju)

---

CHANGELOG.md, line 17 at r3 (raw file):

https://github.com/PowerShell/xActiveDirectory#xadobjectenabledstate

This link does not appear to work (e.g. does not jump to the anchor.

---

README.md, line 44 at r3 (raw file):

* [**xADComputer**](#xadcomputer) creates and manages Active Directory computer accounts.
* **xADDomain** creates new Active Directory forest configurations and new Active Directory domain configurations.

Is it possible to add a link for all resources?

---

README.md, line 100 at r3 (raw file):

  see the parameter `EnabledOnCreation` in this resource, and the resource
  [xADObjectEnabledState](#xadobjectenabledstate) on how to enforce the
  `Enabled` property. _This parameter no longer sets nor enforce the_

sets not enforce -> sets or enforces

---

README.md, line 325 at r3 (raw file):

* **Ensure**: Indicates if the access will be added (Present) or will be removed (Absent). Default is 'Present'.
* **Path**: Active Directory path of the object, specified as a Distinguished Name.

FYI, Comma not needed.

---

README.md, line 326 at r3 (raw file):

* **Ensure**: Indicates if the access will be added (Present) or will be removed (Absent). Default is 'Present'.
* **Path**: Active Directory path of the object, specified as a Distinguished Name.
* **IdentityReference**: Indicates the identity of the principal for the ace. Use the notation DOMAIN\SamAccountName for the identity.

ace -> ACE

---

README.md, line 342 at r3 (raw file):

* **`[Boolean]` ProtectedFromAccidentalDeletion** _(Write)_: Valid values are $true and $false. If not specified, it defaults to $true.
* **`[String]` Ensure** _(Write)_: Specifies whether the OU is present or absent. Valid values are 'Present' and 'Absent'. It not specified, it defaults to 'Present'.
* **`[PSCredential]` Credential** _(Write)_: User account credentials used to perform the operation. Note: _if not running on a domain controller, this is required_.

Reverse: This is required if not running on a domain controller.

---

README.md, line 343 at r3 (raw file):

* **`[String]` Ensure** _(Write)_: Specifies whether the OU is present or absent. Valid values are 'Present' and 'Absent'. It not specified, it defaults to 'Present'.
* **`[PSCredential]` Credential** _(Write)_: User account credentials used to perform the operation. Note: _if not running on a domain controller, this is required_.
* **`[Boolean]` RestoreFromRecycleBin** _(Write)_: Indicates whether or not the organizational unit should first tried to be restored from the recycle bin before creating a new organizational unit.

Maybe rewrite as: Try to restore the organizational unit from the recycle bin before creating a new one.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1 at r3 (raw file):

$script:resourceModulePath = Split-Path -Path (Split-Path -Path $PSScriptRoot -Parent) -Parent

FYI, couldn't this entire module combined with the other common functions? Perhaps for another PR though.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 7 at r3 (raw file):

Import-Module -Name (Join-Path -Path $script:localizationModulePath -ChildPath 'xActiveDirectory.Common.psm1')

data localizedString

FYI, is it possible to move out into a localization file?

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 34 at r3 (raw file):

        AddingGroupMember              = Adding member '{0}' from domain '{1}' to AD group '{2}'. (ADCOMMON0030)

        WasExpectingDomainController     = The operating system product type code returned 2, which indicates that this is domain controller, but was unable to retrieve the domain controller object. (ADCOMMON0001)

Is it possible to move these 10 to the top so that the codes are in order?

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1047 at r3 (raw file):

<#
    .SYNOPSIS
        Converts a hashtable, containing the parameter to property mappings, to

Commas not needed.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1052 at r3 (raw file):

    .PARAMETER PropertyMap
        The property map, as an array of hashtable, to convert to an properties array.

hashtable -> hashtables
to convert to an -> to convert to a

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1066 at r3 (raw file):

        $computerObjectProperties = Convert-PropertyMapToObjectProperties $computerObjectPropertyMap
         $getADComputerResult = Get-ADComputer -Identity 'APP01' -Properties $computerObjectProperties

Indent issue.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1086 at r3 (raw file):

        if ($property -isnot [System.Collections.Hashtable])
        {
            $errorMessage = 'An object in the property map array is not of the type [System.Collections.Hashtable].'

This should be in localization strings.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1224 at r3 (raw file):

    .SYNOPSIS
        This function is used to compare the current and the desired value of a
        property

End with a full stop.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1268 at r3 (raw file):

                $arrayCompare = Compare-Object @compareObjectParameters
                if ($null -ne $arrayCompare)

Needs blank line before.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 475 at r3 (raw file):

            $compareTargetResourceStateResult = Compare-ResourcePropertyState @compareTargetResourceStateParameters
            if ($false -in $compareTargetResourceStateResult.InDesiredState)

Add blank line before line.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 891 at r3 (raw file):

        foreach ($property in $propertiesNotInDesiredState)
        {
            $computerAccountPropertyName = ($script:computerObjectPropertyMap | Where-Object {

Where-Object needs -FilterScript

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 944 at r3 (raw file):

    {
        # User exists and needs removing
        Write-Verbose (

Needs parameter name -Message

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 981 at r3 (raw file):

<#
    .SYNOPSIS
        This is evaluates the service principal names current state against the

This is evaluates -> this evaluates

---

Examples/Resources/xADComputer/2-AddComputerAccountDisabled_Config.ps1, line 22 at r3 (raw file):

<#
    .DESCRIPTION
        This configuration will create a Active Directory computer account

a -> an

---

Examples/Resources/xADComputer/3-AddComputerAccountSpecificPath_Config.ps1, line 22 at r3 (raw file):

<#
    .DESCRIPTION
        This configuration will create a Active Directory computer account

a -> an

---

Examples/Resources/xADComputer/4-AddComputerAccountAndCreateODJRequest_Config.ps1, line 22 at r3 (raw file):

<#
    .DESCRIPTION
        This configuration will create a Active Directory computer account

a -> an

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 3 at r3 (raw file):

# Make sure this is run in the correct order.
[Microsoft.DscResourceKit.IntegrationTest(OrderNumber = 2)]
param()

space after param

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 100 at r3 (raw file):

            It 'Should return $true when Test-DscConfiguration is run' {
                Test-DscConfiguration -Verbose | Should -Be 'True'

Should -BeTrue

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 147 at r3 (raw file):

            It 'Should return $true when Test-DscConfiguration is run' {
                Test-DscConfiguration -Verbose | Should -Be 'True'

Should -BeTrue

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 194 at r3 (raw file):

            It 'Should return $true when Test-DscConfiguration is run' {
                Test-DscConfiguration -Verbose | Should -Be 'True'

Should -BeTrue

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 241 at r3 (raw file):

            It 'Should return $true when Test-DscConfiguration is run' {
                Test-DscConfiguration -Verbose | Should -Be 'True'

Should -BeTrue

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 292 at r3 (raw file):

            It 'Should return $true when Test-DscConfiguration is run' {
                Test-DscConfiguration -Verbose | Should -Be 'True'

Should -BeTrue

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 356 at r3 (raw file):

            It 'Should return $true when Test-DscConfiguration is run' {
                Test-DscConfiguration -Verbose | Should -Be 'True'

Should -BeTrue

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 387 at r3 (raw file):

            It 'Should return $true when Test-DscConfiguration is run' {
                Test-DscConfiguration -Verbose | Should -Be 'True'

Should -BeTrue

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1032 at r3 (raw file):

            }

            Context 'When a property map contain a wrong type' {

contain a wrong type -> contains a wrong type

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1046 at r3 (raw file):

                    {
                        Convert-PropertyMapToObjectProperties $propertyMapValue
                    } | Should -Throw 'An object in the property map array is not of the type [System.Collections.Hashtable].'

Should use Localization string.

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1052 at r3 (raw file):

        Describe 'DscResource.Common\Test-DscPropertyState' -Tag 'TestDscPropertyState' {
            Context -Name 'When passing values' {

FYI, I think we should probably have a context block around each postive/negative pair of tests. E.g. Context 'When comparing tables'

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1281 at r3 (raw file):

            }

            Context 'When passing just two properties and one property is not in desired state' {

'just' not required.

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1313 at r3 (raw file):

            }

            Context 'When passing a common parameter in desired values' {

in desired values -> set to desired value

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 19 at r3 (raw file):

Import-Module -Name (Join-Path -Path $script:moduleRoot -ChildPath (Join-Path -Path 'DSCResource.Tests' -ChildPath 'TestHelper.psm1')) -Force

# TODO: Insert the correct <ModuleName> and <ResourceName> for your resource

Can we remove this TODO?

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 714 at r3 (raw file):

                }

                Context 'When the computer account is absent in Active Directory' {

absent in -> absent from

[johlju]

Reviewable status: 10 of 27 files reviewed, 34 unresolved discussions (waiting on @PlagueHO)

---

CHANGELOG.md, line 17 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

https://github.com/PowerShell/xActiveDirectory#xadobjectenabledstate

This link does not appear to work (e.g. does not jump to the anchor.

Done. This will be resolved once PR #277 is merged (waiting for this PR)

---

README.md, line 44 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is it possible to add a link for all resources?

Done. Added an issue to track this, we can fix this in another PR.

---

README.md, line 100 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

sets not enforce -> sets or enforces

Done.

---

README.md, line 326 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

ace -> ACE

Done.

---

README.md, line 342 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Reverse: This is required if not running on a domain controller.

Done.

---

README.md, line 343 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Maybe rewrite as: Try to restore the organizational unit from the recycle bin before creating a new one.

Done. Fixed this in all resources so it is not forgotten. Though, I'm ignoring missing comment-based help and other issues. Let's clean up a resource at a time. 😃

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

FYI, couldn't this entire module combined with the other common functions? Perhaps for another PR though.

Agree, and thought about it, but haven't gotten around to submit an issue. Added an issue to track this now.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 7 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

FYI, is it possible to move out into a localization file?

Let's fix this in resource cleanup (this particular file I have mentioned in the issue a create above so it can be fixed there).

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 34 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is it possible to move these 10 to the top so that the codes are in order?

Done.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1047 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Commas not needed.

Done.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1052 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

hashtable -> hashtables
to convert to an -> to convert to a

Done.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1066 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Indent issue.

Done.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1086 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This should be in localization strings.

Done.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1224 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

End with a full stop.

Done.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1268 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Needs blank line before.

Done.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 475 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add blank line before line.

Done.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 891 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Where-Object needs -FilterScript

Done.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 944 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Needs parameter name -Message

Done.

---

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 981 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This is evaluates -> this evaluates

Done.

---

Examples/Resources/xADComputer/2-AddComputerAccountDisabled_Config.ps1, line 22 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

a -> an

Done.

---

Examples/Resources/xADComputer/3-AddComputerAccountSpecificPath_Config.ps1, line 22 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

a -> an

Done.

---

Examples/Resources/xADComputer/4-AddComputerAccountAndCreateODJRequest_Config.ps1, line 22 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

a -> an

Done.

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 3 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

space after param

Done.

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 100 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should -BeTrue

Done.

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 147 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should -BeTrue

Done.

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 194 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should -BeTrue

Done.

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 241 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should -BeTrue

Done.

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 292 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should -BeTrue

Done.

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 356 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should -BeTrue

Done.

---

Tests/Integration/MSFT_xADComputer.Integration.Tests.ps1, line 387 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should -BeTrue

Done.

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1032 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

contain a wrong type -> contains a wrong type

Done.

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1046 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should use Localization string.

Done.

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1052 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

FYI, I think we should probably have a context block around each postive/negative pair of tests. E.g. Context 'When comparing tables'

Done

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1281 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

'just' not required.

Done.

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1313 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

in desired values -> set to desired value

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 19 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we remove this TODO?

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 714 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

absent in -> absent from

Done.

[johlju]

Quite an epic adventure on this one!

For sure! It's gonna be great to get this one through! 😄

So general notes: It would really be good to move this resource to automatic documentation.

Added an issue for that! 👍

[johlju]

@PlagueHO Ready for review again. I fixed most, and added issues for a few to track and fix in another PR.

I did the changes blindfolded so I wouldn't see all the other style guideline issues in the rest of the code, 😆 Let's fix that in another PR. If you see a problem in xADComputer related files then please comment on them, I want them to be a baseline for the rest of the work getting this to HQRM.

[PlagueHO]

@johlju - cool! Will finish the review tonight!!

[PlagueHO]

Great work @johlju

Reviewed 17 of 17 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[johlju]

@PlagueHO Thank you for the review! Thank you @devopsjesus for running this in your lab!