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

add ability to have array as package name #656

Merged
merged 2 commits into from
Dec 3, 2017

Conversation

tampakrap
Copy link
Contributor

in case the package has subpackages (like it does for the SUSE family
OSes)

in case the package has subpackages (like it does for the SUSE family
OSes)
@wyardley
Copy link
Contributor

@tampakrap this makes some sense to me, will ask @bastelfreak to review as well.
Would you be able / willing to add / update SUSE support (update metadata, update params.pp) in another PR?

How are the packages broken up -- could you give an example of the packages that would make this work on SUSE, and does it vary by version?

@tampakrap
Copy link
Contributor Author

Hello @wyardley thanks for the review.

Regarding packaging, there is a rabbitmq-server package with a -plugins subpackage, see https://build.opensuse.org/package/view_file/network:messaging:amqp/rabbitmq-server/rabbitmq-server.spec?expand=1

Regarding support, there is already support in params.pp and actually I use this module in production for quite some time now. I just noticed that SUSE is missing from metadata.json, I'd be happy to add it there as well in a separate PR

@wyardley
Copy link
Contributor

I just noticed that SUSE is missing from metadata.json, I'd be happy to add it there as well in a separate PR

@tampakrap Yes, there's a section in params already. The module default is now to use vendor packages, so assuming we merge this to add support for multiple packages, I was suggesting that params should then be updated to install the correct packages (especially if the packages are broken up differently in different versions). Acceptance tests would be good too, though it doesn't look like we support SUSE acceptance tests currently (though you could propose the addition of some in https://github.com/voxpupuli/modulesync_config/tree/master/moduleroot/spec/acceptance/nodesets and https://github.com/voxpupuli/modulesync_config/tree/master/moduleroot/spec/acceptance/nodesets/docker)

Also, this PR should probably include a spec test for the multiple packages use case.

@wyardley wyardley requested a review from bastelfreak October 23, 2017 17:53
@bastelfreak
Copy link
Member

Hi @tampakrap, thanks for this PR.

For any upcoming PRs: The goal is that the moduel works out of the box on a supported operating system. So it would be awesome if you could provide a PR with all data that you pass into the module to make it work on Suse. Would be nice to embed it into the params.pp


I don't think that a unit test is needed for this change

@bastelfreak bastelfreak added the enhancement New feature or request label Oct 24, 2017
@wyardley
Copy link
Contributor

@tampakrap Can you update https://github.com/voxpupuli/puppet-rabbitmq/blob/master/manifests/init.pp#L141 ? puppet strings will update the type, but should probably clarify that this can be a package name or a list of package names, especially since the variable is singular.

@tampakrap
Copy link
Contributor Author

I added the subpackage in params.pp as @bastelfreak said which makes sens. @wyardley give me a hint on how this line should look like please and I'll do it as well

@wyardley
Copy link
Contributor

wyardley commented Dec 2, 2017

@tampakrap sorry for the delayed response. Strings will take care of showing the allowed types, so it would just be to something like this:
# @param package_name Name(s) of the package(s) to install

this module can't do much without them either way
@tampakrap
Copy link
Contributor Author

thanks, I updated it

@wyardley wyardley merged commit a783fc4 into voxpupuli:master Dec 3, 2017
@wyardley
Copy link
Contributor

wyardley commented Dec 3, 2017

Merged, thanks for the contribution.

cegeka-jenkins pushed a commit to cegeka/puppet-rabbitmq that referenced this pull request Mar 26, 2021
…voxpupuli#656)

* add ability to have array as package name

* install by default the -plugins subpackage on SUSE osfamily

this module can't do much without them either way
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants