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

Establish template-formula as the go-to resource for all formula development #21

Open
38 of 71 tasks
myii opened this issue Feb 11, 2019 · 17 comments
Open
38 of 71 tasks

Comments

@myii
Copy link
Member

myii commented Feb 11, 2019

This issue is a collection of ideas initially taken from the log of discussions[1][2][3][4] captured in the wiki.

Visibility

  • Make this template-formula a sticky formula for the whole SaltStack Formulas project.
    • Probably best to wait until most of these changes have been made, to prevent premature use of the template.

Release tagging (for each merged PR)

YAML & Jinja

TOFS pattern vs. pillar

Other pillar-avoidance methods to consider

  • Consider changing all uses of pillar.get to config.get globally (as in, for all formulas eventually).
  • Work through discussions captured from Slack where alternatives are discussed (TODO: capture the discussions in a wiki page here).

Make the formula portable

Documentation

Tests and CI

States

  • Make the formula "actually functional" so that all parts can be run, to aid understanding and quality assurance.
  • Adapt template-formula to not be too focussed on pkg:
    • Must also consider archives, tarball releases, GitHub repos, etc.
  • Add the remove state advocated by @noelmcloughlin.
  • Ensure that each file does only one thing and do it well (instead of monolithic files):
    • E.g. pkg/install.sls, pkg/clean.sls, repo/install.sls, repo/clean.sls, etc.
  • Add 'formula components' that can be reused in a formula as suggested by @javierbertoli, such as:
    • 'service-template'
    • 'pkg-template'
    • 'ini-config-template'
    • 'yaml-config-template'
    • Etc.
  • Prevent the formula from running on unsupported minions -- WIP: feat(unsupported): prevent formula running on unsupported minions #91.

Repo standards

Other resources to evaluate for inclusion

@daks
Copy link
Member

daks commented Feb 26, 2019

Just an idea: document how to create a formula from this template one, inside this repository or/and in saltstack docs

@myii
Copy link
Member Author

myii commented Feb 26, 2019

@daks I've added that to the task list above -- thanks for that suggestion.

@alxwr
Copy link
Member

alxwr commented Apr 30, 2019

@myii I just wanted to say „Thank you!“ :-)
This formula enabled me to implement https://github.com/saltstack-formulas/prometheus-formula really fast. I found the structure and especially your turnkey Travis/kitchen configuration very helpful.

@myii
Copy link
Member Author

myii commented Apr 30, 2019

@alxwr That's really good to hear and it makes all of this effort worthwhile. There were lots of contributions involved, including your own! Good to see that you've got semantic-release running as well.

We've got an upcoming issue due to wanting to implement config.get properly, instead of pillar.get. The only solution I've been able to find so far is using defaults.merge so I'm going to submit a PR in order to open up discussion. Since you're familiar with salt-ssh, I'll request a review from you as well, so that you have the opportunity to argue against my proposal! Actually, the alternative is to go back to pillar.get and that may be the final conclusion anyway...

@alxwr
Copy link
Member

alxwr commented Apr 30, 2019

@myii AFAIR we could work around the defaults.merge issue, because we can detect whether salt or salt-ssh is used. But this is not pretty. :-)
Maybe it's time for a libworkaround.jinja which would be living documentation of several incompatibilities and shortcomings that need to go. :-)

@myii
Copy link
Member Author

myii commented Apr 30, 2019

@alxwr So there's a reliable way to check for salt-ssh? That's excellent news. So we can have both methods of merging the map available and then no-one loses out. Do you mind sharing it here? Then I can include it in the PR immediately, so that everyone can get on with reviewing it, rather than having a long discussion first. I will ensure that you are credited as the author of that specific commit, identifying whether salt or salt-ssh is being used.

@alxwr
Copy link
Member

alxwr commented May 1, 2019

@myii Sry for the delay.:-)
The way I found most straightforward, is inserting grains:salt_ssh: True in the roster file. But I get, that this is not exactly portable.

The second most reliable way is to compare paths:

salt host grains.itms:

    pythonpath:                                                                                                                                                                               
        - /usr/local/bin                                                                                                                                                                      
        - /usr/local/lib/python36.zip                                                                                                                                                         
        - /usr/local/lib/python3.6                                                                                                                                                            
        - /usr/local/lib/python3.6/lib-dynload                                                                                                                                                
        - /usr/local/lib/python3.6/site-packages
    saltpath:
        /usr/local/lib/python3.6/site-packages/salt

salt-ssh host2 grains.itms:

    pythonpath:                                                                                                                                                                               
        - /var/tmp/.root_99b0b6_salt/py3                                                                                                                                                      
        - /var/tmp/.root_99b0b6_salt/pyall                                                                                                                                                    
        - /var/tmp/.root_99b0b6_salt                                                                                                                                                          
        - /usr/lib/python35.zip                                                                                                                                                               
        - /usr/lib/python3.5                                                                                                                                                                  
        - /usr/lib/python3.5/plat-x86_64-linux-gnu                                                                                                                                            
        - /usr/lib/python3.5/lib-dynload                                                                                                                                                      
        - /usr/local/lib/python3.5/dist-packages                                                                                                                                              
        - /usr/lib/python3/dist-packages
    saltpath:                                                                                                                                                                                 
        /var/tmp/.root_99b0b6_salt/pyall/salt

salt-ssh is run from a temporary directory on the minion. This shows in the grain saltpath. (We could also correlate pythonpath[0] with saltpath for higher certainty, but I think focusing on saltpath is sufficient.)

@myii
Copy link
Member Author

myii commented May 1, 2019

@alxwr How about the method in #102? How does that work for you? I got a salt-ssh minion prepared and tested the map both ways. The __cli option appears to be reliable. I did notice the path differences you mentioned above but this appears to be even simpler.

@alxwr
Copy link
Member

alxwr commented May 1, 2019

@myii there is another way to detect salt-ssh, but this involves the py renderer.

__opts__ differs when using salt-ssh:

def config_dir():
    if '__master_opts__' in __opts__:
        # run started via salt-ssh
        return __opts__['__master_opts__']['config_dir']
    else:
        # run started via salt
        return __opts__['config_dir']

Taken from: https://github.com/saltstack-formulas/openssh-formula/blob/master/_pillar/known_hosts_salt_ssh.sls

@myii
Copy link
Member Author

myii commented May 1, 2019

@alxwr Do you mind if we discuss this in #102? It's a bit off-topic for this issue and then it opens up the discussion with the others, so that we can reach a resolution. It's my fault for bringing it up here first!

@alxwr
Copy link
Member

alxwr commented May 1, 2019

@myii I was just going to suggest that. :-)

@CosmicToast
Copy link

Does the merge of saltstack/salt#52156 affect map.jinja and similar in any way?
Might be nice as a replacement to tplroot and/or things like include: - template.X.

@myii
Copy link
Member Author

myii commented May 27, 2019

@5paceToast Appreciate you bringing that merge to our attention. A cursory look suggests that we will be able to improve our use of include at the very least. The problem is that this will take a while to reach us; we generally have to support the three versions of Salt at any given time. So right now, that means back to 2017.7. It all depends on how far back that merge is backported.

In terms of tplroot, that's in the queue at saltstack/salt#51814.

@CosmicToast
Copy link

@myii in that case, it might be nice to also have a template-formula variant that applies only to the latest release, for people interested in that (i.e for development, learning and other purposes).
Would that be politically feasible at all?

@myii
Copy link
Member Author

myii commented May 27, 2019

@5paceToast There have been conversations about having various types of template-formula. There are no right or wrong answers as such but a lot comes down to keeping everything maintained and synchronised. The general direction is to focus on this template-formula for the time being, probably until most of the tasks laid out above are completed. Not to mention the amount of time and effort it requires to propagate and review the changes when applying to the rest of the formulas, which has already been taking place thanks to numerous contributors.

In terms of a template-formula for the latest release only, I would surmise that would be fairly low priority. Not because it isn't a good idea; rather, it would be an extremely useful reference as we bring each formula up-to-date over time. However, the issue remains that we have to support all of the current Salt versions across all 300+ formulas in this org. I'm not sure if there is a formula out there that would benefit from a template-formula for the latest release only.

If someone had the desire to go ahead with this idea, I'm sure there are contributors here who would be happy to offer advice along the way, either here in GitHub or in our Slack/IRC/Matrix room. An actual functioning repo in place becomes an excellent stimulus for further discussions about the merits and pitfalls of such an approach and whether it would be something worth supporting at an org level.

Going back to saltstack/salt#52156 for a moment: this is only available in develop (so far). If it does reach 2019.2, what will the latest release template-formula do? Meaning, the earlier versions of the 2019.2 series (e.g. 2019.2.0, 2019.2.1) won't contain that functionality, so it would be no better than where we are right now.

But from another angle, how about the idea of a template-formula that's actually based on the develop branch, rather than the latest release? Now that would definitely generate some interest. In fact, it could be worth having a develop branch in this repo, to track exactly that. I'll add that to the list above -- thanks for the idea!

@myii
Copy link
Member Author

myii commented Jun 3, 2019

@5paceToast Thanks to @javierbertoli, we've now got testing images for develop. I've already run initial tests with tojson, which shows why we can't use it yet due to having to support 2017.7. So this is an excellent development.

The funny thing is when I looked at the upstream merge again in more detail, I realised that there isn't any way it can be used within this formula, since it is about pillar includes. In any case, at least the discussion led to some useful outcomes.

@CosmicToast
Copy link

The funny thing is when I looked at the upstream merge again in more detail, I realized that there isn't any way it can be used within this formula, since it is about pillar includes. In any case, at least the discussion led to some useful outcomes.

I noticed that too, but shortly after also noticed the include/exclude docs which claim that relative includes were added way back in 0.16.0 while parent-based (grandparent etc) includes in 2015.8. (something that might be usable in template.config.file, for example!)

It seems like some new features sometimes get missed, and some don't even make it to the docs (salt.fileserver.s3fs barely even makes sense without salt.pillar.s3 as context, for example).
Which is why I think a sort of "latest" version of template-formula really makes sense (a develop branch does indeed sound great to me)!

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

No branches or pull requests

4 participants