New resource xADForestProperties (Fixes #177)

[regedit32]

Created a new resource to handle managing UPN and SPN suffixes in a Forest similar to this manual process Add User Principal Name Suffixes. Opened issue #177 for this proposed resource.

---

This change is 

[codecov-io]

Codecov Report

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

@@         Coverage Diff          @@
##            dev   #178    +/-   ##
====================================
+ Coverage    82%    84%    +1%     
====================================
  Files        15     16     +1     
  Lines      1333   1433   +100     
  Branches     10     10            
====================================
+ Hits       1106   1206   +100     
  Misses      217    217            
  Partials     10     10

[johlju]

I will help and review this one in a couple of days or so, if someone haven't until then,

[johlju]

Reviewed 3 of 5 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 37 unresolved discussions.

---

a discussion (no related file):
Could you please add Fixes #177 to the PR description, or PR title, so that the issue auto-closes on merge.

---

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

xADPrincipalNameSuffix

Discussion about the name in the resource is ongoing in the issue #177. Depending on name change the resource description might need to change a little.

---

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

The xADPrincipalNameSuffix DSC resource will manage User Principal Name (UPN) suffixes and Service Principal Name suffixes in a forest.

* **ForestName**: Specifies the target Active Directory forest for the change

We should add '.' at the end of the sentence here.

---

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

Specifies the User Principal Name (UPN)

Maybe we could change this to 'Specifies one or more User...' since it is an array.

---

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

* **ForestName**: Specifies the target Active Directory forest for the change
* **UserPrincipalNameSuffix**: Specifies the User Principal Name (UPN) Suffix to add/remove. (optional)

Nitpick (non-blocking): Could we have (optional) before the '.', or at the start of the sentence? Or is it more correct to write it this way?

---

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

Specifies the Service Principal Name (SPN)

Maybe we could change this to 'Specifies one or more Service...' since it is an array.

---

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

"Specifies

Seems a double-quote sneaked in here at the beginning or the sentence.

---

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

### Create User Principal Name (UPN) suffixes and Service Principal Name suffixes

In this example, we create a 'NANO-200' computer account in the 'Nano' OU of the 'example.com' Active Directory domain as well as creating an Offline Domain Join Request file.

I think this description is not correct of what the example actually does?

---

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

$params

Could we change this to $parameters to avoid abbreviations?

---

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

"dc.contoso.com"

Could we use single-quotes here instead, to be consistent? Throughout.

We try to avoid using double-quotes where it's not needed (in HQRM). There is not actual guideline for this apparently so I leave this non-blocking.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 22 at r2 (raw file):

. $adCommonFunctions

function Get-TargetResource

Could you please add comment-based help? Throughout.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 45 at r2 (raw file):

        [Parameter()]
        [ValidateSet("Present","Absent")]

Could we use single-quotes here instead, to be consistent? Throughout.

We try to avoid using double-quotes where it's not needed (in HQRM). There is not actual guideline for this apparently so I leave this non-blocking.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 61 at r2 (raw file):

        ServicePrincipalNameSuffix = @($forest.SPNSuffixes)
        Credential = ""
        Ensure = $Ensure

This will return the state as 'Present' or 'Absent' even if it isn't. Could we evaluate if the values are actually in desired state and return the actual state?

---

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

if(

We should use a space between keyword and parenthesis; if (. Throughout.

https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#one-space-between-keyword-and-parenthesis

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 105 at r2 (raw file):

        if($Ensure -eq 'Present')
        {
            $compare = Compare-Object -ReferenceObject $UserPrincipalNameSuffix -DifferenceObject $forest.UPNSuffixes

Can we have a logical error here 🤔

What happens in this scenario.

1. If the user want's to add UPN 'a' and there are no UPN's yet, LCM will run and this will correctly be false (different).
2. Next time LCM runs, the user still wants want's to add UPN 'a' and now there is a UPN 'a' already, this will correctly be $true (same).
3. Another user manually adds UPN 'b' to the forest.
4. LCM runs and user still wants want's to add only UPN 'a', and now there is a UPN 'a' and 'b' - won't this wrongly be false (different) and will set $inDesiredState = $false and try to run Set-TargetResource?

Same for SPN below.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 141 at r2 (raw file):

ForestSPNSuffixNotInDesiredState

Should be ForestSpnSuffixNotInDesiredState (Pascal Case), the same as it is in the strings-file already.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 152 at r2 (raw file):

SPNSuffixNotInDesiredState

Should be SpnSuffixNotInDesiredState (Pascal Case), the same as it is in the strings-file already.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 160 at r2 (raw file):

                if ($suffix -in $forest.SPNSuffixes)
                {
                    Write-Verbose -Message ($localizedData.SPNSuffixNotInDesiredState -f $suffix)

Same as previous comment.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 200 at r2 (raw file):

$setParams

Could we change this to $setADForestParameters to avoid abbreviations and more clearly show what parameters we are building (especially when reviewing 😄)?

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 200 at r2 (raw file):

@{Identity = $ForestName}

Please change the hash tables according to the style guideline. Throughout.

https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-hashtables-or-objects

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 208 at r2 (raw file):

    if($Ensure -eq 'Present')
    {
        $action = 'Replace'

If we use replace, that means any manually added suffixes will me removed? If so, maybe we should add only the one that is missing?

If we should have the option of replacing all in the forest, then maybe we should have three parameters;

• UserPrincipalNameSuffix - This we replace all UPN in the forest. Cannot be used at the same time as UserPrincipalNameSuffixToInclude and UserPrincipalNameSuffixToExclude
• UserPrincipalNameSuffixToInclude - This add only these UPN's. Can be used at the same time as UserPrincipalNameSuffixToExclude.
• UserPrincipalNameSuffixToExclude - This will remove only these UPN's. Can be used at the same time as UserPrincipalNameSuffixToInclude

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.schema.mof, line 4 at r2 (raw file):

class MSFT_xADPrincipalNameSuffix : OMI_BaseResource
{
    [Key, Description("Specifies the target Active Directory forest for the change")] String ForestName;

Could we add a '.' to the end of the sentence (for the description)?

---

DSCResources/MSFT_xADPrincipalNameSuffix/en-us/MSFT_xADPrincipalNameSuffix.strings.psd1, line 4 at r2 (raw file):

User Principal Name Suffix for '{0}' not in the desired state.

Could we add '...for forest '{0}'...', because it's nearly identical to the text in UpnSuffixNotInDesiredState.

---

DSCResources/MSFT_xADPrincipalNameSuffix/en-us/MSFT_xADPrincipalNameSuffix.strings.psd1, line 5 at r2 (raw file):

    GetForest = Getting forest suffix information from {0}.
    ForestUpnSuffixNotInDesiredState = User Principal Name Suffix for '{0}' not in the desired state.
    ForestSpnSuffixNotInDesiredState = Service Principal Name Suffix for '{0}' not in the desired state.

Same as previous comment.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 1 at r2 (raw file):

$Global:

Should be $global (lower-.case 'g'). Throughout.
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#script-environment-and-global-variable-names-include-scope

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 20 at r2 (raw file):

{
    InModuleScope $Global:DSCResourceName {

Can we remove this extra blank line?

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 24 at r2 (raw file):

$testpresentParams

Please use camelCase on local variables; $testPresentParams (upper 'P'). Throughout.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 34 at r2 (raw file):

        $testAbsentParams['Ensure'] = 'Absent'

    

Can we remove this extra blank line?

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 47 at r2 (raw file):

Mock Assert-Module

Please use named parameters. Mock -CommandName Assert-Module. Throughout.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 47 at r2 (raw file):

-MockWith { }

This parameter is not necessary when there is no need to mock with any code. Throughout.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 56 at r2 (raw file):

            It 'Should Return a "System.Collections.Hashtable" object type' {
                $targetResource -is [System.Collections.Hashtable] | Should Be $true

Nitpick: This could be changed to$targetResource | Should -BeOfType [System.Collections.Hashtable]

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 60 at r2 (raw file):

Should be

We try to write these as Should Be (upper 'B') , or in the new Pester v4 format Should -Be. Throughout.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 73 at r2 (raw file):

        
        Describe "$($Global:DSCResourceName)\Test-TargetResource" {

Can we remove this extra blank line?

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 80 at r2 (raw file):

matche

Typo here, I think it should be 'match'?

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 105 at r2 (raw file):

                Set-TargetResource @testPresentParams -Credential $testCredential

                Assert-MockCalled Set-ADForest -ParameterFilter { $Credential -eq $testCredential } -Scope It             

Please used named parameters -CommandName. Also, it wouldn't hurt if we add -Exactly -Times 1 to know that it is actually called so many times as expected. Throughout.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 109 at r2 (raw file):

.replace

Can we change this to .Replace. Upper 'R'. Throughout.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 117 at r2 (raw file):

.remove

Can we change this to .Remove. Upper 'R'. Throughout.

---

Comments from Reviewable

[regedit32]

@johlju Thanks for the review, I'll make those updates early this week.

[johlju]

@regedit32 Have you had the chance to look at the review comments? This PR also needs to be rebase against dev (using git rebase).

[regedit32]

Thanks @johlju . I started making the changes, but it ended up being a larger rework and I haven't had a chance to get back to it.

[johlju]

@regedit32 No worries, then I know you are still active on this PR. I will wait for you to update the review comments once you are done. 🙂

[johlju]

@regedit32 don't want to rush you in any way, just checking in since it was a month since the last update. Do you see you have a chance to continue the work on this PR?

[regedit32]

@johlju I hope to eventually get back to this and make the changes, but it's a little off my radar at the moment.

[johlju]

@regedit32 Thanks for the update!

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

[regedit32]

Made a lot of changes to the resource and test since I last visited 😄 . Also added example files.

Reviewable status: 0 of 7 files reviewed, 33 unresolved discussions (waiting on @johlju and @regedit32)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add Fixes #177 to the PR description, or PR title, so that the issue auto-closes on merge.

Done.

---

---

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

Previously, johlju (Johan Ljunggren) wrote…

xADPrincipalNameSuffix

Discussion about the name in the resource is ongoing in the issue #177. Depending on name change the resource description might need to change a little.

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

We should add '.' at the end of the sentence here.

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

Specifies the User Principal Name (UPN)

Maybe we could change this to 'Specifies one or more User...' since it is an array.

Changed description to say 'suffix(es)'

---

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

Previously, johlju (Johan Ljunggren) wrote…

Specifies the Service Principal Name (SPN)

Maybe we could change this to 'Specifies one or more Service...' since it is an array.

Changed description to say 'suffix(es)'

---

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

Previously, johlju (Johan Ljunggren) wrote…

"Specifies

Seems a double-quote sneaked in here at the beginning or the sentence.

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

I think this description is not correct of what the example actually does?

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

$params

Could we change this to $parameters to avoid abbreviations?

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 22 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add comment-based help? Throughout.

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 61 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This will return the state as 'Present' or 'Absent' even if it isn't. Could we evaluate if the values are actually in desired state and return the actual state?

Ensure no longer used due to other changes

---

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

Previously, johlju (Johan Ljunggren) wrote…

if(

We should use a space between keyword and parenthesis; if (. Throughout.

https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#one-space-between-keyword-and-parenthesis

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 105 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we have a logical error here 🤔

What happens in this scenario.

1. If the user want's to add UPN 'a' and there are no UPN's yet, LCM will run and this will correctly be false (different).
2. Next time LCM runs, the user still wants want's to add UPN 'a' and now there is a UPN 'a' already, this will correctly be $true (same).
3. Another user manually adds UPN 'b' to the forest.
4. LCM runs and user still wants want's to add only UPN 'a', and now there is a UPN 'a' and 'b' - won't this wrongly be false (different) and will set $inDesiredState = $false and try to run Set-TargetResource?

Same for SPN below.

Updated to allow keeping existing suffix(es)

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 141 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

ForestSPNSuffixNotInDesiredState

Should be ForestSpnSuffixNotInDesiredState (Pascal Case), the same as it is in the strings-file already.

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 152 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SPNSuffixNotInDesiredState

Should be SpnSuffixNotInDesiredState (Pascal Case), the same as it is in the strings-file already.

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 160 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as previous comment.

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 200 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$setParams

Could we change this to $setADForestParameters to avoid abbreviations and more clearly show what parameters we are building (especially when reviewing 😄)?

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 200 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

@{Identity = $ForestName}

Please change the hash tables according to the style guideline. Throughout.

https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-hashtables-or-objects

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.psm1, line 208 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If we use replace, that means any manually added suffixes will me removed? If so, maybe we should add only the one that is missing?

If we should have the option of replacing all in the forest, then maybe we should have three parameters;

• UserPrincipalNameSuffix - This we replace all UPN in the forest. Cannot be used at the same time as UserPrincipalNameSuffixToInclude and UserPrincipalNameSuffixToExclude
• UserPrincipalNameSuffixToInclude - This add only these UPN's. Can be used at the same time as UserPrincipalNameSuffixToExclude.
• UserPrincipalNameSuffixToExclude - This will remove only these UPN's. Can be used at the same time as UserPrincipalNameSuffixToInclude

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/MSFT_xADPrincipalNameSuffix.schema.mof, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a '.' to the end of the sentence (for the description)?

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 1 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Global:

Should be $global (lower-.case 'g'). Throughout.
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#script-environment-and-global-variable-names-include-scope

Done, but also changed to script scope

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 20 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we remove this extra blank line?

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 24 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$testpresentParams

Please use camelCase on local variables; $testPresentParams (upper 'P'). Throughout.

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 34 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we remove this extra blank line?

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 47 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Mock Assert-Module

Please use named parameters. Mock -CommandName Assert-Module. Throughout.

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 47 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-MockWith { }

This parameter is not necessary when there is no need to mock with any code. Throughout.

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 60 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should be

We try to write these as Should Be (upper 'B') , or in the new Pester v4 format Should -Be. Throughout.

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 73 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we remove this extra blank line?

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 80 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

matche

Typo here, I think it should be 'match'?

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 105 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please used named parameters -CommandName. Also, it wouldn't hurt if we add -Exactly -Times 1 to know that it is actually called so many times as expected. Throughout.

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 109 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

.replace

Can we change this to .Replace. Upper 'R'. Throughout.

Done.

---

Tests/Unit/MSFT_xADPrincipalNameSuffix.Tests.ps1, line 117 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

.remove

Can we change this to .Remove. Upper 'R'. Throughout.

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/en-us/MSFT_xADPrincipalNameSuffix.strings.psd1, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

User Principal Name Suffix for '{0}' not in the desired state.

Could we add '...for forest '{0}'...', because it's nearly identical to the text in UpnSuffixNotInDesiredState.

Done.

---

DSCResources/MSFT_xADPrincipalNameSuffix/en-us/MSFT_xADPrincipalNameSuffix.strings.psd1, line 5 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as previous comment.

Done.

[johlju]

Reviewed 11 of 11 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johlju and @regedit32)

---

DSCResources/MSFT_xADForestProperties/MSFT_xADForestProperties.psm1, line 107 at r3 (raw file):

[Array]$forest.UpnSuffixes

We should have a space between the type and the variable name. Throughout.

---

DSCResources/MSFT_xADForestProperties/MSFT_xADForestProperties.psm1, line 327 at r3 (raw file):

-f 'replacing with'

If we do it liek this, we can't localize the string 'replacing with'. You could add 'replacing with' as a separate localized string too, but that might be difficult to get grammatically correct in other languages. Maybe it is best to have separate strings even if they are mostly duplicate. 🤔

Throughout.

---

DSCResources/MSFT_xADForestProperties/MSFT_xADForestProperties.schema.mof, line 7 at r3 (raw file):

UserPrincipalNameSuffixToExclude

This property actually removes UPN already present in the forest? The word 'Exclude' is not intuitive then, I misinterpreted it as it would exclude one of those in the include property.
Maybe it's better to name this UserPrincipalNameSuffixToRemove ('..:Remove') if that what it actually does?

---

DSCResources/MSFT_xADForestProperties/MSFT_xADForestProperties.schema.mof, line 10 at r3 (raw file):

ServicePrincipalNameSuffixToExclude

Same as previous comment.

[johlju]

@regedit32 Awesome work on this! 😃

[regedit32]

Thank you @johlju !

Reviewable status: 1 of 7 files reviewed, 5 unresolved discussions (waiting on @johlju and @regedit32)

---

DSCResources/MSFT_xADForestProperties/MSFT_xADForestProperties.psm1, line 107 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[Array]$forest.UpnSuffixes

We should have a space between the type and the variable name. Throughout.

Done.

---

DSCResources/MSFT_xADForestProperties/MSFT_xADForestProperties.psm1, line 327 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-f 'replacing with'

If we do it liek this, we can't localize the string 'replacing with'. You could add 'replacing with' as a separate localized string too, but that might be difficult to get grammatically correct in other languages. Maybe it is best to have separate strings even if they are mostly duplicate. 🤔

Throughout.

Ah of course. Separate strings would be better. Done.

---

DSCResources/MSFT_xADForestProperties/MSFT_xADForestProperties.schema.mof, line 7 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

UserPrincipalNameSuffixToExclude

This property actually removes UPN already present in the forest? The word 'Exclude' is not intuitive then, I misinterpreted it as it would exclude one of those in the include property.
Maybe it's better to name this UserPrincipalNameSuffixToRemove ('..:Remove') if that what it actually does?

Agreed. Changed to UserPrincipalNameSuffixToRemove

---

DSCResources/MSFT_xADForestProperties/MSFT_xADForestProperties.schema.mof, line 10 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

ServicePrincipalNameSuffixToExclude

Same as previous comment.

Done.

[johlju]

Reviewed 6 of 6 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @regedit32)

---

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

* **Credential**: (optional) "Specifies the user account credentials to use to perform this task.
* **ForestName**: Specifies the target Active Directory forest for the change.
* **ServicePrincipalNameSuffix**: (optional) Specifies one or more Service Principal Name (SPN) Suffix to add/remove.

Can we add to the description when this parameter is used, all members are replaced (and the same update to comment-based help and in schema.mof too)?

---

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

ServicePrincipalNameSuffix

I think we should mention that this parameter cannot be used at the same time as the other two. Otherwise users might try to use all three at the same time 🤔
Maybe that works though, I haven't checked the code, I just assume this parameter is mutually exclusive. with the other two.

---

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

Include

When we use Remove on the other once, using Include on this reads strange 🤔 Would it be possible to maybe change Include to Add, or similar? What do you think?

[regedit32]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @regedit32 and @johlju)

---

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

Previously, johlju (Johan Ljunggren) wrote…

Can we add to the description when this parameter is used, all members are replaced (and the same update to comment-based help and in schema.mof too)?

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

ServicePrincipalNameSuffix

I think we should mention that this parameter cannot be used at the same time as the other two. Otherwise users might try to use all three at the same time 🤔
Maybe that works though, I haven't checked the code, I just assume this parameter is mutually exclusive. with the other two.

Done.

---

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

Previously, johlju (Johan Ljunggren) wrote…

Include

When we use Remove on the other once, using Include on this reads strange 🤔 Would it be possible to maybe change Include to Add, or similar? What do you think?

Yeah, I should have saw that too. Changed to Add.

[johlju]

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

[johlju]

@regedit32 Awesome work on this one as usual! Thank you! 😃