Determine best go-forward puppet-riak module

[mbbroberg]

As you can see from the moving around of repos and updates, I'm making a big effort to get our Puppet strategy to parity with other tools. My honest question for those who care most about this is straightforward: which of the following is the best repo to start from?

• The current one @haf has been kind enough to maintain here
• The one @Cornellio built over here
• This one from @jbussdieker over here
• Or something else?

@basho-labs/puppet-committers - I'll need your input here!

Let's all discuss this with a positive view on what we need to accomplish to make the most useful code for our communities. If you'd like more guidance, the goals are all outlined here: https://github.com/basho-labs/the-riak-community/blob/master/config-mgmt-strategy.md

[danieldreier]

Quick comparison

basho-labs/puppet-riak

• Has rspec-puppet test coverage
• inline docs / code commenting looks good
• not a fan of the hiera() function calls in parameters; this is better handled using automatic parameter substitution
• very much not a fan of how https://github.com/basho-labs/puppet-riak/blob/master/manifests/params.pp pulls down a specific file using httpfile. Poor package distribution should be fixed, not worked around. For distributing patches I can (grudgingly) see a good use case for this httpfile resource, but it should be a different module because it solves a totally unrelated problem to managing riak.
• module uses the Anchor pattern, which dates back to the puppet 2.x days; this should be cleaned up
• no cluster management

Cornellio/puppet-riak

• No test coverage
• inline docs / code commenting looks good
• uses an exec and sed to modify sudoers. This is extremely inelegant and should be handled using augeas or a sudoers module. I wouldn't use it with this in place.
• riak::baseconfig is very opinionated and linux-specific; I wouldn't expect a riak module to go change the mount options on /.
• has very basic ability to use execs to join a cluster, but not to full bootstrap one, as far as I can tell. I wouldn't really be comfortable using this.

jbussdieker/puppet-riak

• very basic rspec-puppet test coverage
• has travis-ci but currently failing and hasn't been fixed in ~3 months. No idea if it's a serious problem or not.
• does not manage repositories
• less flexible configuration parameters than basho-labs/puppet-riak, since it defines each variable rather than iterating over a hash to generate the config. However, it is nice to have all the options listed out that way.
• no way to set riak package version, provider, etc.

I haven't done an incredibly in-depth review, but it looks to me like basho-labs/puppet-riak is the most mature of these modules. I think there are elements of the others to be used as inspiration. I don't intend this as being overly critical of the other modules; they were probably written for specific use cases, and this is looking to fill a very general use case.

The major blind spot is that I haven't evaluated what versions of riak these modules support. The basho-labs/puppet-riak module looks most flexible for that because it doesn't hardcode the config settings.

[mbbroberg]

To keep this thread up-to-date and confirm: we're only concerned with coverage of 2.X going forward @danieldreier. Thank you for this review!!

[Cornellio]

Thank you for the input. For what it's worth, I plan on making improvements to what I've done so far.

[danieldreier]

The next major steps I'd see for this module are:

Reduce barriers to contribution

Adding comprehensive test coverage is critical to facilitating community contribution, because it makes it possible for a less-experienced participant to refactor existing code and have confidence that the changes won't break existing usage.

• [ ] Write beaker tests that can be run locally using virtualbox (I can copy the outline we used in the puppet-puppet module)
• [ ] Audit / improve rspec-puppet test coverage
• [ ] Add travis-ci (or other) CI to run rspec-puppet tests against every pull request, and display a current test status badge in the readme
• [ ] Add a CONTRIBUTING.md file that describes how to run rspec and integration tests locally, links to documentation on how to contribute to test coverage

Cleanup

• [ ] Drop puppet 2.7 support and purge docs references to it
• [ ] Drop Debian 6 support
• [ ] replace httpfile with maestrodev/puppet-wget
• [ ] Refactor anchor pattern with proper dependencies
• [ ] Refactor around the puppet-module-skeleton to normalize the rakefile, testing, etc to common patterns
• [ ] Refactor params.pp; it doesn't make sense to have so many versions of the same conditional

Major improvements

• [ ] Add Riak 2 support
• [ ] Add (beaker-testable) Debian Wheezy support
• [ ] Add (beaker-testable) FreeBSD 10 support
• [ ] Add (beaker-testable) CentOS 7 support (not interesting to me, but I know people will want it)
• [ ] Add (beaker-testable) Ubuntu 14.04 support (not interesting to me, but I know people will want it)

I'm not trying to tell other people what to do, just to create some momentum and a common sense of direction. We can break them out as other issues but I figured it'd be easier to have a discussion with them listed here. Totally open to changing the list.

[danieldreier]

@Cornellio looking back, I feel like I didn't phrase my evaluation of your module in a very friendly way. I apologize for my harsh language; I would have felt bad if somebody had said the same about a module I maintain.

Can you help me understand where the specific optimizations in riak::baseconfig in your module came from? I do like the idea of optionally doing those sorts of optimizations, but I don't want to cargo-cult them into this module without some understanding or docs we can link people to for more understanding.

[haf]

For historical reference, this module was written before automatic parameter resolution worked, when the anchor pattern was 'the way to go' and before Basho had their own repositories for packages. I also don't like to depend on a 'glue' module like wget since wget isn't installed by default on Fedora/RHEL systems and you have to explicitly install it. Curl is better in that regard.

The anchor pattern is needed for all Puppet 3 code, too, so if you clean that up, it won't support (most of) Puppet 3.

Still, it's great that bugs get fixed in puppet and we now don't have these problems anymore. =)

As for the extra module, I have already extracted it https://github.com/haf/puppet-httpfile

[mbbroberg]

I'd like to +1 to @Cornellio -- your work is fantastic. Please note that I brought it up here because of your hard work and I see value in it.

You are more than welcome to continue to edit that code. I also want to invite you to contribute to this repo as it improves thanks to @danieldreier & @haf's teamwork! :+1:

[mbbroberg]

@danieldreier & @haf - I believe we have an idea of what to go forward with. This issue is resolved for me. I love your recommendations too Daniel, so I'll start looking into mapping this stuff out.

[Cornellio]

@danieldreier not a problem, that's how it goes sometimes.

For the optimizations in baseconfig, I got those from the Basho docs on Linux tuning. I wanted this for my own use but can certainly see how it should be optional or broken into a separate module.

I'll look for places to contribute to this repo.

[mbbroberg]

@danieldreier, I'd like to break these out into separate items in Backlog. I started tags based on scrum where we can flesh out work in Backlog, get it to Ready with clear stories (more on Ready definition) and then move it to Done when PR is ready for discussion.

If everyone here would prefer a different strategy, no prob - let's discuss :smile:

[danieldreier]

+1

[danieldreier]

@haf I believe that the switch from anchor to contain happened in puppet 3.4.0.

I totally understand the history here. The reason I'm being relatively aggressive about redoing a lot of this code is that when I helped update puppet-puppet we had a bunch of code dating back as far as the 0.25 puppet versions, and support for a bunch of deprecated features that it turned out nobody was using. Keeping backwards compatibility that it turned out nobody needed took months of work, which was really unfortunate. I'm know how significantly puppet has changed since the 2.7 series, and that the prevailing norms around usage have also shifted significantly. I'm just trying to avoid grandfathering in old approaches unnecessarily, and don't intend it as a judgement on the original code or the work that went into it.

[mbbroberg]

@danieldreier +1 on moving forward aggressively if it helps get us from where we are to something updated with best practices. @haf - glad to talk more on here or offline at mbrender [at] basho!