feat(formula): add metastate per community convention

[noelmcloughlin]

Resolves #410 and helps #352. Using this metastate is optional.

In particular, this metastate avoids having developers reinvent the wheel, i.e. https://github.com/opensds/opensds-installer/blob/c3e8b45a75b3ce1589f0d2ae680e8302ec9c4dd2/salt/srv/salt/install/salt.sls#L1

[javierbertoli]

@noelmcloughlin just out of curiosity, what's the advantage of the syntax you used here over this one? It's the first time I see the one you propose used in a state file.

[noelmcloughlin]

@noelmcloughlin just out of curiosity, what's the advantage of the syntax you used here over this one? It's the first time I see the one you propose used in a state file.

None. I presume the vpn formula works with every OS so it is nice and clean.

The salt-formula has technical debt which results in, at best, state failures, at worst exceptions-

• salt.pkgrepo was broken on Suse, not working on FreeBSD, and not for MacOS.
• same for other state
• salt.formulas fails on MacOS but at least it is only one state failure.

This implementation respects the technical debt, but that does not mean we approve of it.

I can clean it up if you want.

[daks]

I'm surprised too by the syntax, not the Jinja "if" included, but

base:
  '*'

instead of

include:

This syntax is the one generally used for top.sls, I didn't know it could be used by states.
What is the need to have this different syntax? I think you could add the Jinja "if" statements in the more "standard" one.

[myii]

@noelmcloughlin Just a bit busy at the current time. If you could deal with the concerns raised above in the meantime, that would be great.

[noelmcloughlin]

If you could deal with the concerns raised above

Hi @myii, @javierbertoli thanks for reviewing.
@javierbertoli said he was curious not disapproving, but I cleaned up init.sls anyway.

Other PR will need to tidy up FreeBSD/MacOS failed states & exceptions.

[myii]

Actually looking at this, there's just too many ifs and buts at the moment, which is probably why it hasn't already been done. Other than development/testing, I can't think of a situation where you'd want to run all of the states. So perhaps a way around this is to do the core state(s). But even that's difficult, because that would probably be:

• pkgrepo +
• one of the other states (master, minion, ssh or even syndic) +
• formulas (maybe)

Perhaps best to leave this to top.sls matching? Or introduce new variables into the map and then use Jinja if blocks here (but how does this improve upon top.sls?).

[noelmcloughlin]

In the general scheme, the expectation is that each formula should try to include init.sls regardless of the formula name. There are probably a few init.sls in the saltstack-formulas repo which do not make sense either and need to be fixed up. Since this formula has never had an init.sls I think this PR should focus on introducing one - maybe not correct one - but to fix the anomaly.

So the question is what will work even if no states run.

On GNU/Linux I use:

    - salt.minion
    - salt.master
    - salt.ssh
    - salt.formulas

On MacOS I use this (to avoid all the exceptions).

    - salt.formulas

What's acceptable here? Having no init.sls is not really acceptable - salt is software same as any other formula.

[daks]

In the general scheme, the expectation is that each formula should try to include init.sls regardless of the formula name. There are probably a few init.sls in the saltstack-formulas repo which do not make sense either and need to be fixed up. Since this formula has never had an init.sls I think this PR should focus on introducing one - maybe not correct one - but to fix the anomaly.

As said by @myii I'm not sure it makes sense, even if the majority of formulas do it.

At work, we use

- salt.minion

for every server, and only salt.master when declared in our 'services' list.

What's acceptable here? Having no init.sls is not really acceptable - salt is software same as any other formula.

Not sure I agree with that. The README explicits tell which states exists. If you want we can add a mention explicitely stating that there is no salt metastate.

If we really want a metastate, I think it should call salt.minion only because you don't want to install a master on every server.

[noelmcloughlin]

@daks Thx. I updated the commit with a minimal init.sls to satisfy POLA, avoid #410 surprises, and too prevent salt-formula being a special case. I think this PR is reasonable even if I have doubts too.

[getSurreal]

I like the init states that include the other states based on pillar. If salt:master is defined, then include salt.master, etc. So if you manage the master and the minions with salt, you could have the top file call just salt for all of your systems and it would manage the master and minions appropriately.

[noelmcloughlin]

If salt:master is defined, then include salt.master, etc. So if you manage the master and the minions with salt, you could have the top file call just salt for all of your systems and it would manage the master and minions appropriately.

And I like the suggestion because it makes init.sls more useful in this case.
I added WIP to slogan while I work on investigating comments.

[myii]

I like the init states that include the other states based on pillar. If salt:master is defined, then include salt.master, etc. So if you manage the master and the minions with salt, you could have the top file call just salt for all of your systems and it would manage the master and minions appropriately.

@getSurreal This crossed my mind earlier but there are default values for some of these coming through to the map (i.e. we can't do this implicitly). I'm not keen on using pillar.get (or even config.get) inline to skip around the map to try to work things out. So we'd probably have to resort to introducing new explicit values to confirm what should or shouldn't run.

[myii]

If we really want a metastate, I think it should call salt.minion only because you don't want to install a master on every server.

@daks This won't work for those only wanting salt-ssh -- ask @alxwr all about that!

[daks]

If we really want a metastate, I think it should call salt.minion only because you don't want to install a master on every server.

@daks This won't work for those only wanting salt-ssh -- ask @alxwr all about that!

For sure, that why I doubt the need for a metastate :) But installing salt.master in it is clearly not a solution.
This simply a quite-good default. Special needs can always be handled like before.

[getSurreal]

I like the init states that include the other states based on pillar. If salt:master is defined, then include salt.master, etc. So if you manage the master and the minions with salt, you could have the top file call just salt for all of your systems and it would manage the master and minions appropriately.

@getSurreal This crossed my mind earlier but there are default values for some of these coming through to the map (i.e. we can't do this implicitly). I'm not keen on using pillar.get (or even config.get) inline to skip around the map to try to work things out. So we'd probably have to resort to introducing new explicit values to confirm what should or shouldn't run.

@myii Doesn't map only get called from the individual states? Only the init would need to have 'pillar.get'. If someone called 'salt.master' state, it would still load up the defaults and execute even if there was no pillar defined, but if only the 'salt' state got called it would not execute unless 'salt:master' existed in pillar (and similarly for the other states).

[myii]

@getSurreal Looks like I'm going to have to get over my distaste of inline pillar.get calls! Would you be kind enough to suggest the changes in a review of this PR, or in a comment here?

[getSurreal]

Untested, but here's the basic layout. Order can be changed for dependencies. I know salt.formulas is needed before salt.master, but I'm unsure of any other order requirements.

{%- if salt.pillar.get('salt') %}
include:
  {%- if salt.pillar.get('salt:formulas') %}
  - salt.formulas
  {%- endif %}
  {%- if salt.pillar.get('salt:master') %}
  - salt.master
  {%- endif %}
  {%- if salt.pillar.get('salt:api') %}
  - salt.api
  {%- endif %}
  {%- if salt.pillar.get('salt:cloud') %}
  - salt.cloud
  {%- endif %}
  {%- if salt.pillar.get('salt:ssh') %}
  - salt.ssh
  {%- endif %}
  {%- if salt.pillar.get('salt:syndic') %}
  - salt.syndic
  {%- endif %}
  {%- if salt.pillar.get('salt:standalone') %}
  - salt.standalone
  {%- endif %}
  {%- if salt.pillar.get('salt:minion') %}
  - salt.minion
  {%- endif %}
{%- endif %}

[myii]

@getSurreal Thanks for that starting point. I think we've got an issue with some of these, since pillar.example doesn't mention those specific entries, such as:

• salt:api -- nothing in pillar.example but salt_api potentially available in the map.
• salt:formulas -- salt_formulas will work.
• salt:ssh -- there is salt:lookup:salt_ssh and salt:ssh_roster, though.
• salt:standalone -- nothing for this at all, pillar or map.
• salt:syndic -- there is salt:lookup:syndic, though.
• Etc.

Hence, we may not be able to achieve this implicitly and definitely not in a consistent fashion. New explicit values are probably required, at least in some cases.

As you mentioned, the ordering will need to be looked at as well.

[noelmcloughlin]

Thanks @getSurreal for the detailed suggestions. I'm open to incorporating your solution.

A question on implementation.

(1) Do we prefer the conditional logic at top-level salt.init (as @getSurreal illustrated)?
(2) Should conditional logic be moved one level down to each salt.xxxxx state? For example PR #425 allows the following to always work - conditional logic for salt.pkgrepo is delegated.

include:
  - salt.pkgrepo
  - salt.minion

[noelmcloughlin]

@getSurreal @myii @daks
Could you please review this PR now? I think this is best approach right now and can be refined by future PR. Attached is output from archlinux - with/without pillar data: tested_on_archlinux.txt

[getSurreal]

Instead of the "extra_init_states:", I would do something like

      {%- if salt.pillar.get('salt:api') %}
  - salt.api
      {%- endif %}
      {%- if salt.pillar.get('salt:syndic') %}
  - salt.syndic
      {%- endif %}

and add to pillar.example something like

salt:
  api: 
    install (or enable): true
  syndic:
    install (or enable): true

[myii]

Just some initial feedback on the new changes and ideas.

@noelmcloughlin Could we use config.get throughout instead of pillar.get? I'd prefer this to be future-proof, as we continue to roll out our new structure.

@getSurreal Taking the example of salt.syndic, I've got a little nagging concern about this but I can't put my finger on it. The rough take is that I'm concerned about reverse compatibility, where installations are being performed without having to make an entry into the pillar/config, due to the entries already present in the map. Adding salt.syndic to top.sls may be enough. I did configure a syndic in the past but I can't remember if it was using this formula or not. What shouldn't happen is "breaking" existing setups where users have to add salt.syndic to top.sls and make an enabling entry in the pillar/config, where that hasn't been needed before. Something along those lines.

[myii]

salt:
  api: 
    install (or enable): true
  syndic:
    install (or enable): true

Furthermore, these would need to be checked in the state files as well, unless we're using them purely for this meta-state and then would need to be named accordingly. Not that I'm recommending that as what should be done, it doesn't really make sense.

Another angle is that the main meta-state doesn't have to do everything. Covering the main options is probably good enough.

[noelmcloughlin]

@myii
I will change to config.get .... I'm presume it works same as pillar.get

@getSurreal
Historically this formula had no use case for salt:api and salt:syndic dicts .. until now.
But after thinking about this I think your suggestion is promoting a best practice ....

salt:
  api:  { .. dict }    
  syndic: {..dict ..}

If someone introduces new feature for salt.api and salt.syndic states we would obviously prefer them to isolate any required parameters in salt:api and salt:syndic dicts, as best practice.

Guys ...
In summary I would like to adopt both suggestions and let the metastate be controlled by modification of pillars, at least in this PR. There is not a breaking change because init.sls is new SLS in this formula and salt:api and salt:syndic are new (optional) dicts in pillar data.

[noelmcloughlin]

Is this okay?

[myii]

Haven't checked the very latest changes but the yamllint errors need to be resolved:

• https://travis-ci.com/saltstack-formulas/salt-formula/jobs/239347295#L222

[noelmcloughlin]

Yamllint is fixed now.

Suse Travis CI job failure is unrelated to this PR.

STDERR: The command '/bin/sh -c zypper install -y sudo openssh which curl' returned a non-zero code: 106

[noelmcloughlin]

@myii are you happy with this?

[myii]

@noelmcloughlin A couple of inline comments for you. Would you mind rebasing this on the latest version of the formula, so that all of the linters can run?

[noelmcloughlin]

@myii that was done. the commits giving lint problems are not part of my PR.

[myii]

@noelmcloughlin The issue has come via. the merge commits, where the old commit messages are triggering the commitlint violations. That can be avoided via. a clean rebase or using Squash and merge from the GitHub UI, adding an appropriate commit message there. Which do you prefer?

Can I get any final approvals from anyone else involved on this thread? Or I can see that @aboe76 or @sticky-note haven't been involved so far -- any thoughts?

[noelmcloughlin]

The issue has come via. the merge commits, where the old commit messages are triggering the commitlint violations

Okay, I have rebased and tests are passing again. thanks.

[myii]

@noelmcloughlin Thanks for doing that. We'll allow for any final reviews and then get on with merging this soon -- thanks for your patience.

[myii]

@noelmcloughlin Merged (finally!).

[saltstack-formulas-travis]

🎉 This PR is included in version 1.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[vincentor]

Just some initial feedback on the new changes and ideas.

@noelmcloughlin Could we use config.get throughout instead of pillar.get? I'd prefer this to be future-proof, as we continue to roll out our new structure.

@getSurreal Taking the example of salt.syndic, I've got a little nagging concern about this but I can't put my finger on it. The rough take is that I'm concerned about reverse compatibility, where installations are being performed without having to make an entry into the pillar/config, due to the entries already present in the map. Adding salt.syndic to top.sls may be enough. I did configure a syndic in the past but I can't remember if it was using this formula or not. What shouldn't happen is "breaking" existing setups where users have to add salt.syndic to top.sls and make an enabling entry in the pillar/config, where that hasn't been needed before. Something along those lines.

i met a new problem with using config, if use pillar i can override a new value in command line when i use salt-ssh to install minion like this

salt-ssh [email protected] state.apply salt pilla='{"salt":{"minion": {"install": true"}}}'

how can i init a salt.config value in command line option when i use salt-ssh to apply salt formula to install minion?

[vincentor]

Instead of the "extra_init_states:", I would do something like

      {%- if salt.pillar.get('salt:api') %}
  - salt.api
      {%- endif %}
      {%- if salt.pillar.get('salt:syndic') %}
  - salt.syndic
      {%- endif %}

and add to pillar.example something like

salt:
  api: 
    install (or enable): true
  syndic:
    install (or enable): true

remind that salt.pillar.get('salt:api') return a function call object when salt'pillar.get' return the exactly value of pillar salt:api

[baby-gnu]

Hello.

I'm resurrecting the discussion since I'm working on refactoring the formula according to current standards in #529.

Using the top level metastate is good, all the {%- if %} take care to run each sub-component according to the configuration.

But there is an issue when calling salt.master directly like in the kitchen top for windows minions.

I see 3 options:

1. like today, just display a test.show_notification that probably nobody will notice
2. use the 3000 warning state feature that will show at the end of salt '*' state.apply output
3. raise a plain non hard failure

I personally advocate for the third option, it's always better to fail. This breaks only for people running states on non compatible minions and the actual kitchen.yml.

If there are no strong argument against the test.fail_without_changes, I'll continue in that direction.

Regards.