Ease reusability of the template #180

dafyddj · 2019-11-22T23:34:32Z

PR progress checklist (to be filled in by reviewers)

Changes to documentation are appropriate (or tick if not required)
Changes to tests are appropriate (or tick if not required)
Reviews completed

What type of PR is this?

Primary type

[build] Changes related to the build system
[chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ci] Changes to the continuous integration configuration
[feat] A new feature
[fix] A bug fix
[perf] A code change that improves performance
[refactor] A code change that neither fixes a bug nor adds a feature
[revert] A change used to revert a previous commit
[style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[docs] Documentation changes
[test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

Yes.
All state names and state ids are changed to the new unique token.

Related issues and/or pull requests

#98 slightly

Describe the changes you're proposing

While creating my first formula using this template, I found it wasn't as straightforward as I'd hoped.
Obviously the word template is used all over this formula and some need changing and some don't.

This is just an idea I had of uniquely marking out the replaceable template by using the token template__ - of course, it could be something else too.

Then a simple global replace can be performed while converting the template to a real formula.

Thoughts before I continue further?

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Updated the README (e.g. Available states).
Updated pillar.example.

Testing checklist

Included in Kitchen (i.e. under state_top).
Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
Updated the relevant test pillar.

Additional context

myii · 2019-11-23T01:10:58Z

Thanks for starting this PR, @dafyddj. It's an interesting take on things, definitely worth a discussion. I'd like to start by pulling in other contributors who have experience with conversions using the template-formula (based on previous discussions). Hopefully, we can formulate an effective solution to make this process as efficient as possible.

CC: @saltstack-formulas/wg @xenophonf @GMAzrael.

Some alternatives to compare against:

Provide a script to convert this formula according to cookiecutter-style input after cloning.
Prepare detailed documentation to perform the conversion after cloning.
Have a separate repo that's effectively a "pure" template (not numerous things mixed together, as we have in this repo).

myii · 2019-11-25T20:21:55Z

@dafyddj Thanks for the ongoing effort here, this will definitely be useful in resolving this problem. I hope others can share their opinions, so that we select the best approach going forwards.

daks · 2019-11-26T08:34:27Z

I think that in the middle/long term it should be nice to have a tool/script to generate a formula from scratch like other config management tools have. Maybe cookiecutter can be used for it but I don't really know it so I can't judge if it'll work for our needs.
And maybe we already have a certain stability in this formula to work on it already.

In the meantime (until someone take this job or we consider this a priority, maybe we already should?) manual generation (like documented in my PR) can be nice, and if this PR help it, I'm ok with it.

myii · 2019-11-26T12:45:16Z

@dafyddj The script looks great, this is really taking shape. Some comments:

Since our formulas are consumed across various OSes, can we make this as portable as possible (i.e. POSIX standard)?
- sh rather than bash
- Any alternative to perl, such as sed
Apply the improvements suggested by https://www.shellcheck.net/
- Of course, ignoring wherever necessary
- As a side note, this is integrated by default into Travis CI, which I think would be a great addition to the lint stage -- thoughts?

* saltstack-formulas/template-formula#180 (comment)

myii · 2019-11-27T14:00:37Z

@dafyddj I've started #181, with the plan to propagate using shellcheck to all of the formulas. Ignores are simple enough where required.

* saltstack-formulas/template-formula#180 (comment)

# [1.63.0](v1.62.0...v1.63.0) (2019-11-27) ### Code Refactoring * **travis:** use pathspecs for `git ls-files` instead of `grep` ([615e3b2](615e3b2)), closes [/github.com/saltstack-formulas/template-formula/pull/181#discussion_r351421463](https://github.com//github.com/saltstack-formulas/template-formula/pull/181/issues/discussion_r351421463) ### Features * **shellcheck:** apply fixes throughout this repo ([1ea7fbb](1ea7fbb)) * **travis:** run `shellcheck` during lint job ([f52eb37](f52eb37)), closes [/github.com/saltstack-formulas/template-formula/pull/180#issuecomment-558612422](https://github.com//github.com/saltstack-formulas/template-formula/pull/180/issues/issuecomment-558612422)

bin/convert-formula.sh

myii · 2019-12-02T18:17:37Z

@dafyddj Thanks for moving over to sh (and for ex!). A quick question, not prescriptive: what are your thoughts on using uppercase TEMPLATE instead of template__? I haven't thought this through but it just occurred to me, so I thought I'd ask.

dafyddj · 2019-12-02T23:13:23Z

I'm not particularly attached to template__ and I think TEMPLATE could work because the search/replace is case-sensitive. Would we ever have a legitimate need to use TEMPLATE in the formula? Probably not.

Also, do you agree that this conversion process should be CI tested? I'm thinking, in Travis, clone the repo, run the script, then run at least one of the test suites in that cloned repo. Thoughts?

myii · 2019-12-02T23:16:33Z

Also, do you agree that this conversion process should be CI tested? I'm thinking, in Travis, clone the repo, run the script, then run at least one of the test suites in that cloned repo. Thoughts?

@dafyddj Definitely useful and worthwhile. A single job would be enough to test that.

BREAKING CHANGE: changed all state names and ids

dafyddj · 2019-12-05T17:37:30Z

@myii this is mostly done as far as I'm concerned.

It could probably do with some documentation. Can you suggest where best to place the docs - and should the script remove that part of the documentation when it's done?

myii · 2019-12-05T17:52:16Z

@myii this is mostly done as far as I'm concerned.

Great, I'm going to take this for a test drive soon, since I need to make a formula or two. That will help me finalise the review.

It could probably do with some documentation. Can you suggest where best to place the docs - and should the script remove that part of the documentation when it's done?

The README is a good place. A couple of places to consider:

A sub-heading under https://github.com/saltstack-formulas/template-formula#special-notes
Or since this repo is a template, we could use a section right at the very top, before the general notes

Since the end-user is going to be editing the README for their own purposes, I don't think the script needs to remove this. I'll confirm this after my tests.

myii · 2019-12-12T18:44:54Z

bin/convert-formula.sh

+  git commit --quiet --all \
+    --message 'feat: convert `template-formula` to `'"${NEW_NAME}"'-formula`
+
+BREAKING CHANGE: changed all state names and ids'


Suggestion:

git commit --quiet --all \ --message 'feat: convert `template-formula` to `'"${NEW_NAME}"'-formula`' \ --message 'BREAKING CHANGE: changed all state names and ids'

myii · 2019-12-12T18:47:04Z

bin/convert-formula.sh

+  git rm --quiet bin/convert-formula.sh AUTHORS.md CHANGELOG.md
+  git mv TEMPLATE "${NEW_NAME}"
+  grep --recursive --files-with-matches --exclude-dir=.git TEMPLATE . \
+    | xargs -L 1 ex -sc '%s/TEMPLATE/'"${NEW_NAME}"'/g|x'


In my first local tests, the script doesn't get past this line. Since this is working in Travis, there's probably something in the way I've patched things together. The only way past it is to comment out line 3, i.e.:

# set -o errexit # If a command fails exit the whole script

You might get some clues by running as (if you haven't already done so):
DEBUG=true bin/convert-formula.sh ...

Yes, I started with that and then tried all sorts, including removing all of the quiet or silent flags but to no avail. Even when I get it running with this line commented out, there are no warning given. Not sure what's happening with that ex command to cause an error status...

@dafyddj This sounds a little strange but I found a solution (using -u NONE) based on this reference:

https://unix.stackexchange.com/questions/14497/why-would-vim-return-non-zero-exit-code-if-i-exit-immediately-after-opening

So that's:

grep --recursive --files-with-matches --exclude-dir=.git TEMPLATE . \ | xargs -L 1 ex -u NONE -sc '%s/TEMPLATE/'"${NEW_NAME}"'/g|x'

This actually worked, with set -o errexit re-enabled.

Maybe the end user (like me) could have something strange in their configuration, affecting ex when it exits?

myii · 2019-12-12T20:46:33Z

bin/convert-formula.sh

+  git reset \
+    "$(echo 'feat: initial commit' \
+      | git commit-tree 'HEAD^{tree}')"
+  git rm --quiet bin/convert-formula.sh AUTHORS.md CHANGELOG.md


Suggested change

git rm --quiet bin/convert-formula.sh AUTHORS.md CHANGELOG.md

git rm --quiet bin/convert-formula.sh AUTHORS.md CHANGELOG.md \

docs/_static/css/custom.css docs/AUTHORS.rst docs/CHANGELOG.rst \

docs/conf.py docs/CONTRIBUTING_DOCS.rst docs/index.rst

myii · 2019-12-13T07:31:20Z

Current status:

Discuss and finalise changes as suggested above.
Add notes to the README, including explaining that git push -f needs to be used and perhaps how to configure Travis to enable semantic-release (e.g. GH_TOKEN, etc.).

A wish-list collection for the long-run, doesn't prevent merging this PR:

README: Update the headings symbols to match the length of the heading itself.
.rubocop.yml: Remove the Metrics/BlockLength section, specific to the template-formula.
.travis.yml: Remove the env: 'Conversion' block.
FORMULA: reset the version to 1.0.0.

myii · 2019-12-16T14:25:07Z

@dafyddj Merged, the rest of the issues can be covered in a future PR. Thanks for this fantastic PR.

saltstack-formulas-travis · 2019-12-16T14:36:34Z

🎉 This PR is included in version 4.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

…ormulas#180 * saltstack-formulas#180 (comment) * saltstack-formulas#180 (comment) * saltstack-formulas#180 (comment)

fix(convert-formula.sh): apply remaining suggestions from #180

## [4.0.1](v4.0.0...v4.0.1) (2019-12-17) ### Bug Fixes * **convert-formula.sh:** apply remaining suggestions from [#180](#180) ([76ecd44](76ecd44)), closes [/github.com//pull/180#discussion_r357308821](https://github.com//github.com/saltstack-formulas/template-formula/pull/180/issues/discussion_r357308821) [/github.com//pull/180#discussion_r357318860](https://github.com//github.com/saltstack-formulas/template-formula/pull/180/issues/discussion_r357318860) [/github.com//pull/180#discussion_r357362707](https://github.com//github.com/saltstack-formulas/template-formula/pull/180/issues/discussion_r357362707)

* saltstack-formulas/template-formula#180 * saltstack-formulas/template-formula#183

* saltstack-formulas/template-formula#180

dafyddj requested a review from myii November 22, 2019 23:37

dafyddj force-pushed the reusability branch from a09cef9 to 36bbebe Compare November 25, 2019 17:53

dafyddj force-pushed the reusability branch from 3bbc23b to 33d901f Compare November 26, 2019 03:07

myii added a commit to myii/ssf-formula that referenced this pull request Nov 27, 2019

feat(travis): run shellcheck to lint job

6554896

* saltstack-formulas/template-formula#180 (comment)

This was referenced Nov 27, 2019

feat(travis): run shellcheck during lint job myii/ssf-formula#106

Merged

[ssf] ci(travis): run shellcheck during lint job #181

Merged

myii added a commit to myii/ssf-formula that referenced this pull request Nov 27, 2019

feat(travis): run shellcheck during lint job

3bf311c

* saltstack-formulas/template-formula#180 (comment)

myii added a commit to myii/ssf-formula that referenced this pull request Nov 27, 2019

feat(travis): run shellcheck during lint job

f52eb37

* saltstack-formulas/template-formula#180 (comment)

dafyddj force-pushed the reusability branch 2 times, most recently from 8ea358a to 42f5fa4 Compare November 27, 2019 20:44

dafyddj force-pushed the reusability branch from 42f5fa4 to 0be19a9 Compare November 28, 2019 01:45

myii reviewed Nov 29, 2019

View reviewed changes

bin/convert-formula.sh Outdated Show resolved Hide resolved

dafyddj force-pushed the reusability branch 2 times, most recently from b907232 to 703750f Compare November 30, 2019 19:46

dafyddj added 3 commits December 4, 2019 23:47

refactor: improve reusability using an unique keyword TEMPLATE

2e8ded6

BREAKING CHANGE: changed all state names and ids

feat: add script to ease conversion from template to real formula

edfa269

test: add CI test of conversion script

7ad85ae

dafyddj force-pushed the reusability branch from 838cadb to 7ad85ae Compare December 5, 2019 00:03

myii reviewed Dec 12, 2019

View reviewed changes

myii merged commit 7ad85ae into saltstack-formulas:master Dec 16, 2019

myii changed the title ~~WIP: Ease reusability of the template~~ Ease reusability of the template Dec 16, 2019

saltstack-formulas-travis added the released label Dec 16, 2019

myii added a commit to myii/template-formula that referenced this pull request Dec 16, 2019

fix(convert-formula.sh): apply remaining suggestions from saltstack-f…

76ecd44

…ormulas#180 * saltstack-formulas#180 (comment) * saltstack-formulas#180 (comment) * saltstack-formulas#180 (comment)

myii mentioned this pull request Dec 16, 2019

fix(convert-formula.sh): apply remaining suggestions from #180 #182

Merged

19 tasks

dafyddj deleted the reusability branch December 17, 2019 14:41

myii added a commit that referenced this pull request Dec 17, 2019

Merge pull request #182 from myii/fix/improve-reusability

3901e13

fix(convert-formula.sh): apply remaining suggestions from #180

dafyddj mentioned this pull request Dec 19, 2019

Improve reusability #183

Merged

19 tasks

myii added a commit to myii/ssf-formula that referenced this pull request Dec 23, 2019

feat(template): improve reusability

491791c

* saltstack-formulas/template-formula#180 * saltstack-formulas/template-formula#183

myii mentioned this pull request Dec 23, 2019

feat(template): improve reusability myii/ssf-formula#113

Merged

myii added a commit to myii/ssf-formula that referenced this pull request Feb 4, 2020

feat(template): update to use TEMPLATE for reusability

bcc8177

* saltstack-formulas/template-formula#180

myii mentioned this pull request Feb 4, 2020

feat(template): update to use TEMPLATE for reusability myii/ssf-formula#123

Merged

myii added a commit to myii/ssf-formula that referenced this pull request Feb 4, 2020

feat(template): update to use TEMPLATE for reusability

d5c68e5

* saltstack-formulas/template-formula#180

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ease reusability of the template #180

Ease reusability of the template #180

dafyddj commented Nov 22, 2019 •

edited

Loading

myii commented Nov 23, 2019

myii commented Nov 25, 2019

daks commented Nov 26, 2019

myii commented Nov 26, 2019

myii commented Nov 27, 2019 •

edited

Loading

myii commented Dec 2, 2019 •

edited

Loading

dafyddj commented Dec 2, 2019

myii commented Dec 2, 2019

dafyddj commented Dec 5, 2019

myii commented Dec 5, 2019

myii Dec 12, 2019

myii Dec 12, 2019

dafyddj Dec 12, 2019

myii Dec 12, 2019

myii Dec 12, 2019

myii Dec 12, 2019

myii commented Dec 13, 2019

myii commented Dec 16, 2019

saltstack-formulas-travis commented Dec 16, 2019

-  git rm --quiet bin/convert-formula.sh AUTHORS.md CHANGELOG.md
+  git rm --quiet bin/convert-formula.sh AUTHORS.md CHANGELOG.md \
+    docs/_static/css/custom.css docs/AUTHORS.rst docs/CHANGELOG.rst \
+    docs/conf.py docs/CONTRIBUTING_DOCS.rst docs/index.rst

Ease reusability of the template #180

Ease reusability of the template #180

Conversation

dafyddj commented Nov 22, 2019 • edited Loading

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

myii commented Nov 23, 2019

myii commented Nov 25, 2019

daks commented Nov 26, 2019

myii commented Nov 26, 2019

myii commented Nov 27, 2019 • edited Loading

myii commented Dec 2, 2019 • edited Loading

dafyddj commented Dec 2, 2019

myii commented Dec 2, 2019

dafyddj commented Dec 5, 2019

myii commented Dec 5, 2019

myii Dec 12, 2019

Choose a reason for hiding this comment

myii Dec 12, 2019

Choose a reason for hiding this comment

dafyddj Dec 12, 2019

Choose a reason for hiding this comment

myii Dec 12, 2019

Choose a reason for hiding this comment

myii Dec 12, 2019

Choose a reason for hiding this comment

myii Dec 12, 2019

Choose a reason for hiding this comment

myii commented Dec 13, 2019

myii commented Dec 16, 2019

saltstack-formulas-travis commented Dec 16, 2019

dafyddj commented Nov 22, 2019 •

edited

Loading

Does this PR introduce a `BREAKING CHANGE`?

myii commented Nov 27, 2019 •

edited

Loading

myii commented Dec 2, 2019 •

edited

Loading