ADUser: Remove unused non-mandatory parameters from the Get-TargetResource

[SSvilen]

Pull Request (PR) description

According to DSC Resource Style Guidelines & Best Practices there should be only mandatory parameters in the Get-TargetResource function.
The unused non-mandatory parameters are removed.

This Pull Request (PR) fixes the following issues

• Fixes ADUser: Remove unused non-mandatory parameters from the Get-TargetResource function #293

Task list

• Added an entry under the Unreleased section in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in resource directory README.md.
• Resource parameter descriptions added/updated in schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Conceptual help topic added/updated (cultureFolder\about_ResourceName.help.txt).
• 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]

Codecov Report

Merging #487 into dev will decrease coverage by <1%.
The diff coverage is 88%.

@@         Coverage Diff         @@
##            dev   #487   +/-   ##
===================================
- Coverage    98%    98%   -1%     
===================================
  Files        23     23           
  Lines      3050   3062   +12     
  Branches     10     10           
===================================
+ Hits       3008   3017    +9     
- Misses       32     35    +3     
  Partials     10     10

[johlju]

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

---

CHANGELOG.md, line 20 at r1 (raw file):

Get-TargetResource(

Add a space before the parenthesis

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 198 at r1 (raw file):

<#
    .SYNOPSIS

Remove the parameters from this comment-based help as well.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 476 at r1 (raw file):

Quoted 8 lines of code…

    <#
        This is a workaround to make the resource able to enter debug mode.
        For more information see issue https://github.com/PowerShell/ActiveDirectoryDsc/issues/427.
    #>
    if (-not $PSBoundParameters.ContainsKey('CommonName'))
    {
        $CommonName = $UserName
    }

We should be able to remove this from Get-TargetResource, and then we can remove the parameter CommonName as well. 🤔
CommonName is only used by this code. The actual common name should be returned by the code below.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 512 at r1 (raw file):

$Ensure

We should change this to $ensure(lower-case 'e'). Throughout the Get-TargetResource.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 528 at r1 (raw file):

$Password

We can always return $null here since we must not return the password (security risk).

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1166 at r1 (raw file):

    $getParameters = @{ }
    if ($PSBoundParameters.ContainsKey('DomainName'))

Please add a new blank row before each if-block, here and below.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1174 at r1 (raw file):

Quoted 4 lines of code…

    if ($PSBoundParameters.ContainsKey('CommonName'))
    {
        $getParameters['CommonName'] = $PSBoundParameters['CommonName']
    }

This can be removed when the parameter is removed from the function Get-TargetResource.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1840 at r1 (raw file):

    $getParameters = @{ }
    if ($PSBoundParameters.ContainsKey('DomainName'))

Please add a new blank row before each if-block, here and below.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1850 at r1 (raw file):

Quoted 4 lines of code…

    if ($PSBoundParameters.ContainsKey('CommonName'))
    {
        $getParameters['CommonName'] = $PSBoundParameters['CommonName']
    }

This can be removed when the parameter is removed from the function Get-TargetResource.

---

Tests/Unit/MSFT_ADUser.Tests.ps1, line 212 at r1 (raw file):

$testPresentParams.Remove('Ensure')

Instead of using remove here, we should use clone

$getTargetResourceParameters = $testPresentParams.Clone()
$getTargetResourceParameters.Remove('Ensure')

Then use that in the tests.

---

Tests/Unit/MSFT_ADUser.Tests.ps1, line 306 at r1 (raw file):

            BeforeAll {
                $testPresentParams['Ensure'] = 'Present'
            }

Not needed if the previous comment is resolved.

[johlju]

Thank you for this @SSvilen! Just some minor comments, 😃

[X-Guardian]

Are you sure this change can work guys? Both Test-TargetResource and Set-TargetResource call Get-TargetResource with their full list of specified parameters. Get-AdUser only returns values for properties you have explicitly asked for.

[johlju]

I think it works. Get-TargetResource should always return all the properties regardless of the passed parameters. This code builds the properties that should be returned.

https://github.com/PowerShell/ActiveDirectoryDsc/blob/9a190e2c8c74ff7942409d3081144a6c2b182bf5/DSCResources/MSFT_ADUser/MSFT_ADUser.psm1#L494-L508

Not sure if the unit test actually test all the properties. That could be another PR which could be covered by the issue #488.

[johlju]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @SSvilen)

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1166 at r1 (raw file):

if ($PSBoundParameters.ContainsKey('DomainName'))

DOmainName and UserName must always be provided so they could be added directly to the hasttable $getParameters = @{ }. No need to check if they are bound, since they must always be bound.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1840 at r1 (raw file):

if ($PSBoundParameters.ContainsKey('DomainName'))

DOmainName and UserName must always be provided so they could be added directly to the hasttable $getParameters = @{ }. No need to check if they are bound, since they must always be bound.

[X-Guardian]

Ok, yes you are right. ignore me.

[SSvilen]

I somehow got mixed up while merging with dev..Sorry about that..

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

---

CHANGELOG.md, line 20 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Get-TargetResource(

Add a space before the parenthesis

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 198 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Remove the parameters from this comment-based help as well.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 476 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    <#
        This is a workaround to make the resource able to enter debug mode.
        For more information see issue https://github.com/PowerShell/ActiveDirectoryDsc/issues/427.
    #>
    if (-not $PSBoundParameters.ContainsKey('CommonName'))
    {
        $CommonName = $UserName
    }

We should be able to remove this from Get-TargetResource, and then we can remove the parameter CommonName as well. 🤔
CommonName is only used by this code. The actual common name should be returned by the code below.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 512 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Ensure

We should change this to $ensure(lower-case 'e'). Throughout the Get-TargetResource.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 528 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Password

We can always return $null here since we must not return the password (security risk).

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1166 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a new blank row before each if-block, here and below.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1166 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ($PSBoundParameters.ContainsKey('DomainName'))

DOmainName and UserName must always be provided so they could be added directly to the hasttable $getParameters = @{ }. No need to check if they are bound, since they must always be bound.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1174 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    if ($PSBoundParameters.ContainsKey('CommonName'))
    {
        $getParameters['CommonName'] = $PSBoundParameters['CommonName']
    }

This can be removed when the parameter is removed from the function Get-TargetResource.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1840 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a new blank row before each if-block, here and below.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1840 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ($PSBoundParameters.ContainsKey('DomainName'))

DOmainName and UserName must always be provided so they could be added directly to the hasttable $getParameters = @{ }. No need to check if they are bound, since they must always be bound.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1850 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    if ($PSBoundParameters.ContainsKey('CommonName'))
    {
        $getParameters['CommonName'] = $PSBoundParameters['CommonName']
    }

This can be removed when the parameter is removed from the function Get-TargetResource.

Done.

---

Tests/Unit/MSFT_ADUser.Tests.ps1, line 212 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$testPresentParams.Remove('Ensure')

Instead of using remove here, we should use clone

$getTargetResourceParameters = $testPresentParams.Clone()
$getTargetResourceParameters.Remove('Ensure')

Then use that in the tests.

Done.

---

Tests/Unit/MSFT_ADUser.Tests.ps1, line 306 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

            BeforeAll {
                $testPresentParams['Ensure'] = 'Present'
            }

Not needed if the previous comment is resolved.

Done.

[johlju]

Yeah I see that. It is much better to use rebase (git rebase) to resolve merge conflicts. If using git pullor git merge I have seen these things happen, might be because it "compares" against the wrong tracking branch (but my guess, not entirely sure).

Do you want to give it a try to rebase so we get rid of the changes that are already in dev? Let me know if you get stuck and happy to help.

In the example below I assume the remote names point to the URLs as specified below. Use git remote -v to see the correct remote names and change the commands accordingly.

• origin: https://github.com/PowerShellActiveDirectoryDsc
• my: https://github.com/SSvilen/ActiveDirectoryDsc

# Get latest changes from upstream dev
git checkout dev
git fetch origin dev

# Rebase local dev branch and force-push to fork (this is meant to update the tracking branch for the working branch)
git rebase origin/dev
git push my --force

# Rebase working branch
git checkout Issue293-ADUser-Remove-unused-parameters
git rebase my/dev

# fix any merge conflicts, stage the files, and use `git rebase --continue` (and `git rebase --skip` when applicable)

# Force-push the working branch to the fork
git push my --force

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

[johlju]

@SSvilen my example above had an error, the third command should be git rebase origin/dev, I updated it. Run it again and see if it works. Sorry about that.

[SSvilen]

Sorry, I have troubles understanding what exactly happens during a rebase... Should I just create another branch and PR?
I see like 15 commits and they have mostly the same names in my branch (locally).
If I do properly rebase, will they go away?

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

[johlju]

A rebase basically takes the commits in the dev branch and then replays your commits on top of it one by one.

Strange that you see 15 commits. There are only 8 commits in this PR, but it should only be 7, so I think if you try the rebase one more time I hope it will be correct. If it is not correct I will look at it. You should not need to make another branch or PR. Let’s try to avoid that. So try to do the rebase again, and we will see the outcome :)

[johlju]

Let me know if another rebase did not help and I will help you look at it.

[SSvilen]

I think I was doing the rebase completely wrong... I was doing a commit after each conflict resolution. So now the commit history has so many commits with the same name. What do we do now? Leave it like that?
I would create a new PR, if that would keep the commit history 'easy on the eye' :)

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

[johlju]

Ah, then I understand that you got more commits, I did the same mistake when I did the rebase first time too. 😊 No worries, it looks fine now, it’s no problem with the extra commits. I squash all the commits when I merge so they will not be a problem.

Awesome work! We learn by mistakes too 😊

I continue the review tomorrow.

[johlju]

Great work! Just minor comments left.

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

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 629 at r2 (raw file):

$ensure

Ensure should only have all lower-case in the Get-TargetResource function since it is a local variable. In the rest of the functions it should still be $Ensure (upper-case 'E') as it is a parameter variable.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 925 at r2 (raw file):

    $getParameters = @{ }
    $getParameters['DomainName'] = $PSBoundParameters['DomainName']
    $getParameters['UserName'] = $PSBoundParameters['UserName']

We could simplify this to

$getParameters = @{
    DomainName = $DomainName
    UserName = $UserName
}

Throughout in the other functions as well that have the same block.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 929 at r2 (raw file):

$getParameters['DomainController'] = $PSBoundParameters['DomainController']

We could simplify this to

$getParameters['DomainController'] = $DomainController

Throughout where the same same pattern are used.

[SSvilen]

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

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 629 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ensure

Ensure should only have all lower-case in the Get-TargetResource function since it is a local variable. In the rest of the functions it should still be $Ensure (upper-case 'E') as it is a parameter variable.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 925 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    $getParameters = @{ }
    $getParameters['DomainName'] = $PSBoundParameters['DomainName']
    $getParameters['UserName'] = $PSBoundParameters['UserName']

We could simplify this to

$getParameters = @{
    DomainName = $DomainName
    UserName = $UserName
}

Throughout in the other functions as well that have the same block.

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 929 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$getParameters['DomainController'] = $PSBoundParameters['DomainController']

We could simplify this to

$getParameters['DomainController'] = $DomainController

Throughout where the same same pattern are used.

Done.

[johlju]

Almost there, just two small fixes left! After this is ready to be merged I think. 🙂

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SSvilen)

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 922 at r3 (raw file):

$getParameters = @{ }

We can remove this since it is not needed (it is set on the next line)

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1592 at r3 (raw file):

$getParameters = @{ }

We can remove this since it is not needed (it is set on the next line)

[SSvilen]

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

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 922 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$getParameters = @{ }

We can remove this since it is not needed (it is set on the next line)

Done.

---

DSCResources/MSFT_ADUser/MSFT_ADUser.psm1, line 1592 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$getParameters = @{ }

We can remove this since it is not needed (it is set on the next line)

Done.

[johlju]

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

[johlju]

@SSvilen awesome work on this! Thank you! 😃