-
Notifications
You must be signed in to change notification settings - Fork 86
Ease reusability of the template #180
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
Conversation
|
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 CC: @saltstack-formulas/wg @xenophonf @GMAzrael. Some alternatives to compare against:
|
|
@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. |
|
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. 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. |
|
@dafyddj The script looks great, this is really taking shape. Some comments:
|
|
@dafyddj I've started #181, with the plan to propagate using |
# [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)
|
@dafyddj Thanks for moving over to |
|
I'm not particularly attached to 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
|
@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? |
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.
The
Since the end-user is going to be editing the |
| git commit --quiet --all \ | ||
| --message 'feat: convert `template-formula` to `'"${NEW_NAME}"'-formula` | ||
|
|
||
| BREAKING CHANGE: changed all state names and ids' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
git commit --quiet --all \
--message 'feat: convert `template-formula` to `'"${NEW_NAME}"'-formula`' \
--message 'BREAKING CHANGE: changed all state names and ids'| 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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 scriptThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might get some clues by running as (if you haven't already done so):
DEBUG=true bin/convert-formula.sh ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dafyddj This sounds a little strange but I found a solution (using -u NONE) based on this reference:
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 errexitre-enabled. - Maybe the end user (like me) could have something strange in their configuration, affecting
exwhen it exits?
| git reset \ | ||
| "$(echo 'feat: initial commit' \ | ||
| | git commit-tree 'HEAD^{tree}')" | ||
| git rm --quiet bin/convert-formula.sh AUTHORS.md CHANGELOG.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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 |
|
Current status:
A wish-list collection for the long-run, doesn't prevent merging this PR:
|
|
@dafyddj Merged, the rest of the issues can be covered in a future PR. Thanks for this fantastic PR. |
|
🎉 This PR is included in version 4.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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)
## 1.0.0 (2025-08-26) ### ⚠ BREAKING CHANGES * **map:** `map.jinja` now exports a generic `mapdata` variable * **map:** The per grain parameter values are now under `TEMPLATE/parameters/` * changed all state names and ids * **pkgname:** the parameter `pkg` is now a dictionary. References to `template.pkg` should be changed to `template.pkg.name`. * **tofs:** every formula writer will need to change the import to use this new version. * template/libtofs.jinja: provides the “files_switch” macro. * docs/TOFS_pattern.rst: update documentation to use the new path. * template/config/clean.sls: change import from “macros.jinja” to “libtofs.jinja”. * template/config/file.sls: ditoo. * **states:** Wholesale state ID changes will break implementations that are relying on the previous state IDs for requisite purposes. * **pkg:** Changing the `pkg` directory to `package` will break implementations that are depending on `pkg` for `include` or `sls`-based requisite purposes. ### Features * add Gentoo support ([4c2f4ed](dafyddj/tmp-test-template-formula@4c2f4ed)) * add script to ease conversion from template to real formula ([edfa269](dafyddj/tmp-test-template-formula@edfa269)) * **authors:** update automatically alongside `semantic-release` ([8000098](dafyddj/tmp-test-template-formula@8000098)) * **centos-6:** reshape formula and tests for this platform ([a4b1608](dafyddj/tmp-test-template-formula@a4b1608)), closes [saltstack-formulas#104](https://github.com/dafyddj/tmp-test-template-formula/issues/104) * **convert-formula.sh:** assign `@NONE` as whole-formula owner ([cceffff](dafyddj/tmp-test-template-formula@cceffff)) * **kitchen+travis:** add `opensuse-leap` after resolving issues ([7614a3c](dafyddj/tmp-test-template-formula@7614a3c)) * **kitchen+travis:** conduct tests on a wider range of platforms ([1348078](dafyddj/tmp-test-template-formula@1348078)) * **m2r:** use `m2r` to convert automatic `.md` files to `.rst` ([b86ddf4](dafyddj/tmp-test-template-formula@b86ddf4)) * **macos:** basic package and group handling ([8c3fe22](dafyddj/tmp-test-template-formula@8c3fe22)) * **map:** generate a YAML file to validate `map.jinja` ([fc90075](dafyddj/tmp-test-template-formula@fc90075)) * **mapping:** introduce osarchmap per issue [saltstack-formulas#13](https://github.com/dafyddj/tmp-test-template-formula/issues/13) ([41ac40d](dafyddj/tmp-test-template-formula@41ac40d)) * **map:** update to v5 `map.jinja` ([42e1932](dafyddj/tmp-test-template-formula@42e1932)) * **pkg:** add `clean` states ([422c7ac](dafyddj/tmp-test-template-formula@422c7ac)) * **pkg:** use `require` requisite between `pkg` states ([6e7141b](dafyddj/tmp-test-template-formula@6e7141b)) * **rtd:** provide custom CSS file for overriding in-use Sphinx theme ([24bd338](dafyddj/tmp-test-template-formula@24bd338)) * run for real ([041e27b](dafyddj/tmp-test-template-formula@041e27b)) * **semantic-release:** configure for this formula ([cbcfd75](dafyddj/tmp-test-template-formula@cbcfd75)) * **sub-component:** manage a dedicated configuration file ([c4440d7](dafyddj/tmp-test-template-formula@c4440d7)) * **toc:** use `markdown-toc` directly to update inline ([a5bae1e](dafyddj/tmp-test-template-formula@a5bae1e)) * **tofs:** implement backwards-compatible TOFSv2 for configurability ([068a94d](dafyddj/tmp-test-template-formula@068a94d)) * **tofs:** lookup files directory in “tpldir” hierarchy ([5c495fb](dafyddj/tmp-test-template-formula@5c495fb)) * **workflows:** dry-run `semantic-release` in GitHub Actions ([764cd4c](dafyddj/tmp-test-template-formula@764cd4c)) * **yamllint:** include for this repo and apply rules throughout ([e76525f](dafyddj/tmp-test-template-formula@e76525f)) ### Bug Fixes * **_mapdata:** ensure map data is directly under `values` ([bcb8e29](dafyddj/tmp-test-template-formula@bcb8e29)) * **`config/file`:** add missing space before Jinja `}}` ([5cd08ab](dafyddj/tmp-test-template-formula@5cd08ab)) * **`libtofs`:** use `select` to deal with empty strings in path ([afe0751](dafyddj/tmp-test-template-formula@afe0751)) * **`libtofs`:** use `strip` to deal with leading/trailing slashes ([2563a46](dafyddj/tmp-test-template-formula@2563a46)) * **`map.jinja`:** _merge_ defaults and `config.get` ([91bc2f0](dafyddj/tmp-test-template-formula@91bc2f0)) * **`map.jinja`:** remove `merge` from `config.get` (for `salt-ssh`) ([00e474c](dafyddj/tmp-test-template-formula@00e474c)), closes [saltstack-formulas#95](https://github.com/dafyddj/tmp-test-template-formula/issues/95) * **`map.jinja`:** use tplroot ([b9c5e03](dafyddj/tmp-test-template-formula@b9c5e03)) * broken install-hooks due to saltlint v0.8.0 ([7da11c9](dafyddj/tmp-test-template-formula@7da11c9)) * **codeowners:** ensure `lib*` files are owned by `ssf` ([d60cc15](dafyddj/tmp-test-template-formula@d60cc15)) * **comments:** explain that at least an empty dict is required ([426f955](dafyddj/tmp-test-template-formula@426f955)), closes [saltstack-formulas#93](https://github.com/dafyddj/tmp-test-template-formula/issues/93) * **commitlint:** fix header length at 72 chars as agreed ([a95061d](dafyddj/tmp-test-template-formula@a95061d)) * **convert-formula.sh:** add -_ to allowed chars in formula name ([a999fee](dafyddj/tmp-test-template-formula@a999fee)) * **convert-formula.sh:** add `~` to reST underlining during conversion ([80ed8cd](dafyddj/tmp-test-template-formula@80ed8cd)) * **convert-formula.sh:** apply remaining suggestions from [saltstack-formulas#180](https://github.com/dafyddj/tmp-test-template-formula/issues/180) ([76ecd44](dafyddj/tmp-test-template-formula@76ecd44)) * **convert-formula.sh:** delete all existing tags ([7c33601](dafyddj/tmp-test-template-formula@7c33601)), closes [saltstack-formulas#210](https://github.com/dafyddj/tmp-test-template-formula/issues/210) * **convert-formula.sh:** fix reST underlining during conversion ([11068af](dafyddj/tmp-test-template-formula@11068af)) * **convert-formula.sh:** remove "Using this template" post-conversion ([55ab937](dafyddj/tmp-test-template-formula@55ab937)) * **convert-formula.sh:** remove `rubocop` override post-conversion ([aca4e44](dafyddj/tmp-test-template-formula@aca4e44)) * **convert-formula.sh:** remove CI test post-conversion ([06ec949](dafyddj/tmp-test-template-formula@06ec949)) * **convert-formula.sh:** replace instances of `template-formula` for CI ([537fe65](dafyddj/tmp-test-template-formula@537fe65)), closes [saltstack-formulas#231](https://github.com/dafyddj/tmp-test-template-formula/issues/231) * **convert-formula.sh:** reset version to `1.0.0` ([39889ce](dafyddj/tmp-test-template-formula@39889ce)) * **convert-formula.sh:** use portable sed function to make replacements ([41e10b5](dafyddj/tmp-test-template-formula@41e10b5)), closes [saltstack-formulas#192](https://github.com/dafyddj/tmp-test-template-formula/issues/192) * **convert-formula:** `_mapdata` control name must use the formula one ([1f3600d](dafyddj/tmp-test-template-formula@1f3600d)) * fix `CentOS Linux-7` and add `os` details from current CI setup ([4be16ca](dafyddj/tmp-test-template-formula@4be16ca)) * **formula:** update to current oldest supported version of Salt ([878eca1](dafyddj/tmp-test-template-formula@878eca1)) * **gitignore:** add Gemfile.lock to .gitignore ([87fa410](dafyddj/tmp-test-template-formula@87fa410)) * **grain:** fix grain value ([26edfa0](dafyddj/tmp-test-template-formula@26edfa0)) * **inspec:** validate `map.jinja` configuration ([41d222e](dafyddj/tmp-test-template-formula@41d222e)) * **libmapstack:** allow mapping by booleans and numbers ([bb3a7ea](dafyddj/tmp-test-template-formula@bb3a7ea)) * **libsaltcli+libmatchers:** ensure Salt client API detection [skip ci] ([6eb2073](dafyddj/tmp-test-template-formula@6eb2073)) * **libsaltcli:** update `salt-ssh` detection for `enable_ssh_minions` ([f0e7192](dafyddj/tmp-test-template-formula@f0e7192)) * **libtofs:** “files_switch” mess up the variable defined by “map.jinja” ([ab4ce75](dafyddj/tmp-test-template-formula@ab4ce75)) * **libtofs:** “files_switch” mess up the variable exported by “map.jinja” [skip ci] ([241646f](dafyddj/tmp-test-template-formula@241646f)) * **libtofs:** avoid using subpath by default ([c07471d](dafyddj/tmp-test-template-formula@c07471d)) * **libtofs:** don't crash if “tofs.files_switch” lookup a list ([0979d35](dafyddj/tmp-test-template-formula@0979d35)) * **pillar:** fix `os_family` typo ([3f89c12](dafyddj/tmp-test-template-formula@3f89c12)) * **release.config.js:** use full commit hash in commit link [skip ci] ([4ac8d92](dafyddj/tmp-test-template-formula@4ac8d92)) * **rubocop:** add fixes using `rubocop --safe-auto-correct` ([484ce24](dafyddj/tmp-test-template-formula@484ce24)) * **rubocop:** fix remaining errors manually ([9566b6f](dafyddj/tmp-test-template-formula@9566b6f)) * **running.sls:** use `watch` not `require` to ensure service restart ([3a1fc35](dafyddj/tmp-test-template-formula@3a1fc35)) * **subcomponent:** clean referencing wrong sls ([394808e](dafyddj/tmp-test-template-formula@394808e)) * **suse:** correct OS grain ([6aee580](dafyddj/tmp-test-template-formula@6aee580)) * **tofs:** prepend the config-based `source_files` to the default ([3483e76](dafyddj/tmp-test-template-formula@3483e76)), closes [saltstack-formulas#151](https://github.com/dafyddj/tmp-test-template-formula/issues/151) * **tofs:** update comments in `files_switch` macro for new method ([3fa3640](dafyddj/tmp-test-template-formula@3fa3640)) * **tofs:** update use of state ID in `config` and `pillar` ([3d9a24c](dafyddj/tmp-test-template-formula@3d9a24c)) * **tofs:** use `source_files` instead of `files` ([5110716](dafyddj/tmp-test-template-formula@5110716)) * **tofs:** use `tpldir` derivative `topdir` for pillar (config) paths ([5e9df00](dafyddj/tmp-test-template-formula@5e9df00)) * **travis:** don't install gems twice ([925d8e2](dafyddj/tmp-test-template-formula@925d8e2)) * **travis:** reinstate conversion test [skip ci] ([5d47fda](dafyddj/tmp-test-template-formula@5d47fda)) * **travis:** use version numbers in Gemfile to prevent failed builds ([35f7111](dafyddj/tmp-test-template-formula@35f7111)) * typo in the installation instructions ([306a0d9](dafyddj/tmp-test-template-formula@306a0d9)) * use correct repo ([ca96491](dafyddj/tmp-test-template-formula@ca96491)) ### Performance Improvements * **travis:** improve `salt-lint` invocation [skip ci] ([7a96cd7](dafyddj/tmp-test-template-formula@7a96cd7)) ### Reverts * **kitchen+travis:** disable `debian-8` due to `2019.2` bug ([e8f0f7e](dafyddj/tmp-test-template-formula@e8f0f7e)) * **kitchen+travis:** use `debian:jessie-backports` as `debian-8` ([dcd141a](dafyddj/tmp-test-template-formula@dcd141a)) ### Code Refactoring * improve reusability using an unique keyword TEMPLATE ([2e8ded6](dafyddj/tmp-test-template-formula@2e8ded6)) * **pkg:** change to `package` instead ([2cd82e5](dafyddj/tmp-test-template-formula@2cd82e5)) * **pkgname:** reserve 'pkg' as packaging dict ([c6ae81c](dafyddj/tmp-test-template-formula@c6ae81c)) * **states:** set state IDs based on a dependable structure ([6690ee6](dafyddj/tmp-test-template-formula@6690ee6)) * **tofs:** move “files_switch” macro to “libtofs.jinja” ([da7e692](dafyddj/tmp-test-template-formula@da7e692))
PR progress checklist (to be filled in by reviewers)
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 testsDoes 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
templateis 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
templateby using the tokentemplate__- 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
README(e.g.Available states).pillar.example.Testing checklist
state_top).Additional context