xAdDomainController - Adds IsGlobalCatalog parameter and fix for Ensure schema violation

[Merto410]

Adds Option IsGlobalCatalog to xAdDomainController per issue/feature request #75 and fixes get-dscconfiguration issue with xAdDomainController #111 .

---

This change is 

[msftclas]

Hi @Merto410, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[Merto410]

I believe the appveyor check is wrong. I have validated that on a server, that it shows true when sitename matches and false when it doesn't and set properly moves the server without issue if sitename doesn't match.

[johlju]

I just glanced at this code to help you with the tests failing. Saw some other things I commented on as well

Would you be so kind to add tests for the new parameters and logic as well.

---

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

---

README.md, line 248 at r2 (raw file):
Please review code PR #111. The schema is not correct if you wanted the same functionality as PR #111.

Also, please mention original author in your PR description, since you continue working on the abandoned PR #111.

It's important that when you create a new pull request from someone elses work, that you mention the the original pull request, and also aknowledge the original author and mention the work it is based on. For example mention the original author in the descriptive field when you create the new pull request.

https://github.com/johlju/DscResources/blob/master/GettingStartedWithGitHub.md#how-to-continue-working-on-a-pull-request-when-an-author-contributor-is-unable-to-complete-it

---

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

        [Bool]$IsGlobalCatalog = $true,
    
        [Bool]$Ensure,

What does this do? Not mentioned in the Readme.md, please add it.

---

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

        [Bool]$Ensure,

        [String]$NTDSSettingsObjectDN

Please add this to the README.md.

---

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

        $existingResource = Get-TargetResource @PSBoundParameters
        $isCompliant = $existingResource.Ensure

What is this row meant to achieve ?

---

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

            $isGCCorrect = $false
        }
        if ($isGCCorrect -eq $false -or $isinsite -eq $false)

This is always evaluated to $false. The variables are not assigned before this evaluation? Probably why the test fail.

---

Comments from Reviewable

[Merto410]

@johlju Thanks for looking at it.

to answer:

"Would you be so kind to add tests for the new parameters and logic as well."
Where is the existing tests, and how would I go about modifying/updating the logic
** Never mind. Found the test app. Will work on updating it.

"Also, please mention original author in your PR description"
I can certainly do that but I only linked it when I saw someone had the same issue so that no one would duplicate effort. I resolved it completely independently without input or code from another author. (which is why the functionality isn't quite the same).
PR #111

For the next two, that is part of the fix to the get-dscconfiguration being broken. The vars called in the schema have to be referenced in all three functions. The purpose is referenced in the readme.md. would further explanation of each line be helpful in the readme or would it be better to have line comments?

" $existingResource = Get-TargetResource @PSBoundParameters
$isCompliant = $existingResource.Ensure"

These were in the code prior to my changes but they pull the get to perform the test and set the default state equal to the install state of AD services.

"This is always evaluated to $false. The variables are not assigned before this evaluation? Probably why the test fail."

They evaluate based on the $existingresource values (the if statement right above your quotes code).

Also note that there is a default value assigned to the variable in all locations since we default to $true unless specified $false.

I suspect the test evaluates it as false for some reason because it isn't looking or setting the value properly, but this requires a re-write of the test and I'm not sure where or how to update that. The updated test function is needed to correct the GC state.

It does evaluate correctly in a "production" system

Thanks again

[msftgits]

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

[Merto410]

Updated test to account for Global Catalog options

[raandree]

@Merto410 and @johlju : How is the status of this PR? I would like to use and extend the resource with the GC functionality added.

[Merto410]

@raandree I haven't seen any update on when the review is taking place or what needs to happen to make it go forward. I'm circling the runway :)

[bgelens]

Hi @Merto410. Do you still want to move this PR forward? I'd like to know because I need to figure out if I should review this PR or not. If you want to go forward, I'll review. If not I'll close this PR instead and open a new one myself (or maybe assign it to @raandree).

[johlju]

@Merto410 Sorry that it has taken suck a long time getting this merged. I'm available to get this merged so I'm hoping you have time to work on it. If you don't, then please let me know and I will mark it as abandoned so that someone else can continue the work. Thanks!

[Merto410]

@johlju, @bgelens if it hasn't already been addressed in a different pull then it should reviewed for inclusion yes.

[johlju]

@Merto410 Happy to review this. Could you please rebase this PR against dev so you get the latest changes, and so README.md is correct. Thanks!

[johlju]

Reviewed 1 of 3 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 28 at r5 (raw file):

        [String]$SiteName,
        
        [Bool]$IsGlobalCatalog = $true

We should not add unused non-mandatory parameters to Get-TargetResource.

https://github.com/PowerShell/DscResources/blob/master/BestPractices.md#get-targetresource-should-not-contain-unused-non-mandatory-parameters

---

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 58 at r5 (raw file):

                    $returnValue.SiteName     = $dc.Site
                    $returnValue.IsGlobalCatalog = $dc.IsGlobalCatalog
                    $returnValue.NTDSSettingsObjectDN = $dc.NTDSSettingsObjectDN

We should not return internal objects to Get-DscConfiguration. If this is only necessary for the Set-TargetResource, then I suggest we make sure to get this property another way in the Set-TargetResource function and not add it as a read-only parameter.

---

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 162 at r5 (raw file):

            ## DC is not in the expected Global Catalog state
            Write-Verbose "Setting the Global Catalog state to '$IsGlobalCatalog'"
            if ($IsGlobalCatalog -eq $true){$value = 1}

We should have the braces on there own lines.

---

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 163 at r5 (raw file):

            Write-Verbose "Setting the Global Catalog state to '$IsGlobalCatalog'"
            if ($IsGlobalCatalog -eq $true){$value = 1}
            if ($IsGlobalCatalog -eq $false){$value = 0}

We should have the braces on there own lines.

---

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 164 at r5 (raw file):

Set-adobject

Set-AdObject (upper 'A' and 'O')

---

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.schema.mof, line 12 at r5 (raw file):

    [write] String SiteName;
    [write] Boolean IsGlobalCatalog;
    [read] Boolean Ensure;

Please add the read-only properties to the README.md.

See example how this can be done here; https://github.com/PowerShell/SqlServerDsc#sqlag

---

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.schema.mof, line 13 at r5 (raw file):

    [write] Boolean IsGlobalCatalog;
    [read] Boolean Ensure;
    [read] String NTDSSettingsObjectDN;

See another comment about this read-only property.

---

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 303 at r5 (raw file):

       It 'Calls "Set-ADObject" when "IsGlobalCatalog" does not match' {
         Mock Get-TargetResource {

We should indent with four spaces. Throughout, multiple locations.

---

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 314 at r5 (raw file):

Set-ADObject  -MockWith {}

Double space between cmdlet name and named parameter. Throughout, multiple locations.

---

Comments from Reviewable

[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.

[johlju]

@Merto410 Just pinging to check if you have the time to continue working on this PR? If not I will try to get this through.

[johlju]

This PR continues in PR #255.