CentOS/RHEL-friendly additions

[snobear]

Hi,
Thanks for the puppet-graphite module. I was looking for a "light" puppet module that gets carbon and graphite installed via pip and running on gunicorn without having to worry about setting up apache or nginx configs. I may set that up later, but this is a nice module to get graphite up and running without any extra hassle.

I made some changes to make this puppet module compatible with Red Hat/CentOS. I added a few class params as well, but left the defaults set to what you had before so it shouldn't be too intrusive. I was getting a warning about SECRET_KEY not being set in settings.py/local_settings.py, so I had to move local_settings.py to a template and add a param for that.

Cheers,
Jason

[snobear]

Looks like I need to get better with testing. I'll fix this up...

[philandstuff]

Hi there, thanks for your pull request!

I'm interested in merging this, but there are some issues I'd like to be fixed up before I do.

The main thing is that I'd like the init.d scripts to match the behaviour of the upstart scripts as closely as possible, in order to reduce the possibility of future maintenance breaking CentOS but not Ubuntu (or vice versa) due to differences in service management, and in order to (attempt to) do the same thing as far as possible on different distros.

There are a number of differences in behaviour between your proposed init.d scripts and our existing upstart scripts:

• they don't specify GRAPHITE_STORAGE_DIR or GRAPHITE_CONF_DIR anywhere (so presumably .wsp files go to some default location, and graphite-web searches for config in a default location?)
• they bind to $ipaddress instead of to 127.0.0.1 in graphite-web
• they specify access logs and error logs
• they specify number of gunicorn workers as 1 instead of 2 (-w2 in graphite-web.conf)
• the DESC is generic "Gunicorn Workers" rather than specific "Graphite realtime graphing engine" or "Carbon cache service for Graphite"

Can you fix up the initd scripts to be consistent with the upstart jobs?

There are a few other things that you've changed unrelated to init.d scripts which I think don't belong in this PR. Can you remove them from this PR?

• you've changed the puppet version to '> 3.3.1' from '> 3.2.0'
• you've removed the TIMEZONE = 'UTC' line from local_settings.py

You're welcome to file them as changes in their own pull requests, particularly if you give reasons for why you want to make these changes.

[snobear]

Hey thanks for reviewing. Sure, those changes all sound appropriate; I'll take care of them and resubmit.

you've changed the puppet version to '> 3.3.1' from '> 3.2.0'
you've removed the TIMEZONE = 'UTC' line from local_settings.py

I'll remove these from the PR. The puppet version was changed so I could run the tests with the puppet client version on my system, which is 3.3.1. Is there a better setting for that when you've got multiple people working on the same module? Hopefully that question makes sense - I'm pretty new to writing GOOD puppet modules with rspec tests, etc... :)

Django was not seeing my system time zone setting for some reason, so I had to set it specifically for my timezone in local_settings.py. I may have removed it completely for the PR. I suppose setting the default to UTC and parameterizing this setting would be the best choice.

[philandstuff]

The puppet version was changed so I could run the tests with the puppet client version on my system, which is 3.3.1.

You could set the PUPPET_VERSION environment variable to your desired ruby version when running the build locally; the ~> 3.2.0 dependency only gets used if the variable is unset.

It raises the question of whether our gemfile should really be pinning to 3.2.x, though that's a discussion for another ticket.

[philandstuff]

(note that PUPPET_VERSION is what travis uses to test against multiple different puppet versions)

[snobear]

Hold off on merging...need to make a few more tweaks and do some testing.

[philandstuff]

Hi there. Thanks for updating your PR. I appreciate the work you've put into it and it's addressed all of my previous concerns.

However, I'm a bit concerned that it now adds 12 parameters to the toplevel class; this is a change in the overall interface of the module and means that this pull request is now doing much more than just making the module CentOS/RHEL-friendly.

ie this PR now does two things:

• adds CentOS/RHEL init.d scripts
• makes the module more configurable

these are two separate concerns which ideally should be discussed in two separate PRs.

The benefit from using smaller, single-purpose pull requests is that there's only one issue to discuss, and when agreement is reached, it can be merged or closed as appropriate. On the other hand in this PR, I agree with (and want to merge) the CentOS/RHEL part of the content but have reservations about adding all of the extra parameters. Thus the more contentious content is blocking the good content from being merged.

(Aside: one reason I have reservations is that changing a module's interface where there are existing users is relatively tricky; furthermore, once you've added a configuration parameter, it's hard to change or remove later, so it's important to think carefully before adding them. Also, if you're adding configuration parameters, I'd appreciate if you added tests too.)

Let me reiterate though that I'm still very much interested in merging the CentOS-related material, and if you were to remove the additional parameters from this and just hardcode the current defaults I'd be happy to merge it.

[snobear]

OK that all makes sense. I'll bring it back to the original PR state with the originally-proposed changes. Thanks for your patience; I'm new to collaborating on pup modules.

[philandstuff]

Oh one other thing -- I get email notifications when you make comments, but not when you make commits. So when you've updated the PR and consider it ready for merge, just leave a comment and I'll get pinged to come review.

Also we created a CONTRIBUTING guide in #12 -- I'd be interested if you find that helpful or not (comments to #12, not here).

[samjsharpe]

Closing as this has been outstanding for 3 months. Please feel free to re-open if you'd still like to get the changes merged in!

[snobear]

No prob, I don't know when I'll be able to get to it, so maybe down the road.