WIP: feat(unsupported): prevent formula running on unsupported minions

[myii]

• https://freenode.logbot.info/saltstack-formulas/20190421#c2129159
• Use failhard to prevent execution if unsupported has been set
• Show reason(s) why minion is unsupported

---

Notes

1. These changes actually work -- only selected draft PR to get the discussion going first.
2. Tests pass with these changes: https://travis-ci.org/myii/template-formula/builds/522932447.

---

Rationale

Had this idea when discussing this stunnel-formula PR, where it appears that Debian Jessie cannot be supported. When looking at the .yaml files, it became apparent that the formula was only configured for Debian and FreeBSD. How would someone know if it was supported for their minions? Rather than relying solely on documentation, if the formula would refuse to run for unsupported minions, with reasons why it isn't supported, that would be advantageous. This draft PR is a suggested approach, to open up the discussion.

---

Use of test.fail_without_changes

This doesn't have to use test.fail_without_changes. If there is a better option, that can be used instead. I used it since it was readily available for the task.

---

Output

An example of the output:

----------
          ID: template-unsupported-test-fail
    Function: test.fail_without_changes
        Name: 

#######################################
# Unsupported minion for this formula #
#######################################
* osfinger: Ubuntu 14.04 (does not use `systemd`).
* osfamily: Debian (just testing).
* os: Ubuntu (just testing).

      Result: False
     Comment: Failure!

• This is actually beyond what would be achieved with this PR, which would only result in the osfinger explanation being displayed.
• But if the other entries for unsupported were to be set in osfamilymap and osmap, then it would show all of the reasons that have been provided.
• This could be extended to check for other important features such as the Python and/or Salt versions.
• Naturally, no other states are run after this failure, due to failhard: True.

---

Leading to: unsupported set explicitly

All of the .yaml files could contain a standardised list (within reason). Then unsupported could be explicitly set wherever necessary. Provides protection from rogue execution of formulas as well as serving as documentation about which platforms are (not) supported.

---

Leading to: improved CI

With this, can always keep all platforms in the testing matrix, such as centos-6, which is currently commented out for this formula. Tests can be included to ensure that the formula doesn't run where it's not supposed to.

---

Final comment

This PR probably isn't quite right. I'm not expecting it to be merged as-is -- or maybe not at all. But it serves as a useful mechanism to stimulate the discussion about how this feature could be implemented instead. I believe that is what is important here.

[aboe76]

@myii, nice Idea. I like it because it makes it clear which parts of the formula could use work, or explain why the formula won't work with a certain OS

[myii]

@aboe76 Thanks for the review. One thought I'm having is whether to use a string instead of a dict for unsupported. Because with a dict, you get it merging to show all of the reasons. But with a string instead of a dict, we can do more powerful things, such as enable and disable the formula running at different levels, by using unsupported: ''. For example:

• Disable the whole os_family but then only enable the specific osfinger entries.
• Enable the whole os_family but disable specific os or osfinger entries.

Basically, is it more important to see all of the reasons why the formula won't run for a particular minion or is it more important to be able to enable/disable in a more powerful way? I guess there is also the option of doing both but that's a lot more maintenance.

[myii]

Tried writing some test for unsupported but there doesn't seem to be any available option to test for expected failures, as is working in this centos-6 test run:

       local:
       ----------
                 ID: template-unsupported-test-fail
           Function: test.fail_without_changes
               Name: 
       
       #######################################
       # Unsupported minion for this formula #
       #######################################
       * osfinger: CentOS 6 (does not use `systemd`).
       
             Result: False
            Comment: Failure!
            Started: 09:46:44.150860
           Duration: 14.228 ms
            Changes:   
       
       Summary for local
       ------------
       Succeeded: 0
       Failed:    1
       ------------
       Total states run:     1
       Total run time:  14.228 ms
>>>>>> ------Exception-------
>>>>>> Class: Kitchen::ActionFailed
>>>>>> Message: 1 actions failed.
>>>>>>     Converge failed on instance <v2017-7-py2-bootstrap-centos-6>.  Please see .kitchen/logs/v2017-7-py2-bootstrap-centos-6.log for more details
>>>>>> ----------------------
>>>>>> Please see .kitchen/logs/kitchen.log for more details
>>>>>> Also try running `kitchen diagnose --all` for configuration
The command "bundle exec kitchen verify ${INSTANCE}" exited with 20.

• It's working the way intended, i.e. failing to run the rest of the states, as required.

There is an open issue on the test-kitchen repo which has even been Accepted at some point, but still isn't resolved.

Is there any sensible way to test this without having to write another state for the test run?

[aboe76]

@myii I would opt for the string solution, making it more powerfull to enable/disable certain OSes and have a comment line next to the unsupported string for the reason, that way it's easy to see why and still have more control.

I don't know enough about test-kitchen to help you with the issue you mentioned.

[myii]

@aboe76 OK, glad we're in agreement. When I get a chance, I'll rebase it to use a strings instead.

[myii]

@aboe76 Done and tested. Now unsupported can be set at any level (in the .yaml files), so we can configure this to match the reality, whether setting supported or unsupported by default for an os_family. So I've also updated some of the entries in each of the .yaml files to represent the situation accurately.

[vutny]

Hi @myii !

I absolutely love an idea of having defined list of platforms which are explicitly supported.

Regarding your implementation, I think including the unsupported.sls in each other SLS file is not the best option.
Because some formulas (at least for PosgreSQL I know for sure) has SLS file completely dedicated to some operating system, say Mac OS. But other files does not support the system which a formula could support in general.
I would like to suggest making a check for compatibility (put the include) only in the top init.sls file which serve as an entry point for full provisioning for a piece of software.
This way we could avoid lots of unnecessary duplications and also enable reuse of individual states.
For example, the user would be able hack Pillar configuration to make some parts of the formula actually usable on certain OS, and craft his/her own top file to reference needed states.

[myii]

@vutny Beautiful, that's what I needed. I did look at init.sls (in each component at first) but I didn't see an import of map.jinja, so I dismissed it -- in my hurry to try out the idea as a whole. But as you've mentioned, that's a much simpler implementation and people are hardly going to be running template.package.install by itself (and the similar states). While the template.package.clean and other .clean states are shown in the README, there's no harm running those from an unsupported minion.

So reading what you've written, I believe this may be a little compromise, since I'm speaking about using this in all all of the init.sls files throughout the formula (not just the top-level one), since they are accessible as meta-states mentioned in the README.

[myii]

@aboe76 @vutny Done, now the include check is done from the init.sls files instead. This way it is triggered from each of the meta-states mentioned in the README (which exclude all of the specific .clean states). Thanks again to you both for the recommendations.

[myii]

I've removed the draft status from this PR because it is meeting general approval and it's time for the CI tests to run from here. I've switched to using WIP: instead, so that it can still be left for discussion from the other reviewers, though.

[daks]

Nice job.

As always, we need to confront pros and cons of this way of doing things:

• pros:
  • programatically detect an unsupported OS (not completely because Kitchen doesn't support it)
• cons:
  • adds more 'complexity' and reduce lisibility: more code, more files. Raise the (mental) work to get into a formula and its behaviour

Also I wonder if finally what people'll do is grep for all unsupported config to see which are not supported. Is there really a big difference to only look at those supported in kitchen.yml?

I'm not against the idea but I'm not sure it's worth implementing it everywhere.

[myii]

Thanks for the feedback, @daks.

• cons:
  • adds more 'complexity' and reduce lisibility: more code, more files. Raise the (mental) work to get into a formula and its behaviour

Actually, the plan is to reduce the cognitive load overall. Very simple to add a line to an os_family, os, etc. to tag it as unsupported. This saves users from running any states of the formula on an unsupported minion, they don't have to read the docs first. Docs aren't always updated anyway, as we already know. Furthermore, this could lead to easily providing another info state, that can automatically displays "this part" of the documentation.

Also I wonder if finally what people'll do is grep for all unsupported config to see which are not supported. Is there really a big difference to only look at those supported in kitchen.yml?

Unfortunately, we're unlikely to ever get to a stage where our Kitchen/Travis setup will cover all of the types of minions that a formula supports. So it's not enough to direct users to that.

I'm not against the idea but I'm not sure it's worth implementing it everywhere.

If this is done properly, it could be well worth implementing everywhere. However, let's get it working in one or two places first, to see if it really makes a significant difference.

[javierbertoli]

Sorry for jumping late here, adding a few comments:

• adding it in the init file implies we're considering a single 'entry point' to the formula, which is not the documented way to do things. Even though it's the most common case, it's not the only way to do things, and you can use a single state file in a formula, so we should include it in all the states, if possible. Am I right?

• how about turning it into a lib (like tofs), which can be called at the beginning of a state, so individual states can be 'supported/unsupported' if needed? Case: A formula has an installed.sls, config.sls but the latter just works for a set of os (in a sense, this happens with bind-formula, ie). Disabling the whole formula is not a good thing in this case, but disabling the config.sls state for unsupported oses would be nice.

we can perhaps set a dict like:

unsupported:
  <grain>:
    <grain_value>:
      <state>: reason
  default:
    <grain>:
      <grain_value>: reason

example

unsupported:
  osfamily:
    Debian:
        config: didn't have time to write a config state for this osfamily
    RedHat:
      install: does not support package install for this app
  default:
    osfinger:
      Debian-8: does not include the package

and in the states do a check_support(sls) at the beginning?

In the example, the whole formula will be unsupported for Debian-8, the config state for the Debian family and install for the RedHat family
Does it make sense?

[n-rodriguez]

Does it make sense?

I think so. My 2 cents.

[daks]

Also I wonder if finally what people'll do is grep for all unsupported config to see which are not supported. Is there really a big difference to only look at those supported in kitchen.yml?

Unfortunately, we're unlikely to ever get to a stage where our Kitchen/Travis setup will cover all of the types of minions that a formula supports. So it's not enough to direct users to that.

We're also unlikely to maintain this 'unsupported'-OS list...

I'm not against the idea but I'm not sure it's worth implementing it everywhere.

If this is done properly, it could be well worth implementing everywhere. However, let's get it working in one or two places first, to see if it really makes a significant difference.

Let's do that then, and see if it's maintained in the future (months, years...).

[noelmcloughlin]

I'm not against the idea but I'm not sure it's worth implementing it everywhere.

If this is done properly, it could be well worth implementing everywhere. However, let's get it working in one or two places first, to see if it really makes a significant difference.

Let's do that then, and see if it's maintained in the future (months, years...).

Okay I like the idea. The consensus is to roll out slowly. We could select a small subset of formulas to implement and showcase features where a universal solution/answer/consensus is unclear. I'm in agreement on these points.

[noelmcloughlin]

Please fix th conflict.

[myii]

@noelmcloughlin Thanks for the review. This PR stagnated a bit since it wasn't conclusive which method would serve us best. I'll rebase when we reach the point of merging this PR.

[noelmcloughlin]

Thanks no bother.

Just an off topic comment - on the challenge of deciding how to handle "go-slow" features.

One of the motivations of the template-formula is to provide a "reference solution" or go-to repo for beginners, and for experienced salters who want to follow current conventions evolved by the community. This motivation suggests the template-formula should have some degree of stability - a stable branch I guess, or master.

I was thinking we could fork template-formula to a incubator-formula so features we are not sure about - or want to soak test - stay there before being promoted to template-formula (stable entity) or being rejected and PR\ed out. We could direct beginners to template-formula and redirect more experienced community members to the incubator formula (template-formula + other stuff).

[myii]

@noelmcloughlin I'm not really a fan of having lots of template-style repos out there. Keeping those synchronised (for the common bits) will be painful. I'd much rather we do most of these things here, in separate branches.

For example, after the little sub-discussion that culminated in #21 (comment), @javierbertoli has provided develop "pre-salted" images that will help us test against develop features. So having a develop branch here is a natural step.

Another suggestion is having small, medium, large and full options for this template-formula. For example:

1. s: install only
2. m: install and configure
3. l: install, configure and manage the service
4. f: all the extras as well (i.e. the master branch here)

This will be easier to maintain as separate branches in one repo. As @javierbertoli mentioned, we could easily use extend to reduce the changeset across the branches.

Where there is a kind of template that varies significantly from this common core, then that will require a separate repo for sure.

[myii]

@noelmcloughlin After all of that, I didn't mention where the "go-slow" would go! The develop branch would be perfect for that, since that would be merged back to master at the appropriate times.

[noelmcloughlin]

@myii thanks for the info, appreciated. I will look into branching further.