DomainMode and ForestMode implemented

[nyanhp]

Implemented two new optional parameters DomainMode and ForestMode that will be used with the Install-ADDSDomain and Install-ADDSForest cmdlets at the moment.

Raising the domain or forest functional level is not done with this resource. Thus the new resource keys are not tested for in Test-TargetResource.

There is no additional validation being done with regards to the domain and forest functional level. The limitations of both should already be known to the users (i.e. no downgrades, fitting FFL and DFL, ...)

Fixes #187

---

This change is 

[codecov-io]

Codecov Report

Merging #199 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #199    +/-   ##
====================================
+ Coverage    81%    81%   +<1%     
====================================
  Files        14     14            
  Lines      1214   1238    +24     
  Branches     10     10            
====================================
+ Hits        990   1014    +24     
  Misses      214    214            
  Partials     10     10

[johlju]

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

a discussion (no related file):

• Could you please add an descriptive entry, for each change/issue, to the Unreleased section of the change log in the file README.md? If an entry resolves an issue please reference the issue where applicable. You may suffix (it's optional) each entry with your name. Example of the format for the entry.

* Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account)

---

a discussion (no related file):
There are no examples in this resource module yet. Could you please add an example containing this change, the test framework will test the example by compiling it.

See folder structure and example here: https://github.com/PowerShell/SqlServerDsc/blob/dev/Examples/Resources/SqlServerDatabaseMail/1-EnableDatabaseMail.ps1

---

a discussion (no related file):

Raising the domain or forest functional level is not done with this resource. Thus the new resource keys are not tested for in Test-TargetResource.

Could you add an issue for that so it's being tracked.

---

a discussion (no related file):

There is no additional validation being done with regards to the domain and forest functional level. The limitations of both should already be known to the users (i.e. no downgrades, fitting FFL and DFL, ...)

Could you add that to the resource description? At line 89 - there are no description yet, so please add this as a note or similar so that the user can read this information and a link to where the user can find it.

---

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

* **LogPath**: Specifies the fully qualified, non-UNC path to a directory on a fixed disk of the local computer where the log file for this operation will be written (optional).
* **SysvolPath**: Specifies the fully qualified, non-UNC path to a directory on a fixed disk of the local computer where the Sysvol file will be written. (optional)
* **ForestMode**: Specifies the forest mode if a new forest is deployed. ForestMode will not be raised by this resource for existing forests. (optional)

Could we add the possible values after the description as { Value1 | Value 2 | ... }?

---

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

* **SysvolPath**: Specifies the fully qualified, non-UNC path to a directory on a fixed disk of the local computer where the Sysvol file will be written. (optional)
* **ForestMode**: Specifies the forest mode if a new forest is deployed. ForestMode will not be raised by this resource for existing forests. (optional)
* **DomainMode**: Specifies the domain mode if a new domain is deployed. DomainMode will not be raised by this resource for existing domains. (optional)

Same as previous comment.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 137 at r1 (raw file):

[uint16]$

[UInt16] $. See case and space before $. Same for the one below.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 137 at r1 (raw file):

            ParentDomainName = $domain.ParentDomain;
            DomainNetBIOSName = $domain.NetBIOSName;
            ForestMode = [uint16]$forest.ForestMode

Should this return the same value as is written in the configuration 'Win2008' etc, now it will just return a value? Returning the correct value (written in the configuration) would be beneficial when used in a ReverseDSC scenario.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 138 at r1 (raw file):

            DomainNetBIOSName = $domain.NetBIOSName;
            ForestMode = [uint16]$forest.ForestMode
            DomainMode = [uint16]$domain.DomainMode

Should this return the same value as is written in the configuration 'Win2008' etc, now it will just return a value? Returning the correct value (written in the configuration) would be beneficial when used in a ReverseDSC scenario.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 74 at r1 (raw file):

                Mock Get-ADDomain -ParameterFilter { $Credential -eq $null } { [psobject]@{Forest = $correctDomainName}}

                $result = Get-TargetResource @testDefaultParams -DomainName $correctDomainName;

Can we evaluate the correct values here? (at least those properties you are adding)

$result.ForestMode | Should -Be 'Win2008'
... etc

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 76 at r1 (raw file):

                $result = Get-TargetResource @testDefaultParams -DomainName $correctDomainName;

                Assert-MockCalled Get-ADDomain -ParameterFilter { $Credential -eq $null } -Scope It;

We should add a Assert-MockCalled for Get-ADForest too.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 84 at r1 (raw file):

                Mock Get-ADForest -ParameterFilter { $Credential -eq $null } {  }

                $result = Get-TargetResource @testDefaultParams -DomainName $correctDomainName;

Can we evaluate the correct values here? (at least those properties you are adding)

$result.ForestMode | Should -Be 'Win2008'
... etc

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 86 at r1 (raw file):

                $result = Get-TargetResource @testDefaultParams -DomainName $correctDomainName;

                Assert-MockCalled Get-ADDomain -ParameterFilter { $Credential -eq $null } -Scope It;

We should add a Assert-MockCalled for Get-ADForest too.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 218 at r1 (raw file):

7

Could we add a comment what '7' means?

---

Comments from Reviewable

[nyanhp]

Review status: 0 of 7 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

• Could you please add an descriptive entry, for each change/issue, to the Unreleased section of the change log in the file README.md? If an entry resolves an issue please reference the issue where applicable. You may suffix (it's optional) each entry with your name. Example of the format for the entry.

* Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account)

Done.

---

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Raising the domain or forest functional level is not done with this resource. Thus the new resource keys are not tested for in Test-TargetResource.

Could you add an issue for that so it's being tracked.

Done.

---

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

There is no additional validation being done with regards to the domain and forest functional level. The limitations of both should already be known to the users (i.e. no downgrades, fitting FFL and DFL, ...)

Could you add that to the resource description? At line 89 - there are no description yet, so please add this as a note or similar so that the user can read this information and a link to where the user can find it.

Done.

---

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

There are no examples in this resource module yet. Could you please add an example containing this change, the test framework will test the example by compiling it.

See folder structure and example here: https://github.com/PowerShell/SqlServerDsc/blob/dev/Examples/Resources/SqlServerDatabaseMail/1-EnableDatabaseMail.ps1

I added two examples how to create a domain in a new forest and how to create a child domain.

---

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

Previously, johlju (Johan Ljunggren) wrote…

Could we add the possible values after the description as { Value1 | Value 2 | ... }?

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

Same as previous comment.

Done.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 137 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[uint16]$

[UInt16] $. See case and space before $. Same for the one below.

Done.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 137 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should this return the same value as is written in the configuration 'Win2008' etc, now it will just return a value? Returning the correct value (written in the configuration) would be beneficial when used in a ReverseDSC scenario.

Done.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 138 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should this return the same value as is written in the configuration 'Win2008' etc, now it will just return a value? Returning the correct value (written in the configuration) would be beneficial when used in a ReverseDSC scenario.

I see... I have not thought about that. I have added a new function to cast the ForestMode/DomainMode to the type that is expected by the ADDSDeployment cmdlets so that we can wrap it in a test. My main problem was that the integer values returned were always correct while the enumerations for the Install-* and Get-* cmdlets had different types.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 74 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we evaluate the correct values here? (at least those properties you are adding)

$result.ForestMode | Should -Be 'Win2008'
... etc

Done.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 76 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should add a Assert-MockCalled for Get-ADForest too.

Done.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 84 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we evaluate the correct values here? (at least those properties you are adding)

$result.ForestMode | Should -Be 'Win2008'
... etc

Done.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 86 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should add a Assert-MockCalled for Get-ADForest too.

Done.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 218 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

7

Could we add a comment what '7' means?

Replaced by string values due to changes in the resource itself. I hope WinThreshold is explanatory enough, otherwise I'll add a comment.

---

Comments from Reviewable

[nyanhp]

Hi @johlju, thank you for reviewing! I hope I adressed all issues.

[johlju]

Looking good, thanks for the examples! Just a few comments left. 🙂

---

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

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

deployment with this resource the restrictions

Seems to me the phrase is cut short? Which restrictions? 🙂

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.ps1, line 604 at r2 (raw file):

}

function ConvertTo-DeploymentForestMode

We should add a unit test for this helper function https://github.com/PowerShell/xActiveDirectory/blob/dev/Tests/Unit/MSFT_xADCommon.Tests.ps1

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.ps1, line 632 at r2 (raw file):

    }

    $convertedMode = [Microsoft.DirectoryServices.Deployment.Types.ForestMode] $Mode

Should it be $ModeId here?

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.ps1, line 644 at r2 (raw file):

}

function ConvertTo-DeploymentDomainMode

We should add a unit test for this helper function https://github.com/PowerShell/xActiveDirectory/blob/dev/Tests/Unit/MSFT_xADCommon.Tests.ps1

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 98 at r2 (raw file):

        [String] $SysvolPath,

        [Parameter()] [ValidateSet("Win2008", "Win2008R2", "Win2012", "Win2012R2", "WinThreshold")]

We should use single quotes were applicable. Throughout.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 101 at r2 (raw file):

        [String] $ForestMode,

        [Parameter()] [ValidateSet("Win2008", "Win2008R2", "Win2012", "Win2012R2", "WinThreshold")]

We should use single quotes were applicable. Throughout.

---

Examples/Resources/xADDomain/1-NewForest.ps1, line 36 at r2 (raw file):

PSDesiredStateConfiguration

We should use PSDscResources here instead,

---

Examples/Resources/xADDomain/2-NewChildDomain.ps1, line 37 at r2 (raw file):

    )

    Import-DscResource -ModuleName PSDesiredStateConfiguration

We should use PSDscResources here instead,

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 74 at r1 (raw file):

Previously, nyanhp (Jan-Hendrik Peters [MSFT]) wrote…

Done.

Not seeing this change?

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 84 at r1 (raw file):

Previously, nyanhp (Jan-Hendrik Peters [MSFT]) wrote…

Done.

Not seeing this change?

---

Comments from Reviewable

[johlju]

A recent PR removed all helper functions from MSFT_xADCommon.ps1 to MSFT_xADCommon.psm1.

There are merge conflicts since your PR was not based on the latest changes. Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history). If you don't know how to rebase, please look at how to Resolve merge conflicts.
Let me know if you need any assistance.

[nyanhp]

Rebased and added changes to address additional comments

---

Review status: 2 of 14 files reviewed, 10 unresolved discussions (waiting on @johlju and @nyanhp)

---

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

Previously, johlju (Johan Ljunggren) wrote…

deployment with this resource the restrictions

Seems to me the phrase is cut short? Which restrictions? 🙂

Done.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.ps1, line 604 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should add a unit test for this helper function https://github.com/PowerShell/xActiveDirectory/blob/dev/Tests/Unit/MSFT_xADCommon.Tests.ps1

Cannot add a unit test for functions calling .NET types that don't likely exist on every build system unless I am mistaken and pester can do this now.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.ps1, line 632 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should it be $ModeId here?

Thanks! Done.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.ps1, line 644 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should add a unit test for this helper function https://github.com/PowerShell/xActiveDirectory/blob/dev/Tests/Unit/MSFT_xADCommon.Tests.ps1

Cannot add a unit test for functions calling .NET types that don't likely exist on every build system unless I am mistaken and pester can do this now.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 98 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use single quotes were applicable. Throughout.

Done (for the other occurrences I found as well.

---

DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1, line 101 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use single quotes were applicable. Throughout.

Done (for the other occurrences I found as well.

---

Examples/Resources/xADDomain/1-NewForest.ps1, line 36 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

PSDesiredStateConfiguration

We should use PSDscResources here instead,

Done.

---

Examples/Resources/xADDomain/2-NewChildDomain.ps1, line 37 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use PSDscResources here instead,

Done.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 74 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Not seeing this change?

I added a new test case for that: It 'Calls "Get-ADForest" without credentials if domain member' where I mocked the call to Get-ADForest. Was that not right? It seemed more prudent to test this separately since the first test case was specifically to test Get-ADDomain

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 84 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Not seeing this change?

I added the new test cases instead of testing this in an existing case:
It 'Returns the correct domain mode'
It 'Returns the correct forest mode'
It 'Does not return $null as forest or domain mode'
It 'Calls ConvertTo-DeploymentForestMode and ConvertTo-DeploymentDomainMode'

---

Comments from Reviewable

[nyanhp]

@johlju done. I am unsure about the Unit Tests for the new functions. I am doing a couple of type conversions, and as far as I know, I cannot test that in pester without adding another function doing the actual conversion that I later mock - but that just feels wrong.

I am open to suggestions!

[johlju]

I see a lot of changes in files that is part of an earlier merged PR. Could you rebase to update ?

Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history). If you don't know how to rebase you local dev and working branch, please look at how to Resolve merge conflicts.
Let me know if you need any assistance.

---

Reviewed 3 of 4 files at r4.
Review status: 5 of 14 files reviewed, 6 unresolved discussions (waiting on @johlju)

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.ps1, line 604 at r2 (raw file):

Previously, nyanhp (Jan-Hendrik Peters [MSFT]) wrote…

Cannot add a unit test for functions calling .NET types that don't likely exist on every build system unless I am mistaken and pester can do this now.

Maybe we can make a stub of the class? Like this https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Unit/Stubs/SqlPowerShellSqlExecutionException.cs

That is then loaded using Add-Type in the test.

---

DSCResources/MSFT_xADCommon/MSFT_xADCommon.ps1, line 644 at r2 (raw file):

Previously, nyanhp (Jan-Hendrik Peters [MSFT]) wrote…

Cannot add a unit test for functions calling .NET types that don't likely exist on every build system unless I am mistaken and pester can do this now.

See previous comment.

---

Comments from Reviewable

[nyanhp]

@johlju , that was an excellent hint, thanks! Added that as well.

Rebasing was hell on earth, next time I'd rather open a new pull request... But I guess I have to get used to it.

[johlju]

@nyanhp Normally it fast to rebase, I think this time there was more difficult because a PR removed a file that you were updating replacing it with a new one. Normally it's just the change log that has conflicts. 🙂 In normal circumstances it takes ~a minute to rebase.

[johlju]

@nyanhp Awesome work on the stubs! Just minor comments now, then I think it's ready to go!

---

Reviewed 1 of 7 files at r2, 1 of 10 files at r3, 1 of 2 files at r5, 5 of 5 files at r6.
Review status: all files reviewed, 17 unresolved discussions (waiting on @nyanhp)

---

README.md, line 90 at r6 (raw file):

### **xADDomain**

The xADDomain resource creates a new domain in a new forest or a child domain in an existing forest. While it is possible to set the forest functional level and the domain functional level during deployment with this resource the common restrictions apply. For more information see [TechNet](https://docs.microsoft.com/en-us/windows-server/identity/ad-ds/active-directory-functional-levels)

Can we end the last sentence with full stop (.)?

---

README.md, line 107 at r6 (raw file):

(optional)

Can we move (Optional) to the beginning of the first sentence?

---

README.md, line 109 at r6 (raw file):

(optional)

Can we move (Optional) to the beginning of the first sentence?

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 535 at r6 (raw file):

Should Be Win2012

Should this be (to be more clear what's tested)?

| Should Be [Microsoft.DirectoryServices.Deployment.Types.ForestMode]::Win2012

Throughout.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 74 at r1 (raw file):

Previously, nyanhp (Jan-Hendrik Peters [MSFT]) wrote…

I added a new test case for that: It 'Calls "Get-ADForest" without credentials if domain member' where I mocked the call to Get-ADForest. Was that not right? It seemed more prudent to test this separately since the first test case was specifically to test Get-ADDomain

Sorry, missed those tests! 🙂

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 39 at r6 (raw file):

$mode

Can we split this up into two

$forestMode = 'WinThreshold'
$domainMode = $forestMode

Change $mode appropriately in the code below.

Easier when reviewing to know what a variable contains, I was thrown that forest mode and domain mode would equal the same value in the tests (so had to search where this value was assigned). Splitting these up also helps testing (later) when domain mode and forest mode is not equal.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 54 at r6 (raw file):

            Mock Assert-Module -ParameterFilter { $ModuleName -eq 'ADDSDeployment' } { }
            function ConvertTo-DeploymentForestMode { }

Not sure these are needed? They should already be known to Pester because the resource is importing the helper functions? Does it fail if this and the one below are removed?
We should (normally) only need to make stubs for cmdlets/functions that are not present.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 56 at r6 (raw file):

Mock ConvertTo-DeploymentDomainMode { return $mode }

We should use named parameters, something like this. *You don't have to do the other (not part of your change) that not using the correct style, but lets start with those you adding. But wouldn't mind if all are fixed. 🙂 *

Mock -CommandName ConvertTo-DeploymentDomainMode -MockWith {
    return $mode 
}

Throughout.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 61 at r6 (raw file):

{ }

Not necessary when no logic is mocked. This should be enough

Mock -CommandName Get-ADForest

Throughout.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 127 at r6 (raw file):

                (Get-TargetResource @testDefaultParams -DomainName $correctDomainName).DomainMode | Should Be $mode

                Assert-MockCalled -CommandName ConvertTo-DeploymentDomainMode

We are just testing if the helper function is called (one or more times), could we add ParameterFilter to this so it is called with the expected values? Maybe that is not relevant?
Also, maybe we should test that it only called X number of times? -Exactly -Times 1.

Throughout.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 138 at r6 (raw file):

            }

            It 'Does not return $null as forest or domain mode' {

We should also have a test that actually returns $null for those (when absent?).

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 141 at r6 (raw file):

$result.DomainMode | Should Not Be $null

Can we instead test for the correct value is returned? Not sure what the difference is from the previous test 'Returns the correct domain mode'?

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 142 at r6 (raw file):

$result.ForestMode | Should Not Be $null

Can we instead test for the correct value is returned? Not sure what the difference is from the previous test 'Returns the correct forest mode'?

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 355 at r6 (raw file):

-Scope It;

No need for semi-colon at the end.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 363 at r6 (raw file):

-Scope It;

No need for semi-colon at the end.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 472 at r6 (raw file):

-Scope It;

No need for semi-colon at the end.

---

Tests/Unit/Stubs/Microsoft.ActiveDirectory.Management.cs, line 28 at r6 (raw file):

        UnknownForest
    }
}

Please end this file with a new line.

---

Tests/Unit/Stubs/Microsoft.DirectoryServices.Deployment.Types.cs, line 28 at r6 (raw file):

        WinThreshold = 7
    }
}

Please end this file with a new line.

---

Comments from Reviewable

[nyanhp]

Review status: 5 of 19 files reviewed, 17 unresolved discussions (waiting on @johlju and @nyanhp)

---

README.md, line 90 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we end the last sentence with full stop (.)?

Done.

---

README.md, line 107 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

(optional)

Can we move (Optional) to the beginning of the first sentence?

Did for entire readme

---

README.md, line 109 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

(optional)

Can we move (Optional) to the beginning of the first sentence?

Did for entire readme

---

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 535 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should Be Win2012

Should this be (to be more clear what's tested)?

| Should Be [Microsoft.DirectoryServices.Deployment.Types.ForestMode]::Win2012

Throughout.

Makes sense, updated it accordingly

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 39 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$mode

Can we split this up into two

$forestMode = 'WinThreshold'
$domainMode = $forestMode

Change $mode appropriately in the code below.

Easier when reviewing to know what a variable contains, I was thrown that forest mode and domain mode would equal the same value in the tests (so had to search where this value was assigned). Splitting these up also helps testing (later) when domain mode and forest mode is not equal.

Done.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 54 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Not sure these are needed? They should already be known to Pester because the resource is importing the helper functions? Does it fail if this and the one below are removed?
We should (normally) only need to make stubs for cmdlets/functions that are not present.

Yes, after following your suggestions with regards to loading the .NET enums, the tests got a bit leaner

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 56 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Mock ConvertTo-DeploymentDomainMode { return $mode }

We should use named parameters, something like this. *You don't have to do the other (not part of your change) that not using the correct style, but lets start with those you adding. But wouldn't mind if all are fixed. 🙂 *

Mock -CommandName ConvertTo-DeploymentDomainMode -MockWith {
    return $mode 
}

Throughout.

I have taken the liberty and corrected EVERYTHING, unless some mocks went through the cracks. Thank god for regular expressions.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 61 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

{ }

Not necessary when no logic is mocked. This should be enough

Mock -CommandName Get-ADForest

Throughout.

Done (for all tests)

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 127 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We are just testing if the helper function is called (one or more times), could we add ParameterFilter to this so it is called with the expected values? Maybe that is not relevant?
Also, maybe we should test that it only called X number of times? -Exactly -Times 1.

Throughout.

Not that relevant, and we don't mock them any more

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 138 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should also have a test that actually returns $null for those (when absent?).

Neither domain nor forest mode can ever be $null unless no domain/forest exist. But in this case, the resource throws an error any way, so I assume that it would not be necessary to test this

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 141 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$result.DomainMode | Should Not Be $null

Can we instead test for the correct value is returned? Not sure what the difference is from the previous test 'Returns the correct domain mode'?

You are right. Removed the test case

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 142 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$result.ForestMode | Should Not Be $null

Can we instead test for the correct value is returned? Not sure what the difference is from the previous test 'Returns the correct forest mode'?

You are right. Removed the test case

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 355 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Scope It;

No need for semi-colon at the end.

Removed from all tests

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 363 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Scope It;

No need for semi-colon at the end.

Removed from all tests

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 472 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Scope It;

No need for semi-colon at the end.

Removed from all tests

---

Tests/Unit/Stubs/Microsoft.ActiveDirectory.Management.cs, line 28 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please end this file with a new line.

Done.

---

Tests/Unit/Stubs/Microsoft.DirectoryServices.Deployment.Types.cs, line 28 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please end this file with a new line.

Done.

---

Comments from Reviewable

[nyanhp]

Done everything. Local tests run fine all the time, as soon as I push and AppVeyor tests, the first couple of tests for xADDomain fail for no apparent reason. I might need help there, my limited Pester knowledge has run out.

[johlju]

Strange 🤔 I will look at it tomorrow and see if I can see what’s going on.

[johlju]

Okay, so the unit test works locally, so I guess it's because the feature RSAT-AD-PowerShell is installed in the build worker, so there are already a class loaded.

https://github.com/PowerShell/xActiveDirectory/blob/fcd53fd2351a221bfdf0cc092a59a21aae95369b/appveyor.yml#L8

Maybe adding this will resolve it. Locally it will be loaded, but in the AppVeyor it will only be loaded if the type does not already exist.

I have not tested this in AppVeyor, but a hint where the problem is at least?

function Invoke-TestSetup {
    # If one type does not exist, it's assumed the other ones does not exist either.
    if (-not 'Microsoft.DirectoryServices.Deployment.Types.ForestMode' -as [Type])
    {
        Add-Type -Path (Join-Path -Path (Join-Path -Path (Join-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'Tests') -ChildPath 'Unit') -ChildPath 'Stubs') -ChildPath 'Microsoft.DirectoryServices.Deployment.Types.cs')
    }

    # If one type does not exist, it's assumed the other ones does not exist either.
    if (-not 'Microsoft.ActiveDirectory.Management.ADForestMode' -as [Type])
    {
        Add-Type -Path (Join-Path -Path (Join-Path -Path (Join-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'Tests') -ChildPath 'Unit') -ChildPath 'Stubs') -ChildPath 'Microsoft.ActiveDirectory.Management.cs')
    }
}

[johlju]

Awsome work on this! Thank you so much for fixing style in so many files! 😃

---

Reviewed 13 of 14 files at r7.
Review status: 18 of 19 files reviewed, 3 unresolved discussions (waiting on @johlju and @nyanhp)

---

README.md, line 109 at r6 (raw file):

Previously, nyanhp (Jan-Hendrik Peters [MSFT]) wrote…

Did for entire readme

Thank you so much for doing the entire README.md! Although, I meant that it should go before the first sentence in the description. Would you mind resolving that?

Actually, if you would like, in the end we would want the the parameter names to look like this (to align with what other resource modules does)

* **`[String]` Name** _(Key)_:
* **`[String]` DatabaseName** _(Required)_:
* **`[UInt32]` BackupPriority** _(Write)_:

See example: https://github.com/PowerShell/SqlServerDsc#parameters

Then '(optional)' would be obsolete , since the schema type qualifier Key, Required, Write or Read is written ('(optional)' its equivalent to type qualifierWrite).
Readparameters can look like this; https://github.com/PowerShell/SqlServerDsc#read-only-properties-from-get-targetresource

It good enough if you move (optional) to the beginning of the description, but if you want to do they new format as above, then I would be very grateful (on less item in the backlog)! 😃

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 401 at r7 (raw file):

Mock djoin.exe { $LASTEXITCODE = 0; 'OK' }

Should be

Mock -CommandName djoin.exe -MockWith { $LASTEXITCODE = 0; 'OK' }

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 402 at r7 (raw file):

{ }

Non-blocking. We can remove these. Throughout if you would like to help clean up these even if they are not part of your change.

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 56 at r6 (raw file):

Previously, nyanhp (Jan-Hendrik Peters [MSFT]) wrote…

I have taken the liberty and corrected EVERYTHING, unless some mocks went through the cracks. Thank god for regular expressions.

Awesome thank you! 😃 👍

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 138 at r6 (raw file):

Previously, nyanhp (Jan-Hendrik Peters [MSFT]) wrote…

Neither domain nor forest mode can ever be $null unless no domain/forest exist. But in this case, the resource throws an error any way, so I assume that it would not be necessary to test this

Ah, okay, so that is an issue 🤔 We don't need to resolve this in this PR, but it should not throw in Get-TargetResource when no domain exist, it should always return the properties in the schema.mof with appropriate values (like $null values). If it throws then Get-DscConfiguration fails, in those rare circumstances where there is no domain when the user runs Get-DscConfiguration if the domain failed to be created.

Would you mind submitting an issue to so this issue can be tracked?

---

Comments from Reviewable

[nyanhp]

Thanks for the suggestion @johlju , I'll incorporate and test this. This might be it - if the Ad-Domain-Services feature is not installed, the deployment module and it's accompanying types are not present as well.

[nyanhp]

Review status: 8 of 19 files reviewed, 3 unresolved discussions (waiting on @johlju and @nyanhp)

---

README.md, line 109 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Thank you so much for doing the entire README.md! Although, I meant that it should go before the first sentence in the description. Would you mind resolving that?

Actually, if you would like, in the end we would want the the parameter names to look like this (to align with what other resource modules does)

* **`[String]` Name** _(Key)_:
* **`[String]` DatabaseName** _(Required)_:
* **`[UInt32]` BackupPriority** _(Write)_:

See example: https://github.com/PowerShell/SqlServerDsc#parameters

Then '(optional)' would be obsolete , since the schema type qualifier Key, Required, Write or Read is written ('(optional)' its equivalent to type qualifierWrite).
Readparameters can look like this; https://github.com/PowerShell/SqlServerDsc#read-only-properties-from-get-targetresource

It good enough if you move (optional) to the beginning of the description, but if you want to do they new format as above, then I would be very grateful (on less item in the backlog)! 😃

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 401 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Mock djoin.exe { $LASTEXITCODE = 0; 'OK' }

Should be

Mock -CommandName djoin.exe -MockWith { $LASTEXITCODE = 0; 'OK' }

Done.

---

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 402 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

{ }

Non-blocking. We can remove these. Throughout if you would like to help clean up these even if they are not part of your change.

Done throughout

---

Tests/Unit/MSFT_xADDomain.Tests.ps1, line 138 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah, okay, so that is an issue 🤔 We don't need to resolve this in this PR, but it should not throw in Get-TargetResource when no domain exist, it should always return the properties in the schema.mof with appropriate values (like $null values). If it throws then Get-DscConfiguration fails, in those rare circumstances where there is no domain when the user runs Get-DscConfiguration if the domain failed to be created.

Would you mind submitting an issue to so this issue can be tracked?

Changed the conversion functions to accept null values

---

Comments from Reviewable

[johlju]

Thank you so much for fixing the README.md thorughout! It looks great now! 😃 Just two small comments left, then I think this is good to go!

---

Reviewed 11 of 11 files at r8.
Review status: all files reviewed, 2 unresolved discussions (waiting on @nyanhp)

---

README.md, line 98 at r8 (raw file):

  * _Note: These are NOT used during domain creation._

(AD sets the local admin credentials as new domain administrator credentials during setup.)

Can we maybe move this and add it to the "Note:" row? It will render a bit nicer if we do that. Either that, or move it as a note in the resource description?

See how it renders here, it breaks the list; https://github.com/nyanhp/xActiveDirectory/blob/b991a576bee43edaaf0004d388ac5ce503b27bab/README.md#xaddomain

---

README.md, line 316 at r8 (raw file):

## Versions

### Unreleased

Please rebase to get the latest changed in the change log after the latest release.

There was a new release of the xActiveDirectory, so the items that was under the Unreleased section was moved to a new sction with a new release number, see here; https://github.com/PowerShell/xActiveDirectory#unreleased

Git doesn't recognize that, so when you rebase you might need to move the items back to the Unreleased section.

---

Comments from Reviewable

[nyanhp]

Review status: 18 of 19 files reviewed, 2 unresolved discussions (waiting on @johlju)

---

README.md, line 98 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we maybe move this and add it to the "Note:" row? It will render a bit nicer if we do that. Either that, or move it as a note in the resource description?

See how it renders here, it breaks the list; https://github.com/nyanhp/xActiveDirectory/blob/b991a576bee43edaaf0004d388ac5ce503b27bab/README.md#xaddomain

Done.

---

README.md, line 316 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please rebase to get the latest changed in the change log after the latest release.

There was a new release of the xActiveDirectory, so the items that was under the Unreleased section was moved to a new sction with a new release number, see here; https://github.com/PowerShell/xActiveDirectory#unreleased

Git doesn't recognize that, so when you rebase you might need to move the items back to the Unreleased section.

Done.

---

Comments from Reviewable

[johlju]

---

Reviewed 1 of 1 files at r9.
Review status: complete! all files reviewed, all discussions resolved

---

Comments from Reviewable

[johlju]

@nyanhp Awesome work on this! Thank you so much! 😃