xADDomainController: Added new parameter IsGlobalCatalog #255

johlju · 2019-04-20T08:05:02Z

Pull Request (PR) description

Changes to xADDomainController
- Added new parameter to disable or enable the Global Catalog (GC) (issue xADDomain and xADDomainController: Ability to enable/disable Global Catalog #75).
- Fixed a bug with the parameter InstallationMediaPath that it would
  not be added if it was specified in a configuration. Now the parameter
  InstallationMediaPath is correctly passed to Install-ADDSDomainController.
- Refactored the resource with major code cleanup and localization.
- Updated unit tests to latest unit test template, and refactored the
  tests for the function 'Set-TargetResource'.
- Improved test code coverage.

This Pull Request (PR) fixes the following issues

Task list

Added an entry under the Unreleased section of the change log in the README.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-04-20T08:11:55Z

Codecov Report

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

@@         Coverage Diff          @@
##            dev   #255    +/-   ##
====================================
+ Coverage    87%    88%   +<1%     
====================================
  Files        20     20            
  Lines      1852   1904    +52     
  Branches     11     10     -1     
====================================
+ Hits       1624   1684    +60     
+ Misses      217    210     -7     
+ Partials     11     10     -1

johlju · 2019-04-20T08:13:26Z

@nyanhp, @devopsjesus, @rchristman89 would anyone of you (or all) help me an review this PR? 😃 Let me know if you need any help to get started with reviewing. I'm here or on (join) Slack.

kungfoome

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 87 at r1 (raw file):

            try
            {
                $dc = Get-ADDomainController -Identity $env:COMPUTERNAME -Credential $DomainAdministratorCredential

variables should be spelled out throughout. Non-Stopping (but maybe open issue for clean-up on this resource)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 88 at r1 (raw file):

            {
                $dc = Get-ADDomainController -Identity $env:COMPUTERNAME -Credential $DomainAdministratorCredential
                Write-Verbose -Message "Found domain controller '$($dc.Name)' in domain '$($dc.Domain)'."

Should be using localized strings. Non-Stopping (but maybe open issue for clean-up on this resource)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 228 at r1 (raw file):

        Write-Verbose -Message "Verified that domain '$($DomainName)' is present, continuing ..."
        $params = @{

params should be spelled out throughout. Non-Stopping (but maybe open issue for clean-up on this resource)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 236 at r1 (raw file):

        }

        if ($DatabasePath -ne $null)

Should use 'ContainsKey' throughout. Non-Stopping (but maybe open issue for clean-up on this resource)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 289 at r1 (raw file):

            }

            $dc = Get-ADDomainController -Identity $env:COMPUTERNAME -Credential $DomainAdministratorCredential -ErrorAction 'Stop'

dc should be spelled out throughout. Non-Stopping (but maybe open issue for clean-up on this resource)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 298 at r1 (raw file):

            else
            {
                throw 'Could not get the distinguished name of the NTDSSettingsObject directory object that represents this domain controller.'

Should be using localized strings. Non-Stopping (but maybe open issue for clean-up on this resource)

Examples/Resources/xADDomainController/1-AddDomainControllerToDomainMinimal.ps1, line 46 at r1 (raw file):

        }

        xADDomainController 'DomainController1'

Can we call this something like 'DomainControllerMinimal'

Examples/Resources/xADDomainController/2-AddDomainControllerToDomainAllProperties.ps1, line 46 at r1 (raw file):

        }

        xADDomainController 'DomainController1'

Can we call this 'DomainControllerAllProperties'?

Examples/Resources/xADDomainController/3-AddDomainControllerToDomainUsingIFM.ps1, line 46 at r1 (raw file):

        }

        xADDomainController 'DomainController1'

Can we call this something like 'DomainControllerWithIFM'

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 304 at r1 (raw file):

                Mock -CommandName Install-ADDSDomainController -ParameterFilter { $InstallationMediaPath -eq $correctInstallationMediaPath }

                Set-TargetResource @testDefaultParams -DomainName $correctDomainName -InstallationMediaPath $correctInstallationMediaPath -Verbose

Should Verbose be enabled in test cases?

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 321 at r1 (raw file):

                Mock -CommandName Move-ADDirectoryServer

                Set-TargetResource @testDefaultParams -DomainName $correctDomainName -SiteName $correctSiteName -Verbose

same as above

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 337 at r1 (raw file):

                Mock -CommandName Move-ADDirectoryServer

                Set-TargetResource @testDefaultParams -DomainName $correctDomainName -SiteName $correctSiteName -Verbose

same as above

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 352 at r1 (raw file):

                Mock -CommandName Move-ADDirectoryServer

                Set-TargetResource @testDefaultParams -DomainName $correctDomainName -Verbose

same as above

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 379 at r1 (raw file):

                    }

                    Set-TargetResource @testDefaultParams -DomainName $correctDomainName -IsGlobalCatalog $true -Verbose

same as above

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 395 at r1 (raw file):

                    }

                    Set-TargetResource @testDefaultParams -DomainName $correctDomainName -IsGlobalCatalog $false -Verbose

same as above

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 411 at r1 (raw file):

                    }

                    Set-TargetResource @testDefaultParams -DomainName $correctDomainName -IsGlobalCatalog $true -Verbose

same as above

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 425 at r1 (raw file):

                    }

                    Set-TargetResource @testDefaultParams -DomainName $correctDomainName -Verbose

same as above

johlju · 2019-04-27T09:52:51Z

@kungfu71186 Many thanks for the review 🙇 I will get on these in a day or so!

johlju

I made major refactor of this resource when I added localization. I have not tested this resource in a lab after the refactor. Looking to see how the unit tests work out after this refactor. Theoretical the refactor should not have changed any functionality.

Reviewable status: 1 of 8 files reviewed, 17 unresolved discussions (waiting on @kungfu71186)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 87 at r1 (raw file):

Previously, kungfu71186 wrote…

variables should be spelled out throughout. Non-Stopping (but maybe open issue for clean-up on this resource)

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 88 at r1 (raw file):

Previously, kungfu71186 wrote…

Should be using localized strings. Non-Stopping (but maybe open issue for clean-up on this resource)

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 228 at r1 (raw file):

Previously, kungfu71186 wrote…

params should be spelled out throughout. Non-Stopping (but maybe open issue for clean-up on this resource)

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 236 at r1 (raw file):

Previously, kungfu71186 wrote…

Should use 'ContainsKey' throughout. Non-Stopping (but maybe open issue for clean-up on this resource)

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 289 at r1 (raw file):

Previously, kungfu71186 wrote…

dc should be spelled out throughout. Non-Stopping (but maybe open issue for clean-up on this resource)

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 298 at r1 (raw file):

Previously, kungfu71186 wrote…

Should be using localized strings. Non-Stopping (but maybe open issue for clean-up on this resource)

Done.

Examples/Resources/xADDomainController/1-AddDomainControllerToDomainMinimal.ps1, line 46 at r1 (raw file):

Previously, kungfu71186 wrote…

Can we call this something like 'DomainControllerMinimal'

Done.

Examples/Resources/xADDomainController/2-AddDomainControllerToDomainAllProperties.ps1, line 46 at r1 (raw file):

Previously, kungfu71186 wrote…

Can we call this 'DomainControllerAllProperties'?

Done.

Examples/Resources/xADDomainController/3-AddDomainControllerToDomainUsingIFM.ps1, line 46 at r1 (raw file):

Previously, kungfu71186 wrote…

Can we call this something like 'DomainControllerWithIFM'

Done.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 304 at r1 (raw file):

Previously, kungfu71186 wrote…

Should Verbose be enabled in test cases?

Done. Added it to the splatting hash table so I only needed to set it once.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 321 at r1 (raw file):

Previously, kungfu71186 wrote…

same as above

Done.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 337 at r1 (raw file):

Previously, kungfu71186 wrote…

same as above

Done.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 352 at r1 (raw file):

Previously, kungfu71186 wrote…

same as above

Done.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 379 at r1 (raw file):

Previously, kungfu71186 wrote…

same as above

Done.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 395 at r1 (raw file):

Previously, kungfu71186 wrote…

same as above

Done.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 411 at r1 (raw file):

Previously, kungfu71186 wrote…

same as above

Done.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 425 at r1 (raw file):

Previously, kungfu71186 wrote…

same as above

Done.

johlju · 2019-04-28T14:04:10Z

@kungfu71186 You mind looking at this again? I refactored it a bit, so a little more to review though. 🙂

kungfoome

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 17 at r2 (raw file):

<#
    .SYNOPSIS
        Returns the current state of the certificate that may need to be requested.

Should be 'Domain Controller', not certificate

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 77 at r2 (raw file):

    Assert-Module -ModuleName 'ActiveDirectory'

    $returnValue = @{

Is this standardize (returnValue variable name) or should it be? Just curious tbh

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 161 at r2 (raw file):

<#
    .SYNOPSIS
        Returns the current state of the certificate that may need to be requested.

Should be 'Domain Controller', not certificate

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 311 at r2 (raw file):

            if ($IsGlobalCatalog)
            {
                $value = 1

Can we make this a little more descriptive. Maybe just 'globalCatalogValue'

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 356 at r2 (raw file):

<#
    .SYNOPSIS
        Returns the current state of the certificate that may need to be requested.

same, certificate

kungfoome · 2019-04-28T16:24:08Z

@johlju looks pretty good on my end. Good job!

kungfoome

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 77 at r2 (raw file):

    Assert-Module -ModuleName 'ActiveDirectory'

    $returnValue = @{

Is this standardize (returnValue variable name), like some other resources use targetResource. Just curious tbh

johlju

Reviewable status: 7 of 8 files reviewed, 5 unresolved discussions (waiting on @kungfu71186)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 17 at r2 (raw file):

Previously, kungfu71186 wrote…

Should be 'Domain Controller', not certificate

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 77 at r2 (raw file):

Previously, kungfu71186 wrote…

Is this standardize (returnValue variable name) or should it be? Just curious tbh

Done. There is no actual standard for the variable name. I like have the variable names descriptive, so it it easier to read the code to understand what the variable contain when reading just a part of the code.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 77 at r2 (raw file):

Previously, kungfu71186 wrote…

Is this standardize (returnValue variable name), like some other resources use targetResource. Just curious tbh

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 161 at r2 (raw file):

Previously, kungfu71186 wrote…

Should be 'Domain Controller', not certificate

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 311 at r2 (raw file):

Previously, kungfu71186 wrote…

Can we make this a little more descriptive. Maybe just 'globalCatalogValue'

Done.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 356 at r2 (raw file):

Previously, kungfu71186 wrote…

same, certificate

Done.

kungfoome

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

kungfoome · 2019-04-29T12:31:46Z

@johlju not sure if anyone else needs to review, but looks good to me.

johlju · 2019-04-29T12:41:12Z

@kungfu71186 I just push another commit, can you sign off on that too? I found out the hard way when manually doing an integration tests. The
Install-ADDSDomainController just halted. My config used the same account for both the primary and secondary. Secondary needed the domain account, while primary used the local account. :)
I wrongly assumed it would understand to use the domain name I also provided in the configuration. 😄

No, no other need to review. Your review and my manual test should be enough. 😃 Thank you!

kungfoome

Awesome, i will try to do a manual test tonight

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

README.md, line 124 at r4 (raw file):

>seemingly halt (without reporting an error) when trying to replicate
>information from another domain controller.
>Make sure to use a correct domain account with the correct permssion as

permission spelling

johlju

Awesome! I will wait to merge this until you say okay on your testing too then! 🙂

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @kungfu71186)

README.md, line 124 at r4 (raw file):

Previously, kungfu71186 wrote…

permission spelling

Done. Doh. VSCode even told me it was spelled wrong. I just missed it. :/

johlju · 2019-04-29T13:19:38Z

@kungfu71186 I usually get the spelling right, but the grammar is not my strong suit (English is second language), so if you see me do grammatical errors, then please correct me. :) @PlagueHO are usually good at spotting those for me too. 😄

kungfoome · 2019-04-29T15:04:54Z

@johlju no worries, English is my first language, but sometimes it seems like it's my third lol.

kungfoome

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

kungfoome · 2019-04-29T15:44:57Z

@johlju Ran through a clean DC install with 2019. One issue I ran into was the windows feature AD-Domain-Service is required, without this you can't run Install-ADDSDomainController This isn't in the examples either.

Another strange issue is after I rebooted the machine, it says that it is a DC, but asked to finish the promotion. not sure why. Maybe a 2019 thing? Could also be my environment.

Found one other issue. If I do something like:

DomainName: mydomain.contoso.com

Config looks like:

        xADDomainController 'DomainControllerMinimal'
        {
            DomainName                    = 'mydomain'
            DomainAdministratorCredential = $DomainAdministratorCredential
            SafemodeAdministratorPassword = $DomainAdministratorCredential
        }

It fails the check because of:

https://github.com/johlju/xActiveDirectory/blob/80f5c2b1fc08ac3627987b8167a97550c16a99ac/DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1#L134

Its comparing using the fqdn. I think this check is sufficient enough:
https://github.com/johlju/xActiveDirectory/blob/80f5c2b1fc08ac3627987b8167a97550c16a99ac/DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1#L110

I think if you move: https://github.com/johlju/xActiveDirectory/blob/80f5c2b1fc08ac3627987b8167a97550c16a99ac/DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1#L130-L148 this into the try block, you will be ok with your check or do something like i did:

https://github.com/kungfu71186/xActiveDirectory/blob/a7dfce34d372520a82cda1601dd5054810fbfcd3/DSCResources/MSFT_xADKDSKey/MSFT_xADKDSKey.psm1#L496

Maybe we can make that a common function. Assert-IsDomainController or whatever.

You could also do it like MSA:

try 
{
Get-ADDomainController -Identity $env:COMPUTERNAME
} 
catch [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException]
{
this isn't a domain controller
}
catch{}

johlju · 2019-04-30T16:05:12Z

One issue I ran into was the windows feature AD-Domain-Service is required, without this you can't run Install-ADDSDomainController This isn't in the examples either.

It's in the example here; https://github.com/PowerShell/xActiveDirectory#examples
Those examples should really move to the Examples folder (submitted an issue for that).

Another strange issue is after I rebooted the machine, it says that it is a DC, but asked to finish the promotion. not sure why. Maybe a 2019 thing? Could also be my environment.

I did not get this on Windows Server 2016 🤔

johlju · 2019-04-30T16:12:35Z

catch [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException]

I actually did this and changed it in commit d84393b

The reason I changed it because the test failed... And that is because I never actually throw the correct error it was expecting, so of course it failed. 😞 I must have been tired. 😕

Thank you for pointing out that again for me, I will make the correction.

johlju · 2019-05-01T06:43:56Z

I'm gonna get the code coverage (thats the failing test), also seeing a strange error on Win 2016. Not sure if it is related to this change, or my configuration, but want to make sure.

johlju

Reviewable status: all files reviewed, 1 unresolved discussion

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 948 at r8 (raw file):

catch [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException]

@kungfu71186 I actually get Microsoft.ActiveDirectory.Management.ADServerDownException when calling 'Get' function when the node is not already in a domain. If the node is in a domain I get [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException]. So I think we need to catch both.

kungfoome · 2019-05-01T07:52:27Z

Interesting. I had only tried while the node was on the domain this time.

johlju · 2019-05-01T07:54:47Z

I wonder if we should do this instead Get-ADDomainController -Filter '*' -Server 'companylab.se'. That gives us all the domain controllers. If the $env:COMPUTERNAME is not part of the list returned, and the helper function Assert-IsDomainController returns false , then we could assume thise is not a domain controller?

johlju · 2019-05-01T07:55:13Z

Then we don't have to handle any errors at all. Much cleaner. Well, only error we need to handle if the Get-ADDomainController fails entirely. :)

johlju

Reviewable status: 6 of 11 files reviewed, all discussions resolved (waiting on @kungfu71186)

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 948 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

catch [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException]
@kungfu71186 I actually get Microsoft.ActiveDirectory.Management.ADServerDownException when calling 'Get' function when the node is not already in a domain. If the node is in a domain I get [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException]. So I think we need to catch both.

Done

johlju · 2019-05-01T10:19:11Z

@kungfu71186 Can you be so kind to review/approve these changes again?

I feel comfortable that this change should be stable and work now (it feels more stable). I refactored the unit tests for Set-TargetResource too, and fixed a 100% code coverage for the xADDomainController resource.
The unit test passed locally on a Domain Controller, so they should pass in AppVeyor too, otherwise I will fix them promptly.

johlju · 2019-05-01T10:25:02Z

I tested this change manually by running Get and Test with Invoke-DscResource, and then I run a full configuration on a node that was not part of a domain, and it successfully rebooted as a domain controller, and also not having a global catalog. 🙂

kungfoome

@

Reviewed 3 of 5 files at r9, 1 of 1 files at r10.
Reviewable status: 10 of 11 files reviewed, 7 unresolved discussions (waiting on @johlju and @kungfu71186)

CHANGELOG.md, line 28 at r10 (raw file):

template, and

no comma here

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

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

        WasExpectingDomainController     = The operating system product type code is 2 so was expecting this node to be a domain controller, but no domain controller object was returned. (ADC0001)

It looks like this is used for when you get the domain controller object, but the product type code returns 2, indicating that it is a domain controller and you should have gotten the DC object.

Maybe something like:

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

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 917 at r10 (raw file):

If the domain controller is not found, and empty object ($null) is returned.

If the domain controller is not found, an empty object ($null) is returned.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 918 at r10 (raw file):

    .OUTPUTS
        If the domain controller is not found, and empty object ($null) is returned.
    .NOTES

Need newline before

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 919 at r10 (raw file):

        If the domain controller is not found, and empty object ($null) is returned.
    .NOTES
        Throws and Microsoft.ActiveDirectory.Management.ADServerDownException

Throws an exception of Microsoft.ActiveDirectory.Management.ADServerDownException if the domain cannot be contacted.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 976 at r10 (raw file):

    .SYNOPSIS
        Returns the domain controller object if the node is a domain controller,
        otherwise it return $null.

Returns $true if the node is a domain controller, otherwise it returns $false

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 455 at r10 (raw file):

                    }

                    Context 'When the domain controller should change state og Global Catalog, but fail to return a domain controller object' {

og = of? typo

kungfoome · 2019-05-01T16:38:31Z

@johlju looks like code is good, just some grammatical issues and then ready for merge. Yay!!!

johlju

Added two more localized strings. Realized it never say the global catalog was not in desired state.

Reviewable status: 6 of 11 files reviewed, 7 unresolved discussions (waiting on @johlju and @kungfu71186)

CHANGELOG.md, line 28 at r10 (raw file):

Previously, kungfu71186 wrote…

template, and
no comma here

Done.

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

Previously, kungfu71186 wrote…

It looks like this is used for when you get the domain controller object, but the product type code returns 2, indicating that it is a domain controller and you should have gotten the DC object.

Maybe something like:

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

Done.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 917 at r10 (raw file):

Previously, kungfu71186 wrote…

If the domain controller is not found, and empty object ($null) is returned.
If the domain controller is not found, an empty object ($null) is returned.

Done.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 918 at r10 (raw file):

Previously, kungfu71186 wrote…

Need newline before

Done.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 919 at r10 (raw file):

Previously, kungfu71186 wrote…

Throws an exception of Microsoft.ActiveDirectory.Management.ADServerDownException if the domain cannot be contacted.

Done.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 976 at r10 (raw file):

Previously, kungfu71186 wrote…

Returns $true if the node is a domain controller, otherwise it returns $false

Done. Copy paste mistake :/

johlju · 2019-05-02T17:21:37Z

@kungfu71186 When you have time, could you please sign off on the last changes. 😃

kungfoome

Reviewed 5 of 5 files at r11.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

kungfoome

@johlju looks good! Hopefully I didn't miss anything.

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

johlju

Reviewed 1 of 7 files at r1, 4 of 10 files at r6, 1 of 5 files at r9, 5 of 5 files at r11.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

johlju

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

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 455 at r10 (raw file):

Previously, kungfu71186 wrote…

og = of? typo

Done.

johlju · 2019-05-02T18:26:16Z

I will run one more manual test in Win 2019, then I merge

johlju · 2019-05-02T18:54:22Z

@kungfu71186 Two additional domain controllers installed without any issue. One with GC and one without GC.

johlju added the needs review The pull request needs a code review. label Apr 20, 2019

johlju mentioned this pull request Apr 20, 2019

xAdDomainController - Adds IsGlobalCatalog parameter and fix for Ensure schema violation #135

Closed

kungfoome reviewed Apr 26, 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 Apr 27, 2019

johlju force-pushed the changes-from-PR#135 branch from b677659 to 266d84b Compare April 28, 2019 11:03

johlju commented Apr 28, 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 Apr 28, 2019

kungfoome reviewed Apr 28, 2019

View reviewed changes

johlju commented Apr 29, 2019

View reviewed changes

kungfoome reviewed Apr 29, 2019

View reviewed changes

johlju commented Apr 29, 2019

View reviewed changes

kungfoome reviewed Apr 29, 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 Apr 30, 2019

johlju commented May 1, 2019

View reviewed changes

johlju added 2 commits May 1, 2019 12:14

Fix Get-DomainControllerObject

1f8f4ff

Refactored tests and improved code coverage

4f9fcb5

johlju commented May 1, 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 May 1, 2019

Fix minor type i named parameter

cc58d40

kungfoome reviewed May 1, 2019

View reviewed changes

johlju added 2 commits May 2, 2019 18:48

Added localized message for Global Catalog mismatch

451d6cd

Fix review comments at r10

d9d2bc2

johlju commented May 2, 2019

View reviewed changes

kungfoome reviewed May 2, 2019

View reviewed changes

johlju commented May 2, 2019

View reviewed changes

johlju merged commit d732f41 into dsccommunity:dev May 2, 2019

kwirkykat removed the needs review The pull request needs a code review. label May 2, 2019

johlju deleted the changes-from-PR#135 branch May 2, 2019 18:55

SteveL-MSFT added this to Done in powershell/dscresources May 14, 2019

SteveL-MSFT removed this from Done in powershell/dscresources Nov 27, 2019

xADDomainController: Added new parameter IsGlobalCatalog #255

xADDomainController: Added new parameter IsGlobalCatalog #255

Conversation

johlju commented Apr 20, 2019 • edited

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov-io commented Apr 20, 2019 • edited

Codecov Report

johlju commented Apr 20, 2019

kungfoome left a comment

Choose a reason for hiding this comment

johlju commented Apr 27, 2019

johlju left a comment

Choose a reason for hiding this comment

johlju commented Apr 28, 2019

kungfoome left a comment

Choose a reason for hiding this comment

kungfoome commented Apr 28, 2019

kungfoome left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

kungfoome left a comment

Choose a reason for hiding this comment

kungfoome commented Apr 29, 2019

johlju commented Apr 29, 2019 • edited

kungfoome left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Apr 29, 2019

kungfoome commented Apr 29, 2019

kungfoome left a comment

Choose a reason for hiding this comment

kungfoome commented Apr 29, 2019 • edited

johlju commented Apr 30, 2019

johlju commented Apr 30, 2019

johlju commented May 1, 2019

johlju left a comment

Choose a reason for hiding this comment

kungfoome commented May 1, 2019

johlju commented May 1, 2019

johlju commented May 1, 2019 • edited

johlju left a comment

Choose a reason for hiding this comment

johlju commented May 1, 2019

johlju commented May 1, 2019 • edited

kungfoome left a comment

Choose a reason for hiding this comment

kungfoome commented May 1, 2019

johlju left a comment

Choose a reason for hiding this comment

johlju commented May 2, 2019 • edited

kungfoome left a comment

Choose a reason for hiding this comment

kungfoome 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

johlju commented May 2, 2019 • edited

johlju commented May 2, 2019

johlju commented Apr 20, 2019 •

edited

codecov-io commented Apr 20, 2019 •

edited

johlju commented Apr 29, 2019 •

edited

kungfoome commented Apr 29, 2019 •

edited

johlju commented May 1, 2019 •

edited

johlju commented May 1, 2019 •

edited

johlju commented May 2, 2019 •

edited

johlju commented May 2, 2019 •

edited