xComputer: Fix for Error When Joining a Domain and Renaming a Computer when JoinOU is specified #222
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #222 +/- ##
====================================
+ Coverage 83% 83% +<1%
====================================
Files 10 10
Lines 1068 1074 +6
====================================
+ Hits 891 897 +6
Misses 177 177 |
|
Can someone review this PR? Cheers. |
There was a problem hiding this comment.
Sorry about delay @X-Guardian - I haven't had any time outside my day job lately 😢 planning to catch up over the weekend.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @X-Guardian)
CHANGELOG.md, line 6 at r2 (raw file):
- xComputer: - Fix for 'Directory Service is Busy' Error When Joining a Domain and Renaming
Nitpicks (sorry 😁 ): Can you tweak the capitalization of the first letters of these words and also end with a full stop after the )?
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 260 at r2 (raw file):
catch [System.InvalidOperationException] { if ($_.Exception -like '*The directory service is busy*')
Will this work with non English localized operating systems? I suspect it might not - is there an exception code we can check for instead of a the message string?
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 263 at r2 (raw file):
{ Write-Verbose -Message $script:localizedData.DirectoryBusyMessage Rename-Computer -NewName $Name -DomainCredential $Credential
Can you add a comment that explains why the "Directory service is busy message" indicates that a Rename-Computer is required? Is it possible that this will be a false positive? e.g. the message does not indicate this? Is there a link to some documentation describing this behavior that we can include?
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 272 at r2 (raw file):
catch { New-InvalidOperationException -ErrorRecord $_
This might not be an Invalid-OperationException - yet we're throwing it as that. So perhaps better just rethrow the original exception: throw $_
Tests/Unit/MSFT_Computer.Tests.ps1, line 683 at r2 (raw file):
} It 'Should Try a separate rename if the domain is busy' {
Lower case t for 'try' (sorry - I know, total nitpick 😁)
Tests/Unit/MSFT_Computer.Tests.ps1, line 686 at r2 (raw file):
Mock -CommandName Get-WMIObject -MockWith { [PSCustomObject] @{ Domain = 'Contoso';
Can you remove ; at the end? They aren't required when splitting hash table onto multiple lines.
Tests/Unit/MSFT_Computer.Tests.ps1, line 709 at r2 (raw file):
Assert-MockCalled -CommandName Add-Computer -Exactly -Times 1 -Scope It -ParameterFilter { $DomainName -and $NewName } Assert-MockCalled -CommandName Add-Computer -Exactly -Times 0 -Scope It -ParameterFilter { $WorkGroupName } }
The indent on this line isn't correct.
Tests/Unit/MSFT_Computer.Tests.ps1, line 712 at r2 (raw file):
It 'Should Throw the correct error if Add-Computer errors with an unknown InvalidOperationException' {
Can you remove this blank line?
Tests/Unit/MSFT_Computer.Tests.ps1, line 718 at r2 (raw file):
Mock -CommandName Get-WMIObject -MockWith { [PSCustomObject] @{ Domain = 'Contoso';
Can you remove ; at the end? They aren't required when splitting hash table onto multiple lines.
Tests/Unit/MSFT_Computer.Tests.ps1, line 746 at r2 (raw file):
Assert-MockCalled -CommandName Add-Computer -Exactly -Times 1 -Scope It -ParameterFilter { $DomainName -and $NewName } Assert-MockCalled -CommandName Add-Computer -Exactly -Times 0 -Scope It -ParameterFilter { $WorkGroupName } }
Indent is not correct.
Tests/Unit/MSFT_Computer.Tests.ps1, line 749 at r2 (raw file):
It 'Should Throw the correct error if Add-Computer errors with an unknown error' {
Can you remove blank line?
Tests/Unit/MSFT_Computer.Tests.ps1, line 753 at r2 (raw file):
Mock -CommandName Get-WMIObject -MockWith { [PSCustomObject] @{ Domain = 'Contoso';
Can you remove ; at the end? They aren't required when splitting hash table onto multiple lines.
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 12 unresolved discussions (waiting on @PlagueHO)
CHANGELOG.md, line 6 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nitpicks (sorry 😁 ): Can you tweak the capitalization of the first letters of these words and also end with a full stop after the )?
Done
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 260 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Will this work with non English localized operating systems? I suspect it might not - is there an exception code we can check for instead of a the message string?
Agreed. Looked at this again, the FullyQualifiedErrorId` property has a usable value. (Don't know how I missed it last time!) I've modified the code and the tests to use this.
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 263 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a comment that explains why the "Directory service is busy message" indicates that a Rename-Computer is required? Is it possible that this will be a false positive? e.g. the message does not indicate this? Is there a link to some documentation describing this behavior that we can include?
Added a comment, and with the better FullyQualilfedErrorId I hope this is better explained. If you look at issue #221, there is an example of the full error, whereby it says the computer has joined the domain successfully but failed on the rename. I've also attached a clixml export of the error record if you want to take a look.
There is no formal documentation on this issue that I can find, but there are lots of Internet articles describing the issue, here are a couple of examples:
- https://social.technet.microsoft.com/Forums/windowsserver/en-US/81105b18-b1ff-4fcc-ae5c-2c1a7cf7bf3d/addcomputer-to-domain-with-new-name-returns-error?forum=winserverpowershell
- https://powershell.org/forums/topic/the-directory-service-is-busy/
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 272 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This might not be an Invalid-OperationException - yet we're throwing it as that. So perhaps better just rethrow the original exception:
throw $_
Done.
Tests/Unit/MSFT_Computer.Tests.ps1, line 683 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Lower case t for 'try' (sorry - I know, total nitpick 😁)
Done.
Tests/Unit/MSFT_Computer.Tests.ps1, line 686 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove ; at the end? They aren't required when splitting hash table onto multiple lines.
Yes I know, I was just copying all the others in the test... Done.
Tests/Unit/MSFT_Computer.Tests.ps1, line 709 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
The indent on this line isn't correct.
Done.
Tests/Unit/MSFT_Computer.Tests.ps1, line 712 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove this blank line?
Done.
Tests/Unit/MSFT_Computer.Tests.ps1, line 718 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove ; at the end? They aren't required when splitting hash table onto multiple lines.
Done.
Tests/Unit/MSFT_Computer.Tests.ps1, line 746 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Indent is not correct.
Done.
Tests/Unit/MSFT_Computer.Tests.ps1, line 749 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove blank line?
Done.
Tests/Unit/MSFT_Computer.Tests.ps1, line 753 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove ; at the end? They aren't required when splitting hash table onto multiple lines.
Done.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @X-Guardian)
CHANGELOG.md, line 6 at r2 (raw file):
Previously, X-Guardian (Simon Heather) wrote…
Done
LOL, sorry, I actually meant the 'Error When Joining a Domain and Renaming a Computer' -> 'error when joining a domain and renaming a computer'.
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 263 at r2 (raw file):
Previously, X-Guardian (Simon Heather) wrote…
Added a comment, and with the better
FullyQualilfedErrorIdI hope this is better explained. If you look at issue #221, there is an example of the full error, whereby it says the computer has joined the domain successfully but failed on the rename. I've also attached a clixml export of the error record if you want to take a look.There is no formal documentation on this issue that I can find, but there are lots of Internet articles describing the issue, here are a couple of examples:
Can you maybe just include those links in a comment in the code?
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 254 at r3 (raw file):
# Rename the computer, and join it to the domain. $script:FailToRenameAfterJoinDomainErrorId = 'FailToRenameAfterJoinDomain,Microsoft.PowerShell.Commands.AddComputerCommand'
Does this variable need to be script scoped? It could also be moved up to just before the 'if' where it is actually used which will make it easier to follow the flow.
Tests/Unit/MSFT_Computer.Tests.ps1, line 686 at r2 (raw file):
Previously, X-Guardian (Simon Heather) wrote…
Yes I know, I was just copying all the others in the test... Done.
Thank you. At some point we should remove the other ones too. Something for a future clean up.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @PlagueHO)
CHANGELOG.md, line 6 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
LOL, sorry, I actually meant the 'Error When Joining a Domain and Renaming a Computer' -> 'error when joining a domain and renaming a computer'.
Done
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 263 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you maybe just include those links in a comment in the code?
Done.
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 254 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Does this variable need to be script scoped? It could also be moved up to just before the 'if' where it is actually used which will make it easier to follow the flow.
I originally script scoped it so I could use it in the Pester tests, and then forgot to do so! I've now updated the tests to use it. Other option is to define it outside of the function so it is already script scoped. Which do you prefer?
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @X-Guardian)
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 254 at r3 (raw file):
Previously, X-Guardian (Simon Heather) wrote…
I originally script scoped it so I could use it in the Pester tests, and then forgot to do so! I've now updated the tests to use it. Other option is to define it outside of the function so it is already script scoped. Which do you prefer?
I think defining it outside the function is the approach I prefer.
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 261 at r4 (raw file):
catch [System.InvalidOperationException] { # If the rename failed during the domain join, re-try the rename.
Can you use a comment block here? E.g.
<#
....
#>
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @PlagueHO)
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 254 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think defining it outside the function is the approach I prefer.
Done.
DSCResources/MSFT_Computer/MSFT_Computer.psm1, line 261 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use a comment block here? E.g.
<#
....
#>
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status:complete! all files reviewed, all discussions resolved
Pull Request (PR) description
This PR fixes the
Directory Service is Busyerror when using thexComputerresource to join a domain and rename a computer at the same time, whenJoinOUis also specified.This is achieved by catching the
Add-Computererror, and performing a separateRename-Computerfunction call if this occurs.This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is