Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to load sls explicitely #28

Closed
wants to merge 1 commit into from
Closed

Allow to load sls explicitely #28

wants to merge 1 commit into from

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Oct 7, 2015

load only sls we are interested in. By listing them in pillars.

Still defaults on same list for backward compatibility.

@gravyboat
Copy link
Contributor

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?

@morsik
Copy link
Contributor

morsik commented Nov 20, 2015

@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.

@gravyboat
Copy link
Contributor

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.

@puneetk
Copy link
Contributor

puneetk commented Nov 24, 2015

@puneetk
Copy link
Contributor

puneetk commented Nov 24, 2015

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

@gravyboat
Copy link
Contributor

@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.

@johnkeates
Copy link
Contributor

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.

@johnkeates
Copy link
Contributor

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.

@hoonetorg
Copy link
Contributor

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

@johnkeates johnkeates closed this Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants