RHEL Support

[croomes]

Hi, this is a revival of @snobear's PR #9 for RHEL5 & 6 (sysvinit) support.

It adds support for systemd, but only triggers on Fedora and Centos/RHEL 7+. Centos 7 is fine but I don't have a 6 server to test on. The init scripts are mostly the same from the previous PR so any issues should be minor and you can assign anything raised to me.

I've tried not to change anything in the Ubuntu configs except for reworking the tests.

[philandstuff]

Thank you for your contribution!

I'm afraid I don't have time to look at this right now but I or one of
my colleagues will try to review this at some point this week.

[philandstuff]

I've had a chance to look through this in more detail now. I think
our biggest concern for merging this is that as we don't use any
RedHat-flavoured OS at the moment, we could make further changes to
the module later on and unknowingly break the RedHat features.

To give us some confidence that we weren't making breaking changes, it
might be useful to have some integration tests such as beaker or
serverspec to ensure that the module is capable of getting the
appropriate daemons running and bound to the appropriate ports. They
don't have to be comprehensive; just checking that carbon-cache
listens on 2003 and graphite-web on 8000 would be enough.

I have some other bits of smaller feedback which I'll detail as
line-by-line comments. But the main blocker is integration tests.

[philandstuff]

could this conditional be moved to params.pp?

[croomes]

yes, good idea.

[philandstuff]

also did you mean to add an empty CONTRIBUTORS file?

[croomes]

Hi, thanks for taking the time to review.

I know you guys don't use Redhat so I've made sure the unit tests were complete (even over-complete). Integration tests are a good idea but it's not something I've done before. If it was already in place it should be simple to add as the tests should be the same across both platforms, and it would just be a matter of defining new targets. I'd be happy to take this on as a separate PR given it's a new feature.

I'll update later today (hopefully) with the fixes above.

[philandstuff]

Yeah, I quite understand if you don't have the time or expertise to add integration tests. Unfortunately I don't think we can merge this without them, because ultimately the unit tests are only testing values on puppet resources, not whether the code actually works.

Sadly I don't have the time or expertise either 😧

[croomes]

Are there already integration tests? If so I should be able to copy them. If not, isn't that a big ask?

[philandstuff]

Our position is that we're not happy supporting a system we don't run and don't understand without an adequate safety net. I quite understand if you're not willing to provide that as it certainly is a big ask.

We don't currently have any integration tests on any of our modules but we have some work in the backlog to write some at some point. If you wait until that gets done (and I can't make any promises on any kind of timescale here) then we could look at merging this PR once we have them.

[croomes]

I've made the fixes. I can really only suggest updating the readme to say Redhat is not officially supported but testing, bug reports and fixes are welcome. Happy if you refer all related tickets to me.

[philandstuff]

@croomes we have an open PR for beaker tests now -- #26.

I tried running the new beaker tests against this PR by merging the two branches together, but something in the way we're dealing with stankevich/python is making them try to install the ubuntu packages rather than the centos ones -- it tries to install python-virtualenv and python-pip, which look like ubuntuish names rather than centosian.

I'm out of time for now today, but I thought I'd let you know how far I've got.

[croomes]

@philandstuff excellent, really happy to hear! I won't be able to help much during the week but will catch up on the weekend and help where I can. In the meantime, those packages are the same on Centos:

# rpm -q python-virtualenv python-pip
python-virtualenv-1.10.1-2.el7.noarch
python-pip-1.3.1-4.el7.noarch

I expect the graphite-web test will fail because of the gunicorn-django deprecation. I've got a branch in my fork that moves to wsgi.

[samjsharpe]

I'm closing, not because I don't think this is a very valuable addition, but because it isn't currently mergeable (after the beaker changes) and I can't rebase and push to croomes:rhel so it's sitting on our dashboard without being actionable ;o)

Please do reopen when you're ready @croomes!

[croomes]

Hi @samjsharpe and @philandstuff - I've rebased but github won't let me re-open the PR, only project admins can. If you re-open I'll re-push, otherwise I'll create a new PR. Let me know if you prefer it rebased into a single commit.

Just got beaker running - very cool. I'll get the Centos failures fixed soon.

[samjsharpe]

Now that I can totally help with ;o)

[samjsharpe]

These tests are really annoying @croomes.

They fail on your branch because we have a dependency on 'stankevich/python', '>= 1.2.0' which has basically a package {'python-pip': } resource. That package doesn't exist in vanilla CentOS, so the install fails in the beaker tests and causes every subsequent resource to fail.

I guess you have EPEL enabled on your real servers (because that's where 'python-pip' usually comes from), which is why it works. I'm also hoping that you are enabling EPEL in puppet with some kind of standard third-party module include.

Which I think means you could add a requirement on the same external module you currently use to Modulefile and then if the system is RedHat/CentOS include the EPEL repo here as a dependency before any of the graphite install stuff happens.

[philandstuff]

Agree with @samjsharpe here except that dependencies required for centos should go in spec_helper_acceptance.rb rather than the Modulefile.

Looks like voxpupuli/puppet-python#115 covers this issue too, so if that can be fixed upstream we won't have to do anything.

[samjsharpe]

I am closing this in favour of #25 as I obviously couldn't push changes to this branch. I'm nearly there - I just need to figure out why the graphite-web service doesn't start on CentOS6 and I think it might be a good idea to also run Beaker tests against CentOS7 as it seems quite different.

[philandstuff]

@samjsharpe you're closing #25 in favour of #25?

[samjsharpe]

this is why i should leave early on Fridays: smile:

I of course meant i am closing in favour of #30 which i am actively working on and xontaina all these commits.