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

Install package via title, not name (#684) #686

Merged
merged 2 commits into from
Mar 14, 2018

Conversation

wyardley
Copy link
Contributor

@wyardley wyardley commented Mar 7, 2018

This fixes an issue with the package resource accepting an array for $title, but not for the name parameter. I'm not sure why it was originally coded this way, but it seems to work with one more minor adjustment to the Arch tests (now that #685 is merged).

It adds support in metadata.json for SLES 11 (only) to test the additional package we expect there. I didn't add support for SLES 12 or openSUSE because I'm not 100% sure about the versioning, and >=12 has systemd, so there are some additional adjustments needed there that should probably be done by someone who can do more testing and who knows the versioning situation (for example, we may not want to just do systemd based on >=12 because of the weird numbering w/ openSUSE).

Additionally, it slightly simplifies the resource requirement for the package(s) and moves them to init.pp

@wyardley wyardley added the bug Something isn't working label Mar 7, 2018
Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bastelfreak
Copy link
Member

good catch @alexjfisher


@wyardley I think the initial idea was that testing is easier. The unit test only matches for the resource name, but ignores the actual different package name

This fixes an issue with the package resource accepting an array for
title, but not for the name parameter. It adds support in metadata.json
for SLES 11 (only) to test the additional package we expect there.
@wyardley
Copy link
Contributor Author

@alexjfisher Updated; not sure if https://github.com/voxpupuli/puppet-rabbitmq/blob/master/manifests/repo/rhel.pp#L8 / https://github.com/voxpupuli/puppet-rabbitmq/blob/master/manifests/repo/apt.pp#L19 can be left alone (or maybe just handle the ordering in a cleaner way?)
I'd rather make the minimum of structural changes here tho.

@alexjfisher
Copy link
Member

@wyardley i think it might be neater and easier to understand as eg
Class[‘rabbitmq::repo::rhel’] -> Class[‘rabbitmq::install’]
I’d probably move it to init.pp too.

@wyardley wyardley merged commit b305696 into voxpupuli:master Mar 14, 2018
@wyardley wyardley deleted the issues_684 branch March 14, 2018 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants