major overhaul to align with the template-formula

[rbjorklin]

• see: https://github.com/saltstack-formulas/template-formula
• systemd unit
  file:https://learn.hashicorp.com/vault/operations/ops-vault-ha-consul#vault-server-systemd-unit-file
• currently the self-signed certificate part has not been ported but
  that can be done upon request

[rbjorklin]

@myii @noelmcloughlin Please review.

[myii]

@rbjorklin After our discussion on Slack, I wasn't expecting an overhaul to this extent! I'm a bit short of time right now but I've had a cursory look and it looks good. I've got a couple of very minor points that I'll raise inline. This will require some testing and I hope that others who are more familiar with this formula can provide that feedback.

@dafyddj @myoung34 Since you have the largest contributions to this formula, would you mind having a look over this PR?

[myii]

@rbjorklin Could you review the CI logs in order to get the tests passing?

[myii]

This could be tidied up a little more with reference to https://github.com/saltstack-formulas/template-formula/blob/master/template/map.jinja.

[myii]

To be more helpful:

1. Use or {}.
2. Consider the comment below.
3. Merging strategy and layout.

[rbjorklin]

Thanks for the updated comment but I still don't follow. If I remove the base part it doesn't merge correctly. Could you please clarify?

[myii]

@rbjorklin Use this instead:

# -*- coding: utf-8 -*-
# vim: ft=sls syntax=yaml softtabstop=2 tabstop=2 shiftwidth=2 expandtab autoindent

{% import_yaml "vault/yaml/defaults.yaml" or {} as default_settings %}
{% import_yaml "vault/yaml/osfamilymap.yaml" or {} as osfamilymap %}
{% import_yaml "vault/yaml/initfamilymap.yaml" or {} as initfamilymap %}

{%- set defaults = salt['grains.filter_by'](default_settings,
    default='vault',
    merge=salt['grains.filter_by'](osfamilymap, grain='os_family',
      merge=salt['grains.filter_by'](initfamilymap, grain='init',
        merge=salt['pillar.get']('vault:lookup', default={})
      )
    )
) %}

{#- Merge the vault pillar #}
{%- set vault = salt['pillar.get']('vault', default=defaults, merge=True) %}

Please test the above since it's a direct conversion using the template-formula as a basis.

[myii]

@rbjorklin Since you've just reset the location of the .yaml files, then the paths need to be updated for the example above (vault/yaml/... => vault/...).

[rbjorklin]

What's the vault:lookup for?

[myii]

@rbjorklin The lookup is mentioned in the main documentation both here and here. For some discussions about this, refer to:

• vm.overcommit_memory is in incorrect block in pillar redis-formula#75 (comment)
  • The discussion starts with this comment.
• Fix apache:lookup regression apache-formula#252 (comment)

While vault:lookup may not exist now, this structure allows for it to be adopted at a later stage.

[rbjorklin]

Cleaned up!

[myii]

@rbjorklin Thanks for this. Just missing the or {}, which was raised on Slack, reported as an issue and was fixed by this PR. There was a subsequent conversation where the person was redirected to the fix.

So that would be:

{% import_yaml "vault/defaults.yaml" or {} as defaults %}
{% import_yaml "vault/osfamilymap.yaml" or {} as osfamilymap %}
{% import_yaml "vault/initfamilymap.yaml" or {} as initfamilymap %}

[myii]

Redundant use of both watch and watch_in. Easier to keep the watch, since vault-package-install-archive-extracted is missing the watch_in. If selecting the watch_in instead, then the missing watch_in needs to be added.

[rbjorklin]

If you look closely you can see that the watch_in refers to a non-existing id. I missed this while cleaning up.

[myii]

@rbjorklin Note, these are missing the state type (archive, file & file respectively). So should be:

    - watch:
      - archive: vault-package-install-archive-extracted
      - file: vault-service-init-file-managed
      - file: vault-config-config-file-serialize

[rbjorklin]

Not strictly necessary any more: https://docs.saltstack.com/en/latest/ref/states/requisites.html#omitting-state-module-in-requisites

[myii]

This is generally kept explicit across formulas.

[rbjorklin]

I don't have a strong opinion either way, I just kept it the way it was in this formula. I'll let @myoung34 have the final word here.

[noelmcloughlin]

I usually keep explicit

[noelmcloughlin]

Hi @rbjorklin Congrats on producing a good PR. It LG to me having reviewed syntax and context.

I have some minor suggestions-

• Merge signature.sls into gpg.sls since both are concerned with gpg checking?
• Include vault.package.gpg and vault.package.gpg.clean states in formula.
• include an osmap.yaml file to handle platform: xxxxx variable for MacOS.

thanks
Noel

[rbjorklin]

Thanks for you feedback @myii and @noelmcloughlin!
@noelmcloughlin could you provide an appropriate grain for MacOS? I don't have access to that platform.

[noelmcloughlin]

Thanks for you feedback @myii and @noelmcloughlin!
@noelmcloughlin could you provide an appropriate grain for MacOS? I don't have access to that platform.

The grains.os value is MacOS.

[dafyddj]

Hi, while this is a great effort, I just wonder whether it couldn't be done in a more piecemeal fashion.
There are a lot of file renames which obscure what is actually changed.
For example, amongst other things, I notice you have removed the upgrade logic I put in in my previous changes, which means the upgrade test (install_binary), done manually due to limitations in Test Kitchen, now fails.

[noelmcloughlin]

Thanks for you feedback @myii and @noelmcloughlin!
@noelmcloughlin could you provide an appropriate grain for MacOS? I don't have access to that platform.

The grains.os value is MacOS.

Also you need to introduce a vault.package.gpg.clean.sls file for completeness. It probably just deletes the signature/gpg files or basically reverse of .gpg.init.

[rbjorklin]

@dafyddj when you say upgrade logic do you mean the possibility of having multiple versions of Vault installed at the same time? (That's the only thing I can remember removing). I didn't know there was an actual use case for that and would happily put it back in there if there's a valid use case!

@noelmcloughlin I actually created that but I seem to have missed git add on the file...

[noelmcloughlin]

Hi @aboe76 @myii @javierbertoli

how about moving all yamls to ./yaml/ subdirectory. I'm +1 on that (but I recall someone else voted -1); I don't like tarbomb directory formats. This one is not too bad but some are bad.

Secondly how about renaming ./files/ to ./jinja because its a lame name and consistently across this org that's what the directory contains?

[myii]

@noelmcloughlin Some basic responses to your questions.

how about moving all yamls to ./yaml/ subdirectory. I'm +1 on that (but I recall someone else voted -1); I don't like tarbomb directory formats. This one is not too bad but some are bad.

I haven't given this much thought so far. One angle is that once we start modularising the formulas as we've started doing, then this will reduce the "tarbomb" effect.

Secondly how about renaming ./files/ to ./jinja because its a lame name and consistently across this org that's what the directory contains?

This won't always work, especially where non-Jinja templates are required. This actually came up in the Slack/IRC/Matrix room yesterday, as a concern about TOFS being Jinja-centric.

[dafyddj]

when you say upgrade logic do you mean the possibility of having multiple versions of Vault installed at the same time? (That's the only thing I can remember removing). I didn't know there was an actual use case for that and would happily put it back in there if there's a valid use case!

The use case is to upgrade from one version to the next. Admittedly, yes, it leaves the previous version on the system, but I never got around to cleaning that up.
Unless I'm mistaken, your changes do not allow for a version upgrade.

[rbjorklin]

@dafyddj I had another look at the formula and also read through: https://www.vaultproject.io/docs/upgrading/index.html and from what I can tell I didn't change the way the state works and how it is now should be compatible with the upgrade guide from Hashicorp. Do you have an example of something that worked before this PR that is broken with it?

[noelmcloughlin]

@noelmcloughlin Some basic responses to your questions.

how about moving all yamls to ./yaml/ subdirectory. I'm +1 on that (but I recall someone else voted -1); I don't like tarbomb directory formats. This one is not too bad but some are bad.

I haven't given this much thought so far. One angle is that once we start modularising the formulas as we've started doing, then this will reduce the "tarbomb" effect.

Secondly how about renaming ./files/ to ./jinja because its a lame name and consistently across this org that's what the directory contains?

This won't always work, especially where non-Jinja templates are required. This actually came up in the Slack/IRC/Matrix room yesterday, as a concern about TOFS being Jinja-centric.

It will work for jinja templates - the idea being that TECH-content is stored in TECH-directory.
Anyway I'll approve the this PR since this is more strategy question.

[noelmcloughlin]

LGTM, thanks @rbjorklin

[myii]

@noelmcloughlin Having another neutral name (i.e. not ./jinja) is definitely something worth discussing. So I wasn't trying to protect ./files specifically!

[myii]

@rbjorklin For the current set of commits, they can be amended to something like the following:

refactor(template): major overhaul to align with the template-formula
refactor(template): fix review comments & tests
refactor(template): fix more test cases
revert(defaults): revert some defaults that were incorrectly changed
fix(config): move watch statement for config as it breaks under dev_mode
refactor(map): cleanup the map.jinja merge & add lookup
fix(backend): fix more review comments
fix(clean): add missed cleanup file & add storage backend to prod test
fix(upgrade): fix upgrade procedure & add MacOS platform
fix(test): update test, clean link

For more information, have a look at CONTRIBUTING. Thanks to @dafyddj, this formula has now adopted semantic-release from the template-formula (#25), which means that the commit messages are used to automatically bump the formula version number as well as populate the CHANGELOG.

---

A recommendation I have here is to keep this as the "master" PR, which shows the overall changes being proposed. Then start using this to produce smaller, feature-based PRs that can be tested and merged with more confidence. This will also be the opportunity to improve the commit messages that will be used in the CHANGELOG as well as spread these changes over multiple, automated releases. How do you feel about that?

[myii]

Picking neutral name like ./files/ or ./template/ is suitable for high level API. But for lower level implementation API I like the idea of quickly seeing what data or text renderers are used (or not) by reviewing directory structure. See https://docs.saltstack.com/en/latest/ref/renderers/index.html#two-kinds-of-renderers. Furthermore a formula could support few different data/text renderers in theory - we should not assume its one or the other going forward.

@noelmcloughlin One problem we're going to run into with this is the TOFS pattern relies on a single, standard directory name when looking for templates. By default, this is files/ but that can be overridden to another value via. pillar/config. But multiple template directories just isn't going to work. Have a look at this open PR on template-formula to see the source lookups that can end up taking place.

[dafyddj]

A recommendation I have here is to keep this as the "master" PR, which shows the overall changes being proposed. Then start using this to produce smaller, feature-based PRs that can be tested and merged with more confidence. This will also be the opportunity to improve the commit messages that will be used in the CHANGELOG as well as spread these changes over multiple, automated releases. How do you feel about that?

Due to the many changes introduced in this PR, I am also in favour of this approach.

[rbjorklin]

While I fully see your point about splitting this up it will create more work for me, time that is taken out of my spare time. This PR is not nearly as big as it might seem change wise due to the most line changes being renames or the addition of the clean state which isn't strictly necessary. There's really only two changes being made to how this formula works:

• The configuration is now written as nicely formatted json straight from a datastructure instead of templated as a .hcl file
• Uses archive.extracted on a remote file and doesn't keep old versions around on the system.

PS. For any future PR I may submit I will make sure to keep the size down and changes separated.

EDIT: The two actual changes in this state are covered by the automatic & manual test suite which at least gives me confidence in this PR being correct at this point.

[myii]

@rbjorklin I appreciate your concerns. In terms of the commit messages, are you happy to rebase your branch to accommodate for those? In order that the release can be prepared automatically. One of the commit messages (preferably the one that changes state names) needs to have a BREAKING CHANGE last line, as outlined here.

[dafyddj]

This is looking good. Thanks for the significant contribution!

[myii]

@dafyddj Thanks for making the time for reviewing this PR.

@rbjorklin It's up to you: would you like to wait for the review from @myoung34 or shall we go ahead and merge this?

[rbjorklin]

Big thanks to everyone for reviewing my changes and providing feedback 😃

If everyone else is okay with merging this I am too.

[myii]

@rbjorklin @dafyddj Merged! That was a big one -- great effort all round. v1.0.0 imminent...

[saltstack-formulas-travis]

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[myii]

@rbjorklin In case you're wondering, the AUTHORS file is updated from statistics pulled via. the GitHub API. That's not always the freshest data so your contribution isn't shown yet. Expect it to be there when the next release takes place.