xADObjectPermissionEntry: Fix failure when applied in the same configuration as xADDomain #299
Conversation
- Import MSFT_xADCommon module - Localize and improve verbose messages - standardise synopsis permission text - Add Assert-ADSPDrive function calls
Codecov Report
@@ Coverage Diff @@
## dev #299 +/- ##
====================================
+ Coverage 90% 90% +<1%
====================================
Files 20 20
Lines 2224 2238 +14
Branches 10 10
====================================
+ Hits 2011 2026 +15
+ Misses 203 202 -1
Partials 10 10 |
- Add Assert-ADPSDrive function mocks - Add Assert-ADPSDrive MockCalled tests - Fix context name typo
johlju
added
the
waiting for author response
|
@X-Guardian Thanks for sending this in and fixing the verbose messages! Waiting for more input in the issue #296 to make sure that we actually need to assert that AD drive is present, but instead that the RSAT-AD-PowerShell should be required. I will review after we come to a conclusion there. |
X-Guardian
changed the title
johlju
added
needs review
There was a problem hiding this comment.
Great work on this! Just minor review comments. 😃
Reviewed 3 of 3 files at r1, 2 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @X-Guardian)
DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1325 at r4 (raw file):
Function
function (lower-case 'f')
DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1339 at r4 (raw file):
Get-PSDrive -Name AD -ErrorAction Stop | Out-Null
Instead evaluate this with a try-catch block, could we evaluate this with an if-block instead?
$activeDirectoryPSDrive = Get-PSDrive -Name 'AD' -ErrorAction 'SilentlyContinue'
if ($null -eq $activeDirectoryPSDrive)
{
# New-PSDrive...
}I think it's more intuitive to use an if-block here instead of a try-catch block. I think we should use try-catch block when want the throw an error to the user (there are exceptions though, but not seeing this one as those).
DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1341 at r4 (raw file):
Catch
catch (lower-case 'c'). Throughout.
DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1344 at r4 (raw file):
Try
try (lower-case 't'). Throughout.
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1450 at r4 (raw file):
-MockWith { }
We can remove this when there a no code to mock. Throughout (thoughout as in you only have to fix the code you added 🙂 We can clean up the rest of the test in another PR).
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1453 at r4 (raw file):
Throw
throw (lower-case 't')
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1476 at r4 (raw file):
Mock -CommandName New-InvalidOperationException -MockWith { }
We should not need this mock, see next comment.
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1478 at r4 (raw file):
{ Assert-ADPSDrive } | Should Not Throw
We should assert that it actually throws the correct error inside an It-block.
It 'Should throw the correct error' {
{ Assert-ADPSDrive } | Should -Throw $script:localizedString.CreatingNewADPSDriveError
}
The $script:localizedString.CreatingNewADPSDriveError will (should) be available automatically once the test runs (because we run the test inside`InModuelScope').
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1494 at r4 (raw file):
Quoted 4 lines of code…
It 'Should call New-InvalidOperationException once' { Assert-MockCalled -CommandName New-InvalidOperationException -Exactly -Times 1 -Scope Context }
Should not be needed, see comment above.
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1502 at r4 (raw file):
Assert-ADPSDrive
We should have this inside an It-block to so Pester can correctly handle unexpected errors.
It 'Should not throw' {
{ Assert-ADPSDrive } | Should -Not -Throw
}
There was a problem hiding this comment.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @johlju)
DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1325 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Function
function(lower-case 'f')
Done.
DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1339 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Get-PSDrive -Name AD -ErrorAction Stop | Out-NullInstead evaluate this with a try-catch block, could we evaluate this with an if-block instead?
$activeDirectoryPSDrive = Get-PSDrive -Name 'AD' -ErrorAction 'SilentlyContinue' if ($null -eq $activeDirectoryPSDrive) { # New-PSDrive... }I think it's more intuitive to use an if-block here instead of a try-catch block. I think we should use try-catch block when want the throw an error to the user (there are exceptions though, but not seeing this one as those).
Done.
DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1341 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Catch
catch(lower-case 'c'). Throughout.
Done.
DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 1344 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Try
try(lower-case 't'). Throughout.
Done.
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1450 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
-MockWith { }We can remove this when there a no code to mock. Throughout (thoughout as in you only have to fix the code you added 🙂 We can clean up the rest of the test in another PR).
Done.
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1453 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Throw
throw(lower-case 't')
Done.
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1476 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Mock -CommandName New-InvalidOperationException -MockWith { }We should not need this mock, see next comment.
Done.
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1478 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
{ Assert-ADPSDrive } | Should Not ThrowWe should assert that it actually throws the correct error inside an It-block.
It 'Should throw the correct error' { { Assert-ADPSDrive } | Should -Throw $script:localizedString.CreatingNewADPSDriveError }The
$script:localizedString.CreatingNewADPSDriveErrorwill (should) be available automatically once the test runs (because we run the test inside`InModuelScope').
Done. (I didn't know you could do this. Very good!)
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1494 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
It 'Should call New-InvalidOperationException once' { Assert-MockCalled -CommandName New-InvalidOperationException -Exactly -Times 1 -Scope Context }Should not be needed, see comment above.
Done.
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1502 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Assert-ADPSDriveWe should have this inside an It-block to so Pester can correctly handle unexpected errors.
It 'Should not throw' { { Assert-ADPSDrive } | Should -Not -Throw }
Done.
johlju
added
needs review
There was a problem hiding this comment.
Reviewed 1 of 2 files at r6, 1 of 1 files at r7.
Reviewable status:complete! all files reviewed, all discussions resolved
Tests/Unit/MSFT_xADCommon.Tests.ps1, line 1478 at r4 (raw file):
Previously, X-Guardian (Simon Heather) wrote…
Done. (I didn't know you could do this. Very good!)
One never stops learning. Happy I could teach something. 😄 It's the most fun with this work! 🙂
|
Merged! Thank you @X-Guardian! 🙂 |
Pull Request (PR) description
This PR fixes the failure when an
xADObjectPermissionEntryresource is applied in the same configuration as a newxADDomainresource is created. This is achieved using a new common functionAssert-ADPSDrivewhich checks if an AD PSDrive exists, and creates one if it is missing.The verbose messages have also been localized and improved and the synopsis text standardized to use
permissioninstead ofaccess.Old verbose messages:
New verbose messages:
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