Closed ticosax closed 7 years ago
Is this really how we want to handle this? In what situation would you not want it installed, turned on, and configured? Also I believe we're missing the pillar definition for these items?
@ticosax: just put haproxy.service
or haproxy.config
inside top.sls
in states
configuration (instead of whole haproxy
). You don't need to include this like that.
I really don't like the way the init is structured both with this change, and prior to this change (seriously, an init that includes your other states, stupid, just drop the states in your top file). Is anyone actually using their init like this? Seems like a way for people to be lazy and just include a single file instead of the other 3.
But to agree with @gravyboat the two uses should be
include:
- formula.haproxy
or
inlcude:
- formula.haproxy.state.u.want.1
- formula.haproxy.state.u.want.2
@puneetk We should probably talk to Whiteinge about updating that. That sort of init is really bad for understanding how things go together. I'm going to create an issue over there. Thanks for pointing that out.
Not sure if this is still a hot topic, but when I created (or, made the existing formula usable by replacing everything) this, it was more of a 'convention over configuration' kind of thought. The reasoning behind init.sls having three components and nothing else basically stems from the idea that by default it makes sense that you want HAProxy installed, configured and started, and only in edge cases you'd want to only use components. In stead of having no init and ruining one's expectation of simply including a formula and getting on with your life, the init basically runs the default configuration. Now, for HAProxy, this makes sense, but I imagine there are a lot of formulas where there is no good 'default' configuration. Ceph and Hadoop for example... but HAProxy isn't nearly as complex as those two.
I could have dumped everything into one SLS and then there would be no option to only select something else while providing it's dependency yourself, for example, a custom HAProxy configuration where all you want is the option to configure it. You'd have to create s HAProxy-config formula to solve that, or someone would end up putting in a PR to split it off and in no-time we'd be ending up right where we are now. The way I see it, this init.sls structure is like a default constructor with no arguments, and it's the most used and preferred way (at least for this formula).
While I like the idea of formulas being libraries, it's pretty painful to have a crapload of sls files in your file_roots just to get things to work using a formula (which is supposed to make life easier, not 'just as hard but in a nice package'), while in most cases all you want is a line in your top file that says 'this host is going to run service xyz'. And there are a ton of users that use formulas just like this. As of recent, most projects I had converted from something else (puppet etc) to SaltStack, almost everything that wasn't already supplied by a formula was repackaged into an application-specific formula. Our file_roots are only used for overrides and patches, everything else is 'as a formula'.
Since it's also the template, I would think it's not a problem in this case, but I imagine the template as-is will never be suitable for everything else.
Oh, and regarding this specific pillar-method: no. AFAIK this is not what a pillar was ever intended for, and since you'll have to edit a top.sls file to get it in anyway, only adding the stuff you need will cost you 1 extra line at most. If you want 1 state you use 1 line, if you want all states you use 1 line, and only if you want to use 2 out of 3 states you'll be using 2 lines. In all cases: less lines than when using the pillar method. So in terms of ease, single point of configuration or efficiency there is no case here.
Merging this would lead to an error if haproxy.service is called standalone, because of this:
https://github.com/hoonetorg/haproxy-formula/blob/develop/haproxy/service.sls#L8-9
The pillar which would lead to the fail would look like this:
haproxy:
include:
- haproxy.service
global:
enable: True
also calling haproxy.service only in top.sls is failing the same way currently:
Comment: The following requisites were not found:
require:
pkg: haproxy
you would need something like this:
# -*- coding: utf-8 -*-
# vim: ft=sls
include:
- template.install
- template.config
- template.service
extend:
template_config__conffile:
file:
- require:
- pkg: template_install__pkg
template_service__service:
service:
- watch:
- file: template_config__conffile
- require:
- pkg: template_install__pkg
Cross-sls requirements always prevent that states run successfully standalone
load only sls we are interested in. By listing them in pillars.
Still defaults on same list for backward compatibility.