Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding 'Ensure' resource property and composite resource #22

Merged
merged 7 commits into from
Jun 18, 2016

Conversation

ebekker
Copy link
Contributor

@ebekker ebekker commented May 11, 2016

  • Ensure resource property makes this resource more consistent with other similar types such as WindowsFeature and adds the ability to exclude certain choco packages from being installed (at least not for too long).
  • Also added a composite resource cChocoPackageInstallerSet which allows you to define a collection of multiple choco packages to install (or uninstall). Again this is in keeping with the conventions of similar resources that offer a bulk item variation such as the new WindowsFeatureSet that was added in WMF 5.0.

Please note:

  • I've only tested this on WMF 5.0 environment
  • I've bumped up the version details to what I thought was logically next and synched up the version info in a few places.

ebekker and others added 7 commits May 11, 2016 10:39
As per [Issue chocolatey#21](chocolatey#21),
adding an `Ensure` resource property to control adding/removing choco
package.
Needed to conform to the structure and format of a composite DSC
resource as defined
[here](https://msdn.microsoft.com/en-us/powershell/dsc/authoringresourcecomposite).
Turns out GitHub is smarter than I thought...
@lawrencegripper
Copy link
Contributor

Thanks for the PR and sorry for the slow response, I've been swamped the last few weeks. Anyone else able to give this a test while I'm blocked? @dlwyatt?

@lawrencegripper lawrencegripper merged commit d98a495 into chocolatey:master Jun 18, 2016
@lawrencegripper
Copy link
Contributor

Looks like the composite schema has broken the build script which runs a quick set of tests against the MOF file.

For the time being I've added a quick change to skip the composite resource here

Is there a better way to handle this?

@ebekker
Copy link
Contributor Author

ebekker commented Jun 18, 2016

Doh! Just looked at the code and realized I needed to commit a .schema.mof file, that's what's breaking here.

@lawrencegripper
Copy link
Contributor

No worries, if you can do a new PR with the mof added and the skip removed from the appveyor script I've just enabled the webhook for the tests to run when a pull request is created. So we can test that out too.

@ebekker
Copy link
Contributor Author

ebekker commented Jun 18, 2016

Actually, scratch that -- that was by design since the InstallerSet resource is implemented as a composite resource, which means it doesn't get its own schema.mof. I'm searching around to see what's the recommended practice for this scenario.

@lawrencegripper
Copy link
Contributor

Yeah when I had a quick read that's what I found too. If the underlying resources are checked then I'm happy to leave the skip in.

Ideally we'd add some more complete testing to the build script at some point, currently just basic schema validation of the resource but I'm happy to leave that for the future.

@ebekker
Copy link
Contributor Author

ebekker commented Jun 18, 2016

I found one example where they explicitly filter out any composite resources from the generated list of resources, so that may be the more correct way to do it, versus a hard-coded exclusion list, but I guess on this scale it's not a terrible violation of programming law :-)

@ebekker
Copy link
Contributor Author

ebekker commented Jun 18, 2016

Same idea here, except this approach is to itemize each resource manually and put the test clauses in that are appropriate for that resource type.

I think your solution should be fine for now, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants