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

BREAKING: manage_repos is now repos_ensure (default false), version is now unused, switch to RabbitMQ's "packagecloud" repos #493

Merged
merged 2 commits into from
Aug 25, 2017

Conversation

wyardley
Copy link
Contributor

@wyardley wyardley commented Aug 10, 2016

Note: This is a pretty big changeset. I will get it passing automated tests, but I'd love code review, and would love for folks to do some more manual testing.

This is some basic cleanup, and to drop support for installing brute force based on hard-coded version (i.e., only support using the official RabbitMQ packages via the packagecloud repo (for RHEL / CentOS), and continue supporting the Debian repo directly on the rabbitmq site).

I'm not sure if having

class rabbitmq::repo::apt(
  $key_source   = 'https://www.rabbitmq.com/rabbitmq-release-signing-key.asc',
) {
  apt::source { 'rabbitmq':
[...]
    key_source   => $key_source,

(in repo/apt.pp, and the equivalent in repo/rhel.pp) is better than just using $package_gpg_key from the main scope; is there any reason not to do it that way?

Should probably do a version bump for this, but thus far have not touched the CHANGELOG or the version in metdata.json. If this gets merged in, maybe someone in the modules team could do that?

SLES support is untested, and may be broken now that we're not directly grabbing RPM for $version from the Rabbitmq site. Would appreciate suggestions / ideas about how to properly support this using the

Dropped Archlinux support.
Left in the OpenBSD stuff, which I believe should work without explicit version, but have not tested.

@wyardley
Copy link
Contributor Author

Missed some of the unit tests locally, will work on those as well.

@wyardley
Copy link
Contributor Author

Hopefully SLES support would still work (just not in the case of $repos_ensure), since there is an available package: https://software.opensuse.org/package/rabbitmq-server

@wyardley
Copy link
Contributor Author

wyardley commented Aug 16, 2016

@hunner @EmilienM @DavidS Spoke with this a bit with some folks in #forge-modules on puppet community slack about this, and commented in #440, but would love to get some feedback about this if you've got a moment.

@hunner hunner added this to the 6.0.0 milestone Sep 1, 2016
@hunner
Copy link
Member

hunner commented Sep 1, 2016

Looks like there is a conflict. Sounds like a 6.0.0 would be timely, so I'll give this a review and plan on merging soon :)

README.md Outdated
@@ -323,7 +306,7 @@ Boolean, set to true to log LDAP auth.

####`manage_repos`

Boolean, whether or not to manage package repositories.
This option is deprecated in favor of repos_ensure, and is no longer honored.
Copy link
Member

Choose a reason for hiding this comment

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

I think for deprecated parameters we can just remove the docs and comment in the code that it is deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I think @DavidS had asked me to put warnings in the code; do you want those still left in (if you try to call the deprecated parameters)?

@wyardley
Copy link
Contributor Author

wyardley commented Sep 1, 2016

@hunner: Thanks! I tried to leave it better than I found it, but there's still room for improvement, so let me know if there's stuff I can do to make this follow best practices better. I think the README now addresses your comments, let me know what you think about the gpg_key variable though.

Would love it if you have time to review the changes proposed in https://tickets.puppetlabs.com/browse/MODULES-3679 as well, and suggest whether or not using composite namevars will be feasible / practical along with prefetching. Created a PR (just to have it to reference; it's still not yet in working state or passing tests) in #504

@wyardley wyardley force-pushed the feature-repos-and-version-fixes branch 2 times, most recently from ef4e517 to 5c620e3 Compare October 3, 2016 17:37
@wyardley
Copy link
Contributor Author

wyardley commented Oct 3, 2016

Rebased against master and squashed my earlier commits into 1.

@bastelfreak is going to suggest some changes (in a separate PR) that will restore Archlinux support. It sounds like this is low effort, as Rabbitmq is available via its package manager already.

@fatmcgav
Copy link
Contributor

@hunner / @wyardley Is this any closer to being landed? Would be good to get support for the official RabbitMQ repos into EL :)

@wyardley
Copy link
Contributor Author

@fatmcgav: not sure, and haven't had much time to devote to working on fixes for this module lately. I'll give a stab to updating this PR against recent master, but @hunner is probably in a better position to speak about the plans for this module.

@wyardley wyardley force-pushed the feature-repos-and-version-fixes branch 3 times, most recently from 0df9f2e to 310895e Compare August 16, 2017 06:07
@wyardley
Copy link
Contributor Author

wyardley commented Aug 16, 2017

Ok, took a stab at getting this in at least semi-presentable shape, even though my memory is a little fuzzy on some of the details. It's now passing spec and acceptance tests (though there is no acceptance test for the repos_ensure => true case currently) - I did test manually and it seems to work.

However, taking out rabbitmq_version in params might break the logic in the commit below, I will try to work around it, but I think it could cause some weird issues unless a dependency is forced (rabbitmq::install is ordered before ::rabbitmq::config, so maybe there won't be an issue?)

ea2303f

@wyardley wyardley mentioned this pull request Aug 17, 2017
@wyardley wyardley force-pushed the feature-repos-and-version-fixes branch 8 times, most recently from 5658769 to bca39ef Compare August 18, 2017 05:43
@wyardley wyardley force-pushed the feature-repos-and-version-fixes branch 4 times, most recently from 246d2ae to 36ca029 Compare August 24, 2017 20:26
@wyardley wyardley force-pushed the feature-repos-and-version-fixes branch from 36ca029 to df2e4fe Compare August 24, 2017 20:36
William Yardley and others added 2 commits August 24, 2017 14:08
…5 support. Remove tests for ancient RMQ version. Rework how $package_gpg_key is used. Remove some checks that tie to deprecated params. Default $repos_ensure to false. Switch to integers for ports and other numeric values
@wyardley wyardley force-pushed the feature-repos-and-version-fixes branch from df2e4fe to 6724952 Compare August 24, 2017 21:08
@wyardley
Copy link
Contributor Author

One difference is that now that repos aren't managed by default, the rmq version on Ubuntu (at least the Ubuntu 14 used in spec tests) will be lower, I think this is why one test I had that was testing if the clustering component is listening on port 25672 fails now with these changes. However, everything else works, and it's listening on 55672, so I just commented out the tests with xit for now.

@hunner hunner merged commit 6724952 into voxpupuli:master Aug 25, 2017
hunner added a commit that referenced this pull request Aug 25, 2017
@wyardley wyardley changed the title repos and version fixes BREAKING: manage_repos is now repos_ensure and defaults to false, version is now unused / deprecated, switch to RabbitMQ's "packagecloud" repos Sep 5, 2017
@wyardley wyardley changed the title BREAKING: manage_repos is now repos_ensure and defaults to false, version is now unused / deprecated, switch to RabbitMQ's "packagecloud" repos BREAKING: manage_repos is now repos_ensure (default false), version is now unused, switch to RabbitMQ's "packagecloud" repos Sep 5, 2017
@wyardley wyardley added the enhancement New feature or request label Sep 5, 2017
@wyardley wyardley deleted the feature-repos-and-version-fixes branch September 6, 2017 04:22
Slm0n87 pushed a commit to Slm0n87/puppet-rabbitmq that referenced this pull request Mar 7, 2019
cegeka-jenkins pushed a commit to cegeka/puppet-rabbitmq that referenced this pull request Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants