xADDomainTrust: Refactor resource and add tests #423
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #423 +/- ##
====================================
+ Coverage 92% 96% +3%
====================================
Files 20 20
Lines 2536 2599 +63
Branches 10 10
====================================
+ Hits 2347 2502 +155
+ Misses 179 87 -92
Partials 10 10 |
johlju
force-pushed
the
add-xADDomainTrust-tests
branch
from
757356d to
8ae34f1
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 2 unresolved discussions
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.schema.mof, line 4 at r1 (raw file):
Valid values are 'Present' and 'Absent'
Remove 'Valid values...' Throughout.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.schema.mof, line 4 at r1 (raw file):
The default is 'Present'.
Change to 'Default value is 'Present'. Throughout.
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, all discussions resolved
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.schema.mof, line 4 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Valid values are 'Present' and 'Absent'Remove 'Valid values...' Throughout.
Done
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.schema.mof, line 4 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
The default is 'Present'.Change to 'Default value is 'Present'. Throughout.
Done
|
If someone in the community would like to review this I would be grateful. It's a big one, so probably gonna take a bit of time. 🙂 |
|
@PlagueHO if you have time I would love it if you could review this one. It is a big one since the whole resource was rewritten since it used a abnormal pattern. Since the same logic was used as in the current resource, there are two big issues (but edge cases) that I wont fix in this PR, submitted issue #424 and #425. |
|
I'm on it. |
There was a problem hiding this comment.
Reviewed 10 of 12 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @johlju)
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 21 at r2 (raw file):
.PARAMETER TargetDomainAdministratorCredential Specifies the credentials to authenticate to the target domain..
Remove second full stop. And throughout.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 24 at r2 (raw file):
.PARAMETER TrustType Specifies the type of trust. Valid values are 'External' or 'Forest'.
If we're moving to auto documentation we shouldn't include the options in here because then we'll get a double up of info. Same with TrustDirection.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 73 at r2 (raw file):
# Create the target object. $newADDirectoryContextParameters = @{
Should be $getADDirectoryContextParameters I think - and throughout.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 189 at r2 (raw file):
) $directoryContextType = ConvertTo-DirectoryContextType -TrustType $TrustType
It looks like we've got a fair chunk of duplicated code here- all the way down to the end of the if block. Is there any way we could split that out into a function to remove the duplication?
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 223 at r2 (raw file):
# Get all properties that are not in desired state. $propertiesNotInDesiredState = $compareTargetResourceStateResult | Where-Object -FilterScript {
I think this needs to be indented
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 242 at r2 (raw file):
) )
Can remove blank line?
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 267 at r2 (raw file):
# Check properties. $trustTypeProperty = $propertiesNotInDesiredState.Where({ $_.ParameterName -eq 'TrustType' }) if ($trustTypeProperty)
Add blank line before.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 295 at r2 (raw file):
<# In case the trust direction property should be wrong, there are no need to update that property twice since it was set
are -> is
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 501 at r2 (raw file):
<# If the user did not specify Ensure property, then it is not part of the $PSBoundParameters, but it still need to be compared.
need -> needs
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 508 at r2 (raw file):
$desiredValues['Ensure'] = $Ensure $compareTargetResourceStateParameters = @{
Should be $compareREsourcePropertyStateParameters I think. And throughout.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 528 at r2 (raw file):
.NOTES This is a wrapper for enable unit testing of this resource.
for -> to
DSCResources/MSFT_xADDomainTrust/README.md, line 3 at r2 (raw file):
# Description The xADDomainTrust DSC resource will manage Domain Trusts within Active Directory. A trust is a relationship, which you establish between domains or forests. That makes it possible for users in one domain to be authenticated by a domain controller in the other domain.
FYI, Strictly a trust does more than just allowing users to be authenticated by other domains. I'm wondering if we should leave out the detail around what a trust is because if a user doesn't know what a trust is then they should probably not be creating them 😁
DSCResources/MSFT_xADDomainTrust/en-US/about_xADDomainTrust.help.txt, line 6 at r2 (raw file):
.DESCRIPTION The xADDomainTrust DSC resource will manage Domain Trusts within Active Directory. A trust is a relationship, which you establish between domains or forests. That makes it possible for users in one domain to be authenticated by a domain controller in the other domain.
Got some white space here and a few other lines.
DSCResources/MSFT_xADDomainTrust/en-US/MSFT_xADDomainTrust.strings.psd1, line 3 at r2 (raw file):
# culture="en-US" ConvertFrom-StringData @' CheckingTrustMessage = Determining if the trust between the '{0}' and the '{1}' with the context type '{2}' exists. (ADDT0001)
Not sure we need the word 'the' - e.g. Determining if the trust between domains '{0}' and '{1}' with the context type '{2}' exists.
Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1, line 1853 at r2 (raw file):
Write-Verbose -Message ($script:localizedData.TypeAlreadyExistInSession -f $TypeName) -Verbose # The type already exist so no need to load the type again.
exist -> exists
Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1, line 1884 at r2 (raw file):
.PARAMETER Name An optional parameter for the target of the directory context.
Remove second spaces between the and target.
Tests/Integration/MSFT_xADDomainTrust.config.ps1, line 20 at r2 (raw file):
$configFile = [System.IO.Path]::ChangeExtension($MyInvocation.MyCommand.Path, 'json') if (Test-Path -Path $configFile)
Add blank line before.
Tests/Integration/MSFT_xADDomainTrust.Integration.Tests.ps1, line 84 at r2 (raw file):
It 'Should return $true when Test-DscConfiguration is run' { Test-DscConfiguration -Verbose | Should -Be 'True'
Should -BeTrue
And throughout
Tests/Unit/xActiveDirectory.Common.Tests.ps1, line 2137 at r2 (raw file):
} Describe 'xActiveDirectory.CommonNew-CimCredentialInstance' {
Missing a slash between module name and function name.
johlju
added
waiting for code fix
- Added new helper functions in xActiveDirectory.Common. - New-CimCredentialInstance - Add-TypeAssembly - New-ADDirectoryContext
- Refactored the resource to enable unit tests, and at the same time changed it to use the same code pattern as the resource xADObjectEnabledState. - Added unit tests (issue dsccommunity#324). - Added comment-based help (issue dsccommunity#337).
- Added integration tests (issue dsccommunity#348).
johlju
force-pushed
the
add-xADDomainTrust-tests
branch
from
8d85283 to
df9e01e
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 30 files reviewed, 18 unresolved discussions (waiting on @PlagueHO)
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 21 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Remove second full stop. And throughout.
Done.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 24 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
If we're moving to auto documentation we shouldn't include the options in here because then we'll get a double up of info. Same with TrustDirection.
Done. Fixed that in the schema, but forgot the comment-based help. Updated with the same as in the schema.mof.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 73 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should be
$getADDirectoryContextParametersI think - and throughout.
Done. Ah, I forgot to when I change the function name. 🙂
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 189 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It looks like we've got a fair chunk of duplicated code here- all the way down to the end of the if block. Is there any way we could split that out into a function to remove the duplication?
Done. Yeah, I thought I would wait for that until I fixed the issue I mentioned which probably will change this in various ways. But since you pointed it out I fixed it. 😉
I don't think it is pretty when the function returns two objects, but that let me remove more duplicate code. I could have returned as hashtable with the objects in. 🤔 Good enough for now? 🙂
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 223 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think this needs to be indented
Done.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 242 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can remove blank line?
Done.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 267 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add blank line before.
Done.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 295 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
are -> is
Done.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 501 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
need -> needs
Done.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 508 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should be
$compareREsourcePropertyStateParametersI think. And throughout.
Done.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 528 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
for -> to
Done.
DSCResources/MSFT_xADDomainTrust/README.md, line 3 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
FYI, Strictly a trust does more than just allowing users to be authenticated by other domains. I'm wondering if we should leave out the detail around what a trust is because if a user doesn't know what a trust is then they should probably not be creating them 😁
Yeah, updated the text.
DSCResources/MSFT_xADDomainTrust/en-US/about_xADDomainTrust.help.txt, line 6 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Got some white space here and a few other lines.
Done. The whitespace is from the cmdlet in DscResource.Tests. Added issue to track this (PowerShell/DscResource.Tests#338), but fixed the function locally and ran it for all *.help.txt
DSCResources/MSFT_xADDomainTrust/en-US/MSFT_xADDomainTrust.strings.psd1, line 3 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Not sure we need the word 'the' - e.g. Determining if the trust between domains '{0}' and '{1}' with the context type '{2}' exists.
Done.
Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1, line 1853 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
exist -> exists
Done.
Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1, line 1884 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Remove second spaces between the and target.
Done.
Tests/Integration/MSFT_xADDomainTrust.config.ps1, line 20 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add blank line before.
Done.
Tests/Integration/MSFT_xADDomainTrust.Integration.Tests.ps1, line 84 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should -BeTrue
And throughout
Done.
Tests/Unit/xActiveDirectory.Common.Tests.ps1, line 2137 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Missing a slash between module name and function name.
Done.
|
@PlagueHO Ready for review again. After a few commits I finally got it to pass the tests. Script Analyzer did not like my new function. ;) I tried to use multiple types in the |
There was a problem hiding this comment.
Great job with that refactoring - I like the pattern! Great job. Only one really minor nitpick!
Reviewed 28 of 28 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 189 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Done. Yeah, I thought I would wait for that until I fixed the issue I mentioned which probably will change this in various ways. But since you pointed it out I fixed it. 😉
I don't think it is pretty when the function returns two objects, but that let me remove more duplicate code. I could have returned as hashtable with the objects in. 🤔 Good enough for now? 🙂
Great job fixing this BTW - I like the use of Tuples here BTW - I'd have used the same pattern.
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 75 at r3 (raw file):
} $trustSource, $trustTarget = Get-TrustTargetAndSourceObject @getTrustTargetAndSourceObject
Total nitpick here @johlju (awesome use of tuples by the way): Get-TrustTargetAndSourceObject returns Source, Target - intuitively the function should be named in the same order the of the tuples - e.g. Get-TrustSourceAndTargetObject.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 75 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Total nitpick here @johlju (awesome use of tuples by the way): Get-TrustTargetAndSourceObject returns Source, Target - intuitively the function should be named in the same order the of the tuples - e.g. Get-TrustSourceAndTargetObject.
Done. Ah, that is more intuitive, didn't think of that. 🙂
Curios. Is this called Tuples? Maybe we can call it that since it returns two objects which could be different types (not in this case though), but it is not a System.Tuple that is returned. But if a function gives more than one object in the output it might be correct to call it Tuples. 🙂 Never use Tuples when coded C# before, used classes instead, and always tried to avoid returning more than one object from a function in PowerShell. But it is cool that it works. 😄
johlju
added
needs review
|
@PlagueHO ready for review again 😃 |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 75 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Done. Ah, that is more intuitive, didn't think of that. 🙂
Curios. Is this called Tuples? Maybe we can call it that since it returns two objects which could be different types (not in this case though), but it is not a
System.Tuplethat is returned. But if a function gives more than one object in the output it might be correct to call it Tuples. 🙂 Never use Tuples when coded C# before, used classes instead, and always tried to avoid returning more than one object from a function in PowerShell. But it is cool that it works. 😄
I guess it isn't strictly a tuple if it is not a System.Tuple, but has some behavioral similarities - so happy to call it that 😁
|
@PlagueHO thank you for the review! I agree that we can call it a tuple. 😄 |
Pull Request (PR) description
it to use the same code pattern as the resource xADObjectEnabledState.
The integration test result is provided in this gist https://gist.github.com/johlju/62c72d492c28604355f602b2aad67668.
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