-
Notifications
You must be signed in to change notification settings - Fork 83
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
Computer: Delete existing AD Computer object when joining Computer to Domain #386
Computer: Delete existing AD Computer object when joining Computer to Domain #386
Conversation
Codecov Report
@@ Coverage Diff @@
## main #386 +/- ##
===================================
Coverage 90% 90%
===================================
Files 17 17
Lines 1690 1716 +26
===================================
+ Hits 1528 1554 +26
Misses 162 162
|
@nickgw Please change use of backticks in your code to us splatting instead as per https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_splatting?view=powershell-7.2 |
@kilasuit Should be all set now now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickgw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickgw)
Hi @nickgw - I've merged your other PR (Awesome, thank you), but it's caused some conflicts. Can you resolve? |
Thanks @nickgw - another great job and much appreciated. I'll get onto review tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @nickgw)
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 697 at r5 (raw file):
.PARAMETER Name Name of the computer to search for in the given domain
Nit, for consistency: Can you add a full stop at the end of this line?
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 700 at r5 (raw file):
.PARAMETER Domain Domain to search
Nit, for consistency: Can you add a full stop at the end of this line?
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 703 at r5 (raw file):
.PARAMETER Credential Credential to search domain with
Nit, for consistency: Can you add a full stop at the end of this line?
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 728 at r5 (raw file):
$searcher = New-Object -TypeName System.DirectoryServices.DirectorySearcher $searcher.Filter = "(&(objectCategory=computer)(objectClass=computer)(cn=$Name))" if ( $DomainName -notlike "LDAP://*")
Can you remove space after the (
?
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 739 at r5 (raw file):
ArgumentList = @( $DomainName, $($Credential.UserName),
Are the outer braces needed on this line and the next?
E.g. could this be:
ArgumentList = @(
$DomainName,
$Credential.UserName,
$Credential.GetNetworkCredential().password
)
I could be wrong though...
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 746 at r5 (raw file):
$searchRoot = New-Object @params } catch
What exceptions are typically handled here? Can we just ditch the whole try/catch and let the exception get handled by the DSC exception handler?
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 760 at r5 (raw file):
.PARAMETER Path Path to Object to delete
Nit, for consistency: Can you add a full stop at the end of this line?
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 763 at r5 (raw file):
.PARAMETER Credential Credential to authenticate to the domain
Nit, for consistency: Can you add a full stop at the end of this line?
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 786 at r5 (raw file):
ArgumentList = @( $DomainName, $($Credential.UserName),
Are the outer braces needed on this line and the next?
E.g. could this be:
ArgumentList = @(
$DomainName,
$Credential.UserName,
$Credential.GetNetworkCredential().password
)
I could be wrong though...
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 795 at r5 (raw file):
$adsiObj.DeleteTree() } catch
What exceptions are typically handled here? Can we just ditch the whole try/catch and let the exception get handled by the DSC exception handler?
tests/Unit/DSC_Computer.Tests.ps1
line 1286 at r5 (raw file):
} Mock 'New-Object' { New-Object 'fake_adsi_searcher' } `
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Suggestion:
Mock -CommandName New-Object -MockWith {
New-Object -TypeName 'fake_adsi_searcher'
} `
-ParameterFilter {
$TypeName -and
$TypeName -eq 'System.DirectoryServices.DirectorySearcher'
}
tests/Unit/DSC_Computer.Tests.ps1
line 1292 at r5 (raw file):
} It 'Throws if name is to long' {
Can you verify that the expected error is being thrown? E.g. Check the error message if possible. I'm assuming that this is actually checking the parameter verifiers?
Also, start It block with 'Should' (just a standard we adopted):
Code snippet:
It 'Should throw the expected exception if the name is to long' {
tests/Unit/DSC_Computer.Tests.ps1
line 1302 at r5 (raw file):
} It 'Throws if name contains illegal characters' {
It should verify that the expected exception is thrown. Can usually just check the error message if it's mocked, or check the error object if it's a custom exception your code is throwing.
Code snippet:
It 'Should throw the expected exception if name contains illegal characters' {
tests/Unit/DSC_Computer.Tests.ps1
line 1313 at r5 (raw file):
It 'Returns ADSI object with ADSI path ' {
Can you remove blank line here?
tests/Unit/DSC_Computer.Tests.ps1
line 1314 at r5 (raw file):
It 'Returns ADSI object with ADSI path ' { Mock 'New-Object' { New-Object 'fake_adsi_directoryentry' } `
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Suggestion:
Mock -CommandName New-Object -MockWith {
New-Object -TypeName 'fake_adsi_directoryentry'
} `
-ParameterFilter {
$TypeName -and
$TypeName -eq 'System.DirectoryServices.DirectoryEntry'
}
tests/Unit/DSC_Computer.Tests.ps1
line 1330 at r5 (raw file):
It 'Returns ADSI object with domain name' {
Can you remove blank line here?
tests/Unit/DSC_Computer.Tests.ps1
line 1335 at r5 (raw file):
$TypeName -and $TypeName -eq 'System.DirectoryServices.DirectoryEntry' }
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Suggestion:
Mock -CommandName New-Object -MockWith {
New-Object -TypeName 'fake_adsi_directoryentry'
} `
-ParameterFilter {
$TypeName -and
$TypeName -eq 'System.DirectoryServices.DirectoryEntry'
}
tests/Unit/DSC_Computer.Tests.ps1
line 1346 at r5 (raw file):
} It 'Should throw if Credential is incorrect' {
It should verify that the expected exception is thrown. Can usually just check the error message if it's mocked, or check the error object if it's a custom exception your code is throwing.
Suggestion:
It 'Should throw the expected exception if Credential is incorrect' {
tests/Unit/DSC_Computer.Tests.ps1
line 1347 at r5 (raw file):
It 'Should throw if Credential is incorrect' { Mock 'New-Object' { Write-Error -message "error" } `
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Suggestion:
Mock -CommandName New-Object -MockWith {
Write-Error -Message 'error'
} `
-ParameterFilter {
$TypeName -and
$TypeName -eq 'System.DirectoryServices.DirectoryEntry'
}
tests/Unit/DSC_Computer.Tests.ps1
line 1359 at r5 (raw file):
-Credential $credential ` -Verbose } | Should -Throw
Can you verify that the expected exception thrown? E.g. Check the error text?
tests/Unit/DSC_Computer.Tests.ps1
line 1365 at r5 (raw file):
Context 'DSC_Computer\Delete-ADSIObject' {
Can you remove blank line here?
tests/Unit/DSC_Computer.Tests.ps1
line 1373 at r5 (raw file):
} It 'Deletes ADSI Object' {
Suggestion:
It 'Should delete the ADSI Object' {
tests/Unit/DSC_Computer.Tests.ps1
line 1374 at r5 (raw file):
It 'Deletes ADSI Object' { Mock 'New-Object' { New-Object 'fake_adsi_directoryentry' } `
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Suggestion:
Mock -CommandName New-Object -MockWith {
New-Object 'fake_adsi_directoryentry'
} `
-ParameterFilter {
$TypeName -and
$TypeName -eq 'System.DirectoryServices.DirectoryEntry'
}
tests/Unit/DSC_Computer.Tests.ps1
line 1395 at r5 (raw file):
-Credential $credential` -Verbose } | Should -Throw
Is it possible to verify the correct/expected exception is being thrown? It looks like this is checking the parameter verifier - so should be possible?
tests/Unit/DSC_Computer.Tests.ps1
line 1399 at r5 (raw file):
It 'Should throw if Credential is incorrect' { Mock 'New-Object' { Write-Error -message "error" } `
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Suggestion:
Mock -CommandName New-Object -MockWith {
Write-Error -message 'error'
} `
-ParameterFilter {
$TypeName -and
$TypeName -eq 'System.DirectoryServices.DirectoryEntry'
}
tests/Unit/DSC_Computer.Tests.ps1
line 1410 at r5 (raw file):
-Credential $credential ` -Verbose } | Should -Throw
FYI, this might not be possible (why it's info only), but can the exception be verified - e.g that the correct one is being thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 4 files reviewed, 20 unresolved discussions (waiting on @PlagueHO)
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 728 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove space after the
(
?
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 739 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Are the outer braces needed on this line and the next?
E.g. could this be:
ArgumentList = @( $DomainName, $Credential.UserName, $Credential.GetNetworkCredential().password )I could be wrong though...
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 746 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
What exceptions are typically handled here? Can we just ditch the whole try/catch and let the exception get handled by the DSC exception handler?
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 786 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Are the outer braces needed on this line and the next?
E.g. could this be:
ArgumentList = @( $DomainName, $Credential.UserName, $Credential.GetNetworkCredential().password )I could be wrong though...
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 795 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
What exceptions are typically handled here? Can we just ditch the whole try/catch and let the exception get handled by the DSC exception handler?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1286 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1292 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you verify that the expected error is being thrown? E.g. Check the error message if possible. I'm assuming that this is actually checking the parameter verifiers?
Also, start It block with 'Should' (just a standard we adopted):
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1302 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It should verify that the expected exception is thrown. Can usually just check the error message if it's mocked, or check the error object if it's a custom exception your code is throwing.
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1313 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove blank line here?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1314 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1330 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove blank line here?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1335 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1346 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It should verify that the expected exception is thrown. Can usually just check the error message if it's mocked, or check the error object if it's a custom exception your code is throwing.
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1347 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1359 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you verify that the expected exception thrown? E.g. Check the error text?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1365 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove blank line here?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1373 at r5 (raw file):
} It 'Deletes ADSI Object' {
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1374 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1395 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Is it possible to verify the correct/expected exception is being thrown? It looks like this is checking the parameter verifier - so should be possible?
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1399 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use parameter names and remove quotes around command and tweak formatting to align to standards?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @nickgw and @PlagueHO)
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 795 at r5 (raw file):
Previously, nickgw (Nick G) wrote…
Done.
It doesn't look like this changed. But see the discussion above about try...catch
to suppress an exception in DSC.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 751 at r6 (raw file):
catch { New-InvalidOperationException -Message ($script:localizedData.InvalidUserNameorPassword -f $Credential.Username)
Is this the only possible cause of an exception here? If there are other causes, then this exception message would be misleading. Could you just eliminate the try... catch
and just let the method throw its own exception?
E.g. just replace the whole block with:
return $searcher.FindOne()
Although if you want to prevent an exception entirely and allow DSC to keep processing even in the case of an exception then the try...catch
is neccessary - but you're probably not safe assuming that the cause of the exception is a bad username/password. Does that make sense?
tests/Unit/DSC_Computer.Tests.ps1
line 1374 at r5 (raw file):
Previously, nickgw (Nick G) wrote…
Done.
I think this one didn't change (still in incorrect format).
tests/Unit/DSC_Computer.Tests.ps1
line 1399 at r5 (raw file):
Previously, nickgw (Nick G) wrote…
Done.
I think this one didn't change (still in incorrect format).
tests/Unit/DSC_Computer.Tests.ps1
line 1355 at r6 (raw file):
It 'Should throw the expected exception if Credential is incorrect' { Mock 'New-Object' { Write-Error -message "Invalid Credentials" } `
I missed this - can you tweak the format to be:
Mock -CommandName New-Object -MockWith {
Write-Error -Message 'Invalid Credentials'
} `
tests/Unit/DSC_Computer.Tests.ps1
line 1367 at r6 (raw file):
-Credential $credential ` -Verbose } | Should -Throw "Invalid Credentials"
Can you use single quotes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 11 unresolved discussions (waiting on @PlagueHO)
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 795 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It doesn't look like this changed. But see the discussion above about
try...catch
to suppress an exception in DSC.
Done.
source/DSCResources/DSC_Computer/DSC_Computer.psm1
line 751 at r6 (raw file):
doesn't look like this changed. But see the discussion above about try...catch to suppress an exception in DSC.
I see what you're saying and removed the try/catch for both of the dotnet methods.
tests/Unit/DSC_Computer.Tests.ps1
line 1374 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think this one didn't change (still in incorrect format).
I think I got this one now.
tests/Unit/DSC_Computer.Tests.ps1
line 1399 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think this one didn't change (still in incorrect format).
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1355 at r6 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I missed this - can you tweak the format to be:
Mock -CommandName New-Object -MockWith { Write-Error -Message 'Invalid Credentials' } `
Done.
tests/Unit/DSC_Computer.Tests.ps1
line 1367 at r6 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use single quotes here?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job @nickgw - just one minor tweak to the CHANGELOG.md to ensure the release process works correctly and then we can merge.
Reviewed 2 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickgw)
CHANGELOG.md
line 7 at r8 (raw file):
## [Unreleased] - Computer
Minor thing I just noticed that might upset the release process: Can you move this down under the ### Added section? Also, can you add a blank line after the ### Added.
E.g.
### Added
- Computer
- Support Options Parameter for domain join - Fixes [Issue #234](https://github.com/dsccommunity/ComputerManagementDsc/issues/234).
- When joining a computer to a domain, existing AD computer objects will be deleted.
There was a problem hiding this 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, 2 unresolved discussions (waiting on @nickgw)
tests/Unit/DSC_Computer.Tests.ps1
line 1356 at r8 (raw file):
It 'Should throw the expected exception if Credential is incorrect' { Mock -CommandName New-Object -MockWith { Write-Error -message "Invalid Credentials"
Nit, Doh! Missed one minor thing:
Write-Error -Message 'Invalid Credentials'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickgw)
Pull Request (PR) description
When joining a computer to a domain, if a computer object with the same name already exists in the domain it will be deleted.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is