search

saltstack-formulas / vault-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
15 stars 59 forks source link

major overhaul to align with the template-formula #23

Closed rbjorklin closed 5 years ago

rbjorklin commented 5 years ago
rbjorklin commented 5 years ago

@myii @noelmcloughlin Please review.

myii commented 5 years ago

@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 commented 5 years ago

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

rbjorklin commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

myii commented 5 years ago

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

noelmcloughlin commented 5 years ago

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

Hi @myii ahh okay, but what I mean is that we should be explicit, not neutral.

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.

I appreciate this is one of those "lets think about it" topics and I'm thinking current conventions could be reviewed and questioned.

dafyddj commented 5 years ago

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

There is a test for upgrading Vault, which unfortunately has to be performed manually due to limitations in Test Kitchen:

  1. In .kitchen.yml, under the suite named install_binary, set version: 0.11.1.
  2. Run kitchen converge install-binary.
  3. Set version: 0.11.2.
  4. Run kitchen converge install-binary.
  5. Run kitchen verify install-binary.
  6. With these changes, test fails.

The version test fails because the Vault binary hasn't changed, due to the way archive.extracted works. How can Salt know to replace the binary unless it does so on every state.apply run?

rbjorklin commented 5 years ago

Thanks for that very clear explanation @dafyddj. I've updated the formula so that upgrades should now be possible. Tests pass but a commtlint seems to have been added which my commits are failing. What's the recommended approach to fix this?

myii commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

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 commented 5 years ago

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

myii commented 5 years ago

@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 commented 5 years ago

Big thanks to everyone for reviewing my changes and providing feedback :smiley:

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

myii commented 5 years ago

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

saltstack-formulas-travis commented 5 years ago

:tada: This PR is included in version 1.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

myii commented 5 years ago

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