(GH-149): Allow null/empty for Source param of Get-TargetResource

ralish commented 3 years ago

Description

Not doing so will break Get-DscConfiguration when called after applying a configuration with a cChocoPackageInstallerSet resource which does not specify the Source parameter. This is due to the composite resource unconditionally setting the parameter.

Related Issue

Fixes #149

Motivation and Context

Without this fix running Get-DscConfiguration will fail in affected configurations.

How Has This Been Tested?

Internal test environment with an affected configuration.

Screenshots (if appropriate):

N/A

Types of changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[X] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[X] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[X] All new and existing tests passed.

ralish commented 3 years ago

Anything I can do to help push this along?

pauby commented 3 years ago

Hey @ralish. Thanks for the PR. Just getting time to review it but I haven't forgotten about it.

ralish commented 3 years ago

@pauby: Anything I can do to help move this along?

pauby commented 3 years ago

@ralish Apologies for taking so long on this. Let me schedule some time this week to focus on reviewing it.

pauby commented 3 years ago

I've had a look at this (apologies again for the time it's taken). I think we're fixing the wrong thing. I don't think we should be removing the null / empty string validation from cChocoPackagelnstall but amending the cChocoPackageInstallerSet to not pass an empty $source?

I feel the unconditional nature of this you mention in your issue is what the actual problem is.

ralish commented 3 years ago

@pauby Its been a while since I looked into the issue and came up with the proposed fix so my memory is a bit hazy, but from a quick look at the proposed change & current source I think the reason I chose this approach is it's backwards compatible. Using this approach the issue is fixed immediately on updating the cChoco module, but changing the cChocoPackageInstallerSet would mean having to recompile and redeploy all affected DSC configurations.

The Get-TargetResource function only returns a hashtable with configuration information for each installed Choco app, so there shouldn't be any flow-on affects by loosening the parameter validation to match cChocoPackageInstallerSet.

pauby commented 3 years ago

With any release of the module, recompiling and redeploying would be necessary.

Passing an empty source doesn't make sense and so I think we should fix that logic where it's broken.

ralish commented 3 years ago

OK sure. Do you want to make the change or rather I amend the PR?

pauby commented 3 years ago

If you're happy to amend the PR then go for it and I'll pull it in.

ralish commented 3 years ago

@pauby Done. Tested locally on a Windows Server 2012 R2 VM and all seems to work well.

ralish commented 3 years ago

@pauby Just checking if the amended PR looks fine to you? Eager to close this out :)

pauby commented 3 years ago

LGTM! Thanks @ralish. This will be released in the next version.

chocolatey / cChoco