xADUser: Adding ServicePrincipalNames property

[SSvilen]

Pull Request (PR) description

Adding ServicePrincipalNames property to the xADUser resource. This would enhance the creating of service accounts in Active Directory.

This Pull Request (PR) fixes the following issues

• Fixes xADUser: Add ServicePrincipalNames property #153

Task list

• [x ] Added an entry under the Unreleased section in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• [ x] Resource documentation added/updated in README.md.
• [ x] 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.
• [ x] Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• [ x] New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[SSvilen]

I created my new branch, before I've deleted the branch for my last PR. I guess that is why I see the commits from that branch here too? I'm not sure..

Reviewable status: 0 of 5 files reviewed, all discussions resolved

[codecov-io]

Codecov Report

Merging #274 into dev will increase coverage by <1%.
The diff coverage is 92%.

@@         Coverage Diff          @@
##            dev   #274    +/-   ##
====================================
+ Coverage    90%    90%   +<1%     
====================================
  Files        20     20            
  Lines      2208   2224    +16     
  Branches     10     10            
====================================
+ Hits       1995   2011    +16     
  Misses      203    203            
  Partials     10     10

[SSvilen]

I need to add some pester tests. I'll try to do it in a couple of days.

Reviewable status: 0 of 5 files reviewed, all discussions resolved

[johlju]

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @SSvilen)

---

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

**`[String]`

We should have another * before the existing two * ** to make it a list item. See previous row.

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 382 at r1 (raw file):

$ServicePrincipalNames

Since this is non-mandatory and we don't need the values to get the current values, we should remove this.
See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#get-targetresource-should-not-contain-unused-non-mandatory-parameters

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 449 at r1 (raw file):

$targetResource[$property.Parameter] = $adUser.($property.Parameter);

When this returns the 'ServicePrincipalNames', and it only contains one item, I think we need to cast it to a string array to make sure the correct type is returned, so that Get-DscConfiguration does not complain. 🤔

So if a parameter has the type [System.String[]], then when returning the property I think we need to do [System.String[]] $adUser.($property.Parameter). 🤔

---

DSCResources/MSFT_xADUser/MSFT_xADUser.schema.mof, line 52 at r1 (raw file):

...for the user account

We should have a full stop (.) at the end of the sentence.

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 58 at r1 (raw file):

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

We should have a test for the Get-TargetResource to make sure it returns the correct value(s) for the ServicePrincipalNames property.

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 525 at r1 (raw file):

-ParameterFilter { $Replace.ContainsKey('ServicePrincipalName') }

This is not needed here. Removing the filter from the Mock make sure that if the code calls the mock with the wrong parameters, the mock is used instead of calling the actual Set-ADUser which it would do with the current parameter filter.
It is enough that the parameter filter is on the Assert-MockCalled.

[johlju]

@SSvilen Awesome work on this! Just a few comments. 🙂

[SSvilen]

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

---

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

Previously, johlju (Johan Ljunggren) wrote…

**`[String]`

We should have another * before the existing two * ** to make it a list item. See previous row.

Done.

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 382 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ServicePrincipalNames

Since this is non-mandatory and we don't need the values to get the current values, we should remove this.
See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#get-targetresource-should-not-contain-unused-non-mandatory-parameters

Ive removed it. But doesnt this mean, that the other attributes there should be removed too?

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 449 at r1 (raw file):

$targetResource[$property.Parameter] = $adUser.($property.Parameter);

We do that at line 800. We can do it in the get-targetresource - I just did it in the same way like xADComputer for consistency.

---

DSCResources/MSFT_xADUser/MSFT_xADUser.schema.mof, line 52 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

...for the user account

We should have a full stop (.) at the end of the sentence.

Done.

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 58 at r1 (raw file):

ServicePrincipalNames
How should I name the test? I can think of any good name.

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 525 at r1 (raw file):

ParameterFilter
OK. Same as the as one of the comments above - I took what's already there for consistency. Tbh I don't even understand, where this $replace is coming from.

[johlju]

@SSvilen I don't see another commit pushed to this PR (pushed to your working branch). Your changes has not turned up here yet, probably because you did not do a git push?

[SSvilen]

I did not push yet, because I thought we should clarify or discuss my comments before that.
Should I always push even if I have some open topics?

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

[SSvilen]

Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @johlju)

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 382 at r1 (raw file):

Previously, SSvilen (Svilen) wrote…

Ive removed it. But doesnt this mean, that the other attributes there should be removed too?

I can not remove that line, because those three lines depend on it:
Test-TargetResource
Set-TargetResource
Set-TargetResource

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 525 at r1 (raw file):

Previously, SSvilen (Svilen) wrote…

ParameterFilter
OK. Same as the as one of the comments above - I took what's already there for consistency. Tbh I don't even understand, where this $replace is coming from.

Seems like when I remove it, the pester test is failing with:

[-] Calls 'Set-ADUser' with 'ServicePrincipalNames' when specified 271ms
RuntimeException: You cannot call a method on a null-valued expression.
at Test-ParameterFilter, C:\Program Files\WindowsPowerShell\Modules\Pester\4.7.3\Functions\Mock.ps1: line 1213
at FindMatchingBlock, C:\Program Files\WindowsPowerShell\Modules\Pester\4.7.3\Functions\Mock.ps1: line 1064
at Invoke-Mock, C:\Program Files\WindowsPowerShell\Modules\Pester\4.7.3\Functions\Mock.ps1: line 952
at , : line 69
at Set-TargetResource, C:\xActiveDirectory\DSCResources\MSFT_xADUser\MSFT_xADUser.psm1: line 1280
at ,
C:\xActiveDirectory\Tests\Unit\MSFT_xADUser.Tests.ps1: line 533

[johlju]

It is okay to push whenever you want, but if you write 'Done' on comments it's better to push a commit too, then I can resolve those comments.

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

---

CHANGELOG.md, line 37 at r2 (raw file):

- Fix the description of the property RestoreFromRecycleBin.

Thanks for fixing that bug in the description! Don't think we need this entry since it is a duplicate with the entry above it? If there are already an entry in the unreleased section that covers the change I don't think we need to add another?

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 382 at r1 (raw file):

Previously, SSvilen (Svilen) wrote…

I can not remove that line, because those three lines depend on it:
Test-TargetResource
Set-TargetResource
Set-TargetResource

Yes, we should not use @PSBoundParameters on those lines. We should instead change to this pattern to use @getTargetResourceParameters instead. We should be able to reuse the $getTargetResourceParameters for both entries in the Set-TargetResource.
https://github.com/PowerShell/xActiveDirectory/blob/43f4be93484cbb2f9c87bdf8bffacaf4ec2bad7d/DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1#L409-L426

Can you submit a new issue to track that we should "remove unused non-mandatory parameters from the Get-TargetResource function". We can then fix all the properties in another PR. 🙂

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 449 at r1 (raw file):

Previously, SSvilen (Svilen) wrote…

$targetResource[$property.Parameter] = $adUser.($property.Parameter);

We do that at line 800. We can do it in the get-targetresource - I just did it in the same way like xADComputer for consistency.

Since a recent PR, xADComputer now does this in another way, it cast the value to string array as mentioned in my previous comment.
https://github.com/PowerShell/xActiveDirectory/blob/dev/DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1#L215

Not sure what you mean with line 800, that is in the Test-TagretResource function. Here we need to do it for the Get-TargetResource so we get the correct type when called with Get-DscConfiguration or Invoke-DscResource.

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 58 at r1 (raw file):

Previously, SSvilen (Svilen) wrote…

ServicePrincipalNames
How should I name the test? I can think of any good name.

See example here:
https://github.com/PowerShell/xActiveDirectory/blob/43f4be93484cbb2f9c87bdf8bffacaf4ec2bad7d/Tests/Unit/MSFT_xADComputer.Tests.ps1#L132-L230

But I think you already covered this? I see that you added that looks to me that it covers the changes in this PR. I mark this comment as satisfied, let me know if you think we need to add more changes to the tests, otherwise you can just acknowledge (resolve/ignore) this comment.

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 525 at r1 (raw file):

Previously, SSvilen (Svilen) wrote…

Seems like when I remove it, the pester test is failing with:

[-] Calls 'Set-ADUser' with 'ServicePrincipalNames' when specified 271ms
RuntimeException: You cannot call a method on a null-valued expression.
at Test-ParameterFilter, C:\Program Files\WindowsPowerShell\Modules\Pester\4.7.3\Functions\Mock.ps1: line 1213
at FindMatchingBlock, C:\Program Files\WindowsPowerShell\Modules\Pester\4.7.3\Functions\Mock.ps1: line 1064
at Invoke-Mock, C:\Program Files\WindowsPowerShell\Modules\Pester\4.7.3\Functions\Mock.ps1: line 952
at , : line 69
at Set-TargetResource, C:\xActiveDirectory\DSCResources\MSFT_xADUser\MSFT_xADUser.psm1: line 1280
at ,
C:\xActiveDirectory\Tests\Unit\MSFT_xADUser.Tests.ps1: line 533

You should not get that error 🤔 I would need to debug to see what's going on to help you, but let's just leave it as-is as the others a done this way. 🙂

$Replace the parameter name from the mock of Set-ADUser. Pester takes all the parameters of the original Set-ADUser, it then create a (new) function and adds the same parameters to that.
Basically you can see it like Pester sees it like this. Very simplified, since it don't even run the mock, but the actual function if the parameter filter does not match. SO below is not correct, but maybe it is easier to grasp what Pester does?

function Set-ADUser {
    param ($Replace) # All parameters from Set-ADUser would be available here.
    
    if ($Replace.ContainsKey('ServicePrincipalName'))  # This is the ParameterFilter
    {
         # run mocked code within MockWith {}
    }
    else
    {
        # Run real Set-ADUser
    }
}

[SSvilen]

Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @johlju)

---

CHANGELOG.md, line 37 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

- Fix the description of the property RestoreFromRecycleBin.

Thanks for fixing that bug in the description! Don't think we need this entry since it is a duplicate with the entry above it? If there are already an entry in the unreleased section that covers the change I don't think we need to add another?

Makes sense. TBH I'm still figuring out what needs to go into CHangelog and what not.
Done.

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 382 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Yes, we should not use @PSBoundParameters on those lines. We should instead change to this pattern to use @getTargetResourceParameters instead. We should be able to reuse the $getTargetResourceParameters for both entries in the Set-TargetResource.
https://github.com/PowerShell/xActiveDirectory/blob/43f4be93484cbb2f9c87bdf8bffacaf4ec2bad7d/DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1#L409-L426

Can you submit a new issue to track that we should "remove unused non-mandatory parameters from the Get-TargetResource function". We can then fix all the properties in another PR. 🙂

Done.

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 449 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Since a recent PR, xADComputer now does this in another way, it cast the value to string array as mentioned in my previous comment.
https://github.com/PowerShell/xActiveDirectory/blob/dev/DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1#L215

Not sure what you mean with line 800, that is in the Test-TagretResource function. Here we need to do it for the Get-TargetResource so we get the correct type when called with Get-DscConfiguration or Invoke-DscResource.

I mean that in that line we take whatever is in property 'ServicePrincipalNames' of the object returned by Get-TargetResource and cast it to [string[]]. But I have already done that inside the Get-TargetRersource, doesn't that make that cast unnecessary?

For instance in xADComputer, you generally assign the SPN property system.String[] object here and then here there is another conversion to [System.String[]]. Or am I missing something?

I have anyway added a line in Get-TargetResource that will do the casting.

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 58 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

See example here:
https://github.com/PowerShell/xActiveDirectory/blob/43f4be93484cbb2f9c87bdf8bffacaf4ec2bad7d/Tests/Unit/MSFT_xADComputer.Tests.ps1#L132-L230

But I think you already covered this? I see that you added that looks to me that it covers the changes in this PR. I mark this comment as satisfied, let me know if you think we need to add more changes to the tests, otherwise you can just acknowledge (resolve/ignore) this comment.

I created one test and called it:
It "Should return correct ServicePrincipalNames"

And it's just for ServicePrincipalNames.
For xADUser there are no such tests like for xADComputer. - may be I can open an issue and copy them over?

[johlju]

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

---

CHANGELOG.md, line 37 at r2 (raw file):

Previously, SSvilen (Svilen) wrote…

Makes sense. TBH I'm still figuring out what needs to go into CHangelog and what not.
Done.

Change log should contain enough information so that users know what happened in the latest release, and how it might affect them or improve their workflow/process. Since we don't have release notes, then the change log acts like releases notes too.

---

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 449 at r1 (raw file):
Looks good to me now.

I mean that in that line we take whatever is in property 'ServicePrincipalNames' of the object returned by Get-TargetResource and cast it to [string[]]. But I have already done that inside the Get-TargetRersource, doesn't that make that cast unnecessary?

Yes, that seems redundant to cast it again in the Test-TargetResource function. But lets leave it as-is. Suggest we clean that up in another PR.

For instance in xADComputer, you generally assign the SPN property system.String[] object here and then here there is another conversion to [System.String[]]. Or am I missing something?

I looks redundant to cast it in the Set-TargetResource (line 848) in xADCOmputer too. I will submit an issue for that! I missed that when I refactored the code. 🙂

---

Tests/Unit/MSFT_xADUser.Tests.ps1, line 58 at r1 (raw file):

Previously, SSvilen (Svilen) wrote…

I created one test and called it:
It "Should return correct ServicePrincipalNames"

And it's just for ServicePrincipalNames.
For xADUser there are no such tests like for xADComputer. - may be I can open an issue and copy them over?

Yes, I you would like that, then please open an issue to track that the other properties should be tested as well.

[johlju]

@SSvilen I merged this now! Thank you so much for this! 😃