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

xADDomainController: Added new parameter to support IFM #238

Merged
merged 12 commits into from
Jan 29, 2019

Conversation

rchristman89
Copy link

@rchristman89 rchristman89 commented Jan 15, 2019

Pull Request (PR) description

Added new parameter to allow Installation Media to be used with a new Domain Controller install.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry under the Unreleased section of the change log in the README.md.
    Entry should say what was changed, and how that affects users (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 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 Reviewable

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff          @@
##            dev   #238    +/-   ##
====================================
+ Coverage    84%    84%   +<1%     
====================================
  Files        16     16            
  Lines      1433   1437     +4     
  Branches     10     10            
====================================
+ Hits       1206   1210     +4     
  Misses      217    217            
  Partials     10     10

Copy link
Author

@rchristman89 rchristman89 left a comment

Choose a reason for hiding this comment

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

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


DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 49 at r2 (raw file):

Previously, rchristman89 (Ryan Christman) wrote…

@regedit32 I did find that the DCPromo process logs to C:\Windows\debug\dcpromoui.log that records the IFM path. I could add this in the Get but I am struggling with the benefit that it would provide and if that log gets deleted then it would get $null from it.

Thoughts?

Done.


DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 103 at r2 (raw file):

Previously, regedit32 (Reggie Gibson) wrote…

You have System.String and String on the same parameter:

Done.


DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 210 at r2 (raw file):

Previously, rchristman89 (Ryan Christman) wrote…

Good catch. Missed it on all three. Fixed in next commit.

Done.


DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 238 at r2 (raw file):

Previously, regedit32 (Reggie Gibson) wrote…

We want the validatescript on Set-TargetResource, but not Get or Test. The error in the first screenshot looks like the parameter was removed from Get-TargetResource, so when we splat @PsBoundParameters to it, $InstallMediaPath was not a valid parameter.

Done.

Copy link
Author

@rchristman89 rchristman89 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, 1 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @regedit32)

Copy link
Author

@rchristman89 rchristman89 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @regedit32)

Copy link
Member

@regedit32 regedit32 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rchristman89)


Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 205 at r1 (raw file):

Quoted 4 lines of code…
Mock -CommandName Get-TargetResource -MockWith {
                return $stubTargetResource = @{
                    Ensure = $false
                }

I think you mean to just return the hashtable here like this:

Mock -CommandName Get-TargetResource -MockWith {
    @{
        Ensure = $false
    }
}

Copy link
Author

@rchristman89 rchristman89 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @rchristman89 and @regedit32)


Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 205 at r1 (raw file):

Previously, regedit32 (Reggie Gibson) wrote…
Mock -CommandName Get-TargetResource -MockWith {
                return $stubTargetResource = @{
                    Ensure = $false
                }

I think you mean to just return the hashtable here like this:

Mock -CommandName Get-TargetResource -MockWith {
    @{
        Ensure = $false
    }
}

Yup. Fixed and commited

Copy link
Member

@regedit32 regedit32 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r1.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @rchristman89)

Copy link
Author

@rchristman89 rchristman89 left a comment

Choose a reason for hiding this comment

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

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

@mbreakey3 mbreakey3 merged commit 854ca06 into dsccommunity:dev Jan 29, 2019
rchristman89 added a commit to rchristman89/xActiveDirectory that referenced this pull request Jan 29, 2019
Merge pull request dsccommunity#238 from rchristman89/dev
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add IFM option to xADDomainController install
4 participants