WindowsEventLog: Evaluation of event log differ

[johlju]

The existence of the event log name is evaluated twice in this code, this is redundant and should be only evaluated once. Also, the two evaluation differ in that one uses -like and the other uses -eq, this should probably only be either one, not both.

https://github.com/PowerShell/ComputerManagementDsc/blob/75152fc76595b7c36ce6e18439d7488a2df0c0d0/DSCResources/MSFT_WindowsEventLog/MSFT_WindowsEventLog.psm1#L180-L184

[cohdjn]

Working on this along with a few other issues with this resource.

[PlagueHO]

Cool! That would be awesome @cohdjn . Just an FYI, there is an existing problem with the integration tests that are causing the CI to fail. I'll try and get some time this weekend to finally figure out what is going on.

[cohdjn]

This is turning into a refactor. As I've been going through the module, there's a lot of things in here I really don't understand why/how they were done. This could end up being a breaking change across all the open issues for WIndowsEventLog. Let me know how you'd like to proceed.

[PlagueHO]

Hi @cohdjn,

I feel like I encountered this issue too (the duplicate queries of EventLog), some time back. I believe I also couldn't figure out why it was needed, but it did seem to break integration tests or something like that. So I left it in.

What would the breaking change be? We can take them if they improve the resource - but try to avoid them if possible.

[cohdjn]

So far the only possible breaking change is on Get-TargetResource. The IsEnabled parameter is not required and doesn't really do anything. Additionally, the requirement of having IsEnabled declared $True doesn't make sense which (based on what I've done so far) doesn't feel like a breaking change. At least not yet. I'm still reworking parts of the script along with the tests so I don't fully know just yet.

[johlju]

The $IsEnabled can be removed from Get-TargetResource, agree it does not do anything, and we should avoid having non-mandatory parameters in Get-* if they are not needed for Get-* to work. I would not consider that a breaking change, but rather a bug that should be fixed. 🙂 Before we were afraid that AWS or Puppet used these but as I recall they do not, so should be safe to remove without a breaking change. But I leave it up to @PlagueHO to decide. 🙂

[cohdjn]

Cool @johlju. The worst case scenario is putting it back in and just marking its usage as deprecated but left for backward compatibility. I'm good no matter what @PlagueHO decides.

[PlagueHO]

Sounds good to me. Agree with @johlju. We'll make it a non-breaking change.

[cohdjn]

Done. I'm down to re(writing) tests and cleaning/formatting the code so as soon as @PlagueHO gets CI straighten out I should be ready to issue the PR.

[johlju]

@cohdjn it should be good to go. Rebase with the latest changes in main and the pipeline should work.

[cohdjn]

@johlju @PlagueHO It appears there's still wrong with the pipeline. I just checked the front page and it says it's failing...

[gaelcolas]

I'm on it.

[johlju]

@cohdjn yes, it was just the API key that had expired in the deploy step. The tests pipeline did work. But now it is green all over. 😄