xUser, xGroup: Add NewName property

[gaelicWizard]

Pull Request (PR) description

Adds optional parameter SamAccountName to User and Group resources to allow setting this property separately. This requires that GroupName be specified using something other than the SamAccountName, e.g. SID.

This Pull Request (PR) fixes the following issues

• See alsö ADUser, ADGroup: Specify samAccountName separately ActiveDirectoryDsc#655

Task list

• Added an entry to the change log under the Unreleased section of the
  file CHANGELOG.md. Entry should say what was changed and how that
  affects users (if applicable), and reference the issue being resolved
  (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 Community Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
• New/changed code adheres to DSC Community Style Guidelines.

---

This change is 

[codecov]

Codecov Report

Merging #715 (ee58c24) into main (dcadffd) will decrease coverage by 0%.
The diff coverage is 21%.

❗ Current head ee58c24 differs from pull request most recent head 746d7ab. Consider uploading reports for the commit 746d7ab to get more accurate results

@@         Coverage Diff         @@
##           main   #715   +/-   ##
===================================
- Coverage    72%    72%   -1%     
===================================
  Files        30     30           
  Lines      4427   4455   +28     
===================================
+ Hits       3213   3219    +6     
- Misses     1214   1236   +22     

Impacted Files	Coverage Δ
...sources/DSC_xGroupResource/DSC_xGroupResource.psm1	69% <21%> (-2%)	⬇️
...Resources/DSC_xUserResource/DSC_xUserResource.psm1	52% <21%> (-2%)	⬇️

[gaelicWizard]

I added SamAccountName to Get-TargetResource...but maybe that's not how it's supposed to work. Should only mandatory parameters be included in Get-TargetResource?

How do I address the code coverage failure?

[johlju]

Get-TargetResource should only have those non-mandatory parameters that are needed to get it to return the current state.

To raise code coverage unit tests need to be added to cover the new code paths. 🙂 See diff view here to see what is not covered: https://app.codecov.io/gh/dsccommunity/xPSDesiredStateConfiguration/compare/715/diff

You only need to add unit tests for the code you add.

[johlju]

Question, should the property be named SamAccountName? That is an Active Directory term and not used by a local group. 🤔
Should we instead name the parameter Name that is actually what it is, that SID should always have that particular group name? Also, consider adding an assert that if the property Name (currently SamAccountName) is set, then the GroupName must be a SID.

[johlju]

I added @PlagueHO to review this, hopefully he hav time to get around to this.

[gaelicWizard]

Question, should the property be named SamAccountName? That is an Active Directory term and not used by a local group. 🤔

That is the name of the property on the .NET class that is used in the module.

Should we instead name the parameter Name that is actually what it is, that SID should always have that particular group name? Also, consider adding an assert that if the property Name (currently SamAccountName) is set, then the GroupName must be a SID.

I assume that GroupName is an SID for nano server (which doesn't work currently in my branch) because it uses Set-LocalGroup, but for the .NET class wrapped by Get-Group and Save-Group it can be any name which unambiguously identifies the group. (Lots of possibilities in AD, but even for local I think there are more than just SID and SamAccountName for local. I think full name works too? )

[gaelicWizard]

Get-TargetResource should only have those non-mandatory parameters that are needed to get it to return the current state.

Ok, then I'm going to revert that change. I just thought I was supposed to add it at first.

You only need to add unit tests for the code you add.

I'm working on unit tests; I have to learn how to unit test first. Give me a day or two maybe? 😃

Thanks!

[gaelicWizard]

Good news! Many of my assumptions were simply wrong, so I'm going to re-fiddle this out a bit. For one, SamAccountName is a property on the class and is populated and returned to the Get-Group function, and accepts the new value successfully! And ignores it. So. Yeah. Alsö, my memory that Full Name could be used to look up a user alsö appears to be just not true; so it's SID or bust. Anyway. I shall be re-redoing this PR momentarily.

Thanks!

[gaelicWizard]

Following what NetAdapterName does in NetworkingDsc, I've named the new property NewName. I'm still trying to work out integration testing for this

[PlagueHO]

I'll review this weekend. Have blocked out the weekend to catch up on DSC.

[gaelicWizard]

I'm closing this PR b/c these changes touch many more files than I understood at first and on another thread someone pointed out that it makes it very hard to review when multiple resources are changed in a single patch. I've opened new PRs for just-one-resource-at-a-time. Thanks!