Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADOrganizationalUnit: Adding Server parameter #629

Closed
wants to merge 1 commit into from

Conversation

Clebam
Copy link

@Clebam Clebam commented Oct 5, 2020

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

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 Reviewable

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! Great work on this. I have a few comments, but they should be fairly ease to resolve. If you have any questions let me know and I help the best I can. 🙂

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @Clebam)

a discussion (no related file):
Please update the schema file too. I think we should name the new parameter DomainController to align it with for example ADComputer resource

[Write, Description("Specifies the Active Directory Domain Services instance to connect to perform the task.")] String DomainController;
`` 

a discussion (no related file):
Please update the CHANGELOG.md explaining this change and reference to the issue it fixes.


a discussion (no related file):
Please update the unit tests to test this new parameter (that is test the new code).



source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 46 at r1 (raw file):

[System.String]

Should be decorated with [Parameter()].


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 47 at r1 (raw file):

"localhost"

This should not have a default value. See other comments.


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 47 at r1 (raw file):

$Server

Should be renamed to DomainController to align with other resource, see other comment too.


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 56 at r1 (raw file):

Get-ADOrganizationalUnit -Server $Server

The parameter Server should only be used if the parameter DomainController is assigned a value. Suggest splatting the parameters to the cmdlet Get-ADOrganizationalUnit so the parameter hashtable can be built depeding if the parameter is assigned to not ($PSBoundParameters.ContainsKey('DomainController')).


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 125 at r1 (raw file):

Server

Rename to DomainController.


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 134 at r1 (raw file):

Test-TargetResource

Test-TargetResource must specify the DomainController parameter even if it is not used. The comment-based help should specify "Not used in Test-TargetResource' if it is not used.


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 175 at r1 (raw file):

$targetResource = Get-TargetResource -Name $Name -Path $Path

This should also pass the DomainController parameter, but only if it has been assigned ($PSBoundParameters.ContainsKey('DomainController')).


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 254 at r1 (raw file):

Server

Rename to DomainController.


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 268 at r1 (raw file):

Set-TargetResource

Set-TargetResource must specify the DomainController parameter.


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 308 at r1 (raw file):

$targetResource = Get-TargetResource -Name $Name -Path $Path

This should also pass the DomainController parameter, but only if it has been assigned ($PSBoundParameters.ContainsKey('DomainController')).


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 320 at r1 (raw file):

Server = $Server

This should only be passed it is has been assigned a value ($PSBoundParameters.ContainsKey('DomainController')).


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 369 at r1 (raw file):

Server = $Server

This should only be passed it is has been assigned a value ($PSBoundParameters.ContainsKey('DomainController')).


source/DSCResources/MSFT_ADOrganizationalUnit/MSFT_ADOrganizationalUnit.psm1, line 419 at r1 (raw file):

Server = $Server

This should only be passed it is has been assigned a value ($PSBoundParameters.ContainsKey('DomainController')).

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. invalid Used for PR's to tell hacktoberfest that the PR should not count labels Oct 6, 2020
@stale
Copy link

stale bot commented Oct 26, 2020

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.

@stale stale bot added the abandoned The pull request has been abandoned. label Oct 26, 2020
@X-Guardian
Copy link
Contributor

Closing PR as it is abandoned. Please reopen if you wish to continue work on this.

@X-Guardian X-Guardian closed this Dec 26, 2020
@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned. invalid Used for PR's to tell hacktoberfest that the PR should not count
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ADOrganizationalUnit: Add DomainController parameter
3 participants