WindowsEventLog: Multiple Updates - Fixes #355, #349, #338, #229 #359

cohdjn · 2021-02-12T14:50:22Z

Pull Request (PR) description

Multiple Updates for WindowsEventLog

This Pull Request (PR) fixes the following issues

Fixes #229
Fixes #338
Fixes #349
Fixes #355

Task list

Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md in resource folder.
Resource parameter descriptions added/updated in 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

cohdjn · 2021-02-21T06:08:47Z

@PlagueHO Any ETA on when you'll be able to do the code review? Anything I can do to help?

PlagueHO · 2021-02-21T06:27:58Z

Hi @cohdjn - sorry about the delay. I'll be back on this week with reviews. Can you resolve the conflicts on this one?

cohdjn · 2021-02-21T08:31:28Z

@PlagueHO I never had this problem before so I hope I fixed the conflict correctly! I'm still quite the newbie at all this but I'm learning! :)

PlagueHO · 2021-02-22T05:21:03Z

/azurepipelines run

azure-pipelines · 2021-02-22T05:21:12Z

Azure Pipelines successfully started running 1 pipeline(s).

PlagueHO

Reviewed 9 of 16 files at r1, 1 of 1 files at r2.
Reviewable status: 10 of 16 files reviewed, 7 unresolved discussions (waiting on @cohdjn)

CHANGELOG.md, line 17 at r2 (raw file):

### Changed

- ScheduledTask

Can you remove this block of changes as it also appears under ### Fixed?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 33 at r2 (raw file):

        $LogName,

        [Parameter()]

We shouldn't pass any parameters except for ones that are defined as Write and Key in the MOF file. Can we change the resource so that the Get-WindowsEventLogRegisteredSource to support this?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 60 at r2 (raw file):

    }

    if ($PSBoundParameters.ContainsKey('RegisteredSource'))

As above, we shouldn't pass the RegisteredSource to this method. Is there a way we can determine the Registered Source parameters without having to pass the SourceName in the first call?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 257 at r2 (raw file):

    }

    if ($PSBoundParameters.ContainsKey('RegisteredSource'))

What would happen

source/Examples/Resources/WindowsEventlog/6-WindowsEventLog_RegisterEventSource_Config.ps1, line 39 at r2 (raw file):

}

<#

Can we split this into another file? These get automatically published to the PS Gallery as DSC scripts so need to be in separate files.

source/Examples/Resources/WindowsEventlog/6-WindowsEventLog_RegisterEventSource_Config.ps1, line 68 at r2 (raw file):

}

<#

Can we split this into another file? These get automatically published to the PS Gallery as DSC scripts so need to be in separate files.

source/Examples/Resources/WindowsEventlog/7-WindowsEventLog_RestrictGuestAccess_Config.ps1, line 39 at r2 (raw file):

}

<#

Can we split this into another file? These get automatically published to the PS Gallery as DSC scripts so need to be in separate files.

cohdjn

Reviewable status: 10 of 16 files reviewed, 7 unresolved discussions (waiting on @cohdjn and @PlagueHO)

CHANGELOG.md, line 17 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove this block of changes as it also appears under ### Fixed?

Done.

Fixed duplicate entries.

cohdjn

Reviewable status: 10 of 16 files reviewed, 7 unresolved discussions (waiting on @PlagueHO)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 33 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

We shouldn't pass any parameters except for ones that are defined as Write and Key in the MOF file. Can we change the resource so that the Get-WindowsEventLogRegisteredSource to support this?

The MOF has RegisteredSource as a Write value. Maybe I'm confused what you're looking for here?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 60 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

As above, we shouldn't pass the RegisteredSource to this method. Is there a way we can determine the Registered Source parameters without having to pass the SourceName in the first call?

Please refer back to my comment above. I'm not sure I understand what I'm doing wrong or what you'd like to see changed?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 257 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

What would happen

I don't understand.

source/Examples/Resources/WindowsEventlog/6-WindowsEventLog_RegisterEventSource_Config.ps1, line 39 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we split this into another file? These get automatically published to the PS Gallery as DSC scripts so need to be in separate files.

Done.

source/Examples/Resources/WindowsEventlog/6-WindowsEventLog_RegisterEventSource_Config.ps1, line 68 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we split this into another file? These get automatically published to the PS Gallery as DSC scripts so need to be in separate files.

Done.

source/Examples/Resources/WindowsEventlog/7-WindowsEventLog_RestrictGuestAccess_Config.ps1, line 39 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we split this into another file? These get automatically published to the PS Gallery as DSC scripts so need to be in separate files.

Done.

PlagueHO

Reviewed 1 of 7 files at r3.
Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @cohdjn and @PlagueHO)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 33 at r2 (raw file):

Previously, cohdjn (David Nelson) wrote…

The MOF has RegisteredSource as a Write value. Maybe I'm confused what you're looking for here?

Doh! I apologize - I meant Required and Key. Had done a lot of reviews that day 😢

cohdjn

Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @PlagueHO)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 33 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Doh! I apologize - I meant Required and Key. Had done a lot of reviews that day 😢

I removed RegisteredSource. I couldn't see a way to make this work within the constraints you requested.

cohdjn · 2021-02-26T06:51:21Z

@PlagueHO The integration test bombed outside of this resource. I don't know what I should do at this point?

PlagueHO · 2021-02-26T06:57:29Z

/AzurePipelines run

azure-pipelines · 2021-02-26T06:57:38Z

Azure Pipelines successfully started running 1 pipeline(s).

cohdjn

Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @PlagueHO)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 33 at r2 (raw file):

Previously, cohdjn (David Nelson) wrote…

I removed RegisteredSource. I couldn't see a way to make this work within the constraints you requested.

Done.

cohdjn

Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @PlagueHO)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 60 at r2 (raw file):

Previously, cohdjn (David Nelson) wrote…

Please refer back to my comment above. I'm not sure I understand what I'm doing wrong or what you'd like to see changed?

Done.

cohdjn

Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @PlagueHO)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 257 at r2 (raw file):

Previously, cohdjn (David Nelson) wrote…

I don't understand.

Done.

cohdjn · 2021-03-03T15:42:58Z

@PlagueHO Have you been able to finish the code review? Anything else you see that should be changed/fixed?

PlagueHO · 2021-03-03T19:38:19Z

Hi @cohdjn - sorry about the delay. Been snowed under with my day job this week. I'll be onto it this weekend.

PlagueHO

Really sorry about the really long delay @cohdjn - I had to take a month off DSC due to day job taking all capacity. Just some minor tweaks - nothing major, then will merge.

Reviewed 4 of 16 files at r1, 6 of 7 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @cohdjn)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 467 at r4 (raw file):

            -SourceName $RegisteredSource

        $currentCategoryResourceFile = Get-WindowsEventLogRegisteredSourceFile `

FYI: Could probably improve the performance here by getting Get-WindowsEventLogRegisteredSourceFile to return an object with properties for each ResourceFileType. Can do this in future, no need to change this PR.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 529 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose? E.g. 'Get windows event log or throw an exception if it does not exist'

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 560 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 598 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 630 at r4 (raw file):

    )

    $file = ''

Can you add blank line after this one?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 667 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 684 at r4 (raw file):

    $eventLogRegistryPath = 'HKLM:\SYSTEM\CurrentControlSet\Services\EventLog'
    $logRegistryPath = Join-Path -Path $eventLogRegistryPath -ChildPath $LogName

Can you add a blank line after this one?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 694 at r4 (raw file):

    }

    if ($logProperties.RestrictGuestAccess -eq 1)

Could you convert this to:

retrurn ($logProperties.RestrictGuestAccess -eq 1)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 706 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose? Sorry, I realize this is an existing function so should have been corrected already.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 736 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 781 at r4 (raw file):

    foreach ($file in $CategoryResourceFile, $MessageResourceFile, $ParameterResourceFile)
    {
        if ($file -ne '' -and -not (Test-Path -Path $file -IsValid))

Can you use -not [System.String]::IsNullOrEmpty($file) to check that $file is not empty?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 829 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 857 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose?

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 901 at r4 (raw file):

    }

    $logSddl = ( $Sddl.Replace(":", ":`n").Replace(")", ")`n").Split() |

Minor code style nitpick: can you remove the space after the first (?
E.g.

 $logSddl = ($Sddl.Replace(":", ":`n").Replace(")", ")`n").Split() |

Can you use parameter names for ForEach-Object and Where-Object?

Also, can you use formatting:

ForEach-Object -Process {
    $_ | Where-Object -FilterScrtip {
        $_ -notmatch 'BG\)$'
     }
} ) -Join ''

Also, to avoid long/nested commands (helps make code more readable), suggest using a temporary variable:
```powershell
$sddlDescriptors = $Sddl.Replace(":", ":`n").Replace(")", ")`n").Split()
$logSddl = (foreach ($sddlDescriptor in $sddlDescriptors) {
    $sddlDescriptor | Where-Object -NotMatch 'BG\)$'
}) - Join ''

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 914 at r4 (raw file):

<#
    .SYNOPSIS
        Helper function for WindowsEventLog.

Can this describe the function purpose?

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 186 at r4 (raw file):

            Context 'When getting a request for a non-existent target resource' {
                It 'Should throw when an event log does not exist' {
                    { Get-TargetResource `

Can we check the error is the one expected? E.g.

                   $errorMessage = $script:localizedData.GetWindowsEventLogFailure -f 'UndefinedLog`
                   { Get-TargetResource `
                        -LogName 'UndefinedLog' `
                        -Verbose
                    } | Should -Throw $errorMessage

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 194 at r4 (raw file):

            Context 'When getting the default target resource' {
                Mock -CommandName Get-EventLog {

Nit pick: can you specify parameter name - e.g. -MockWith

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 197 at r4 (raw file):

                    return $mocks.GetEventLogAppLogDefaults
                }
                Mock -CommandName Get-WinEvent {

Nit pick: can you specify parameter name - e.g. -MockWith - and throughout

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 206 at r4 (raw file):

                        -Verbose

                    $eventLogConfiguration | Should -BeOfType Hashtable

Can we validate the values of what is returned as the required functions are mocked, so we should return expected values.

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 225 at r4 (raw file):

                        -LogName 'UndefinedLog' `
                        -Verbose
                    } | Should -Throw

Can we validate error is expected one?

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 242 at r4 (raw file):

                        -MaximumSizeInBytes 0 `
                        -Verbose
                    } | Should -Throw

Can we validate error is expected one? And throughout?

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 503 at r4 (raw file):

                It 'Should not throw under any circumstances' {
                    { Get-WindowsEventLogRestrictGuestAccess `

Should also mock Mock -CommandName Get-ItemProperty but just leave the -MockWith out (to return $null). Otherwise the test will look at the local machine registry and so the test is not deterministic.

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 553 at r4 (raw file):

                        -SourceName 'PesterTest' `
                        -CategoryResourceFile 'foo>bar'
                    } | Should -Throw

Can we validate error is expected one? And throughout - it may not always be possible, but if it is a custom error we're throwing we should be able to.

cohdjn · 2021-04-06T05:21:10Z

I think I got everything requested. Just lemme know if there's anything else you see amiss. And please continue to nitpick... the only way I'm going to learn and get better is through this kind of iterative process and I appreciate it!

cohdjn

Reviewable status: 17 of 19 files reviewed, 21 unresolved discussions (waiting on @cohdjn and @PlagueHO)

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 529 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose? E.g. 'Get windows event log or throw an exception if it does not exist'

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 560 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 598 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 630 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add blank line after this one?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 667 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 684 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a blank line after this one?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 694 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could you convert this to:
retrurn ($logProperties.RestrictGuestAccess -eq 1)

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 706 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose? Sorry, I realize this is an existing function so should have been corrected already.

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 736 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 781 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use -not [System.String]::IsNullOrEmpty($file) to check that $file is not empty?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 829 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 857 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose?

Done.

source/DSCResources/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1, line 914 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can this describe the function purpose?

Done.

cohdjn

Reviewable status: 17 of 19 files reviewed, 19 unresolved discussions (waiting on @cohdjn and @PlagueHO)

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 186 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we check the error is the one expected? E.g.

                   $errorMessage = $script:localizedData.GetWindowsEventLogFailure -f 'UndefinedLog`
                   { Get-TargetResource `
                        -LogName 'UndefinedLog' `
                        -Verbose
                    } | Should -Throw $errorMessage

Done.

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 206 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we validate the values of what is returned as the required functions are mocked, so we should return expected values.

Done.

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 225 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we validate error is expected one?

Done.

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 242 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we validate error is expected one? And throughout?

Done.

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 553 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we validate error is expected one? And throughout - it may not always be possible, but if it is a custom error we're throwing we should be able to.

Done.

cohdjn

Reviewable status: 17 of 19 files reviewed, 19 unresolved discussions (waiting on @PlagueHO)

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 503 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should also mock Mock -CommandName Get-ItemProperty but just leave the -MockWith out (to return $null). Otherwise the test will look at the local machine registry and so the test is not deterministic.

Done.

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 553 at r4 (raw file):

Previously, cohdjn (David Nelson) wrote…

Done.

Done.

PlagueHO

Looking awesome @cohdjn - just some minor tweaks to validate errors occur in the expected placed and then will merge 😁 thanks again for putting up with the nitpicks and time to review!

Reviewed 1 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cohdjn)

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 186 at r4 (raw file):

Previously, cohdjn (David Nelson) wrote…

Done.

This test seems to have been removed - was that intentional?

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 220 at r6 (raw file):

                It 'Should throw when MaximumSizeInBytes is less than 64KB' {
                    { Test-TargetResource -LogName 'Application' -MaximumSizeInBytes 0 } | Should -Throw

The error records doesn't appear to be validated for the next few tests - is that correct?

E.g.

                  $errorRecord = Get-InvalidArgumentRecord `
                    -Message ($localizedData.InvalidSystemLocaleError -f $script:badSystemLocale) `
                    -ArgumentName 'SystemLocale'
                It 'Should throw expected error when MaximumSizeInBytes is less than 64KB' {
                    { Test-TargetResource -LogName 'Application' -MaximumSizeInBytes 0 } | Should -Throw $errorRecord
                }

See:

ComputerManagementDsc/tests/Unit/DSC_SystemLocale.Tests.ps1

Line 113 in 307bcb8

It 'Should throw the expected exception' {

Or can these exceptions not be validated?

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 529 at r6 (raw file):

                It 'Should throw when the New-EventLog cmdlet encounters an error' {
                    Mock -CommandName New-EventLog -MockWith { throw }

Can you throw a dummy message and check for that? This just ensures that the error is occurring in the place we expect.

e.g.

                    Mock -CommandName New-EventLog -MockWith { throw 'Event Log Error' }
                    { Register-WindowsEventLogSource -LogName 'Application' -SourceName 'PesterTest' } | Should -Throw 'Event Log Error'

And throughout the remaining tests?

cohdjn

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cohdjn)

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 186 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This test seems to have been removed - was that intentional?

No that wasn't intentional! Thank you for catching it!!

cohdjn

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cohdjn and @PlagueHO)

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 220 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

The error records doesn't appear to be validated for the next few tests - is that correct?

E.g.

                  $errorRecord = Get-InvalidArgumentRecord `
                    -Message ($localizedData.InvalidSystemLocaleError -f $script:badSystemLocale) `
                    -ArgumentName 'SystemLocale'
                It 'Should throw expected error when MaximumSizeInBytes is less than 64KB' {
                    { Test-TargetResource -LogName 'Application' -MaximumSizeInBytes 0 } | Should -Throw $errorRecord
                }

See:

ComputerManagementDsc/tests/Unit/DSC_SystemLocale.Tests.ps1

Line 113 in 307bcb8

It 'Should throw the expected exception' {

Or can these exceptions not be validated?

I don't know how the tests can be validated. I have a ValidateSet on the parameter so I don't know how to properly test for what will be thrown. I'm open to suggestions if there's a way to do it?

cohdjn

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PlagueHO)

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 529 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you throw a dummy message and check for that? This just ensures that the error is occurring in the place we expect.

e.g.
                    Mock -CommandName New-EventLog -MockWith { throw 'Event Log Error' }
                    { Register-WindowsEventLogSource -LogName 'Application' -SourceName 'PesterTest' } | Should -Throw 'Event Log Error'
And throughout the remaining tests?

Done.

cohdjn

Committing...

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PlagueHO)

PlagueHO

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

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 220 at r6 (raw file):

Previously, cohdjn (David Nelson) wrote…

I don't know how the tests can be validated. I have a ValidateSet on the parameter so I don't know how to properly test for what will be thrown. I'm open to suggestions if there's a way to do it?

Ahhhh - I didn't realize these were being picked up by [Validate] - so ignore this then.

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 529 at r6 (raw file):

Previously, cohdjn (David Nelson) wrote…

Done.

Have the changes been pushed?

cohdjn

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

tests/Unit/DSC_WindowsEventLog.Tests.ps1, line 529 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Have the changes been pushed?

It will be shortly. I ran into a problem after running the build script one last time throwing errors. I'm trying to get that fixed up now.

PlagueHO

I'll merge once the tests pass. Thank you again!

Reviewed 2 of 2 files at r7, 1 of 1 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @cohdjn)

cohdjn · 2021-04-09T09:51:16Z

Awesome! Thanks again buddy!!

PlagueHO

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @cohdjn)

cohdjn added 2 commits February 12, 2021 06:43

Fixes issues #355, #349, #338, #229

a54bc2a

Removed extraneous string data

3371706

PlagueHO self-requested a review February 12, 2021 20:26

PlagueHO added the needs review The pull request needs a code review. label Feb 12, 2021

Merge branch 'master' into master

df965d7

PlagueHO requested changes Feb 22, 2021

View reviewed changes

cohdjn commented Feb 22, 2021

View reviewed changes

Update CHANGELOG.md

056176f

Fixed duplicate entries.

cohdjn commented Feb 22, 2021

View reviewed changes

Split examples into their own respective files.

40d120a

PlagueHO requested changes Feb 24, 2021

View reviewed changes

Removed RegisteredSource parameter

ff39926

cohdjn commented Feb 26, 2021

View reviewed changes

PlagueHO requested changes Apr 4, 2021

View reviewed changes

PlagueHO added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Apr 5, 2021

Code review changes

1e9af13

Fixed leftover from internal test environment.

90049e1

cohdjn commented Apr 6, 2021

View reviewed changes

PlagueHO requested changes Apr 9, 2021

View reviewed changes

cohdjn commented Apr 9, 2021

View reviewed changes

PlagueHO requested changes Apr 9, 2021

View reviewed changes

cohdjn commented Apr 9, 2021

View reviewed changes

cohdjn and others added 2 commits April 9, 2021 02:08

Additional code review changes

db4916d

Merge branch 'master' into master

687d6a4

PlagueHO approved these changes Apr 9, 2021

View reviewed changes

PlagueHO merged commit bb71e2b into dsccommunity:master Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WindowsEventLog: Multiple Updates - Fixes #355, #349, #338, #229 #359

WindowsEventLog: Multiple Updates - Fixes #355, #349, #338, #229 #359

cohdjn commented Feb 12, 2021 •

edited

Loading

cohdjn commented Feb 21, 2021

PlagueHO commented Feb 21, 2021

cohdjn commented Feb 21, 2021

PlagueHO commented Feb 22, 2021

azure-pipelines bot commented Feb 22, 2021

PlagueHO left a comment

cohdjn left a comment

cohdjn left a comment

PlagueHO left a comment

cohdjn left a comment

cohdjn commented Feb 26, 2021

PlagueHO commented Feb 26, 2021

azure-pipelines bot commented Feb 26, 2021

cohdjn left a comment

cohdjn left a comment

cohdjn left a comment

cohdjn commented Mar 3, 2021

PlagueHO commented Mar 3, 2021

PlagueHO left a comment

cohdjn commented Apr 6, 2021

cohdjn left a comment

cohdjn left a comment

cohdjn left a comment

PlagueHO left a comment

cohdjn left a comment

cohdjn left a comment

cohdjn left a comment

cohdjn left a comment

PlagueHO left a comment

cohdjn left a comment

PlagueHO left a comment

cohdjn commented Apr 9, 2021

PlagueHO left a comment

WindowsEventLog: Multiple Updates - Fixes #355, #349, #338, #229 #359

WindowsEventLog: Multiple Updates - Fixes #355, #349, #338, #229 #359

Conversation

cohdjn commented Feb 12, 2021 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

cohdjn commented Feb 21, 2021

PlagueHO commented Feb 21, 2021

cohdjn commented Feb 21, 2021

PlagueHO commented Feb 22, 2021

azure-pipelines bot commented Feb 22, 2021

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn commented Feb 26, 2021

PlagueHO commented Feb 26, 2021

azure-pipelines bot commented Feb 26, 2021

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn commented Mar 3, 2021

PlagueHO commented Mar 3, 2021

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Apr 6, 2021

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Apr 9, 2021

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Feb 12, 2021 •

edited

Loading