Closed carlcrott closed 9 years ago
@carlcrott thanks for the fixes! This formula is in need of a few improvements to align more with the formulas guidelines doc. Namely to have a few platform-specific data points abstracted out. I commented on two such changes above.
I realize that is beyond the intended scope of your pull req. Do you feel comfortable making those edits? No worries either way. :)
Changes made:
1) Filepaths abstracted into settings.sls w jinja mapping 2) Verified that "supervisord.conf" is used, by default, as a configuration
3) The "/etc/init.d/supervisor" is used as an init script
I believe ... Ideally, the supervisor configs should be in pillar.example? ... but for the time being I think this gets us working.
finally let me know if you want me to rebase these all into a single commit.
@carlcrott great changes. Thanks for doing that. Two minor nit-picks above then this is gold.
@carlcrott Thank you for all the work - I just tried out what would be the merged result and apparently the changes break Redhat service start with supervisor:
Error: No config file found at default paths (/usr/etc/supervisord.conf, /usr/supervisord.conf, supervisord.conf, etc/supervisord.conf, /etc/supervisord.conf); use the -c option to specify a config file at a different path
I didn't do too much in-depth investigation but it looks to me that the upstart problem needs a different solution.
@sroegner thats rather weird as
/etc/supervisord.conf
is listed in that output ... and matches the current patched default path in settings.sls:
{%- set supervisor_conf = gc.get('supervisor_conf', pc.get('supervisor_conf', '/etc/supervisord.conf')) %}
Thoughts?
Appreciate the merge but:
1) Static IDs, using the -name property to name / place the actual files -- that way we're not generating the IDs 2) Configured MySQL-python ( RedHat ) and python-mysqldb ( Debian / Ubuntu ) -- this breaks supervisor in the current master 3) Contents of supervisor_conf -- the current repoed version fails to instantiate supervisor on ubuntu 4) Removal of the custom role: monitor_master grain + convert to use simple node naming in top.sls
On the upside I'm digging this: https://github.com/saltstack-formulas/graphite-formula/blob/master/graphite/settings.sls#L26
I'm also curious -- are there plans for a continuous integration server?
I just wanted to go forward with this and hence started the merge - i made some changes until this worked on Redhat for me. I don't completely understand what the bullet points all mean but had assumed that you'd sent the PR to get things working for you/Ubuntu. In any case I'd prefer to do any of the things in the list via individual GH issues. In my mind the biggest problem right now is the supervisor part which we shouldn't have at all - there is a supervisor-formula out there (just not yet in the saltstack org) which we should look into using. And no - I don't plan to use CI because it would only work with a lot of effort to support the different platforms - i just don't have the time to do this.
I agree w supervisor, however I'll defer to you on whats sufficiently nuclear as to be included in a formula. We've also got Diamond and Django being installed -- unsure on diamond, but https://github.com/saltstack-formulas/django-formula
And Noted: Ill break out into individual Github issues for the rest of these pull requests
Maybe we can try refactoring this per component as we go - i've had good success with formulas depending on each other in the past but we'd have to see how well this works with the somewhat quirky graphite configuration.
Not sure if you guys are having the same issues that I am with pip instancing supervisor ...
details outlined here: http://cuppster.com/2011/05/18/using-supervisor-with-upstart/
TLDR: supervisorctl picks up /etc/init/supervisord.conf and attempts to use it as a supervisor configuration file instead of an init script.