Improve TOFS implementation for increased configurability #28

myii · 2019-02-15T05:06:41Z

As discussed between comments:
- https://freenode.logbot.info/saltstack-formulas/20190214#c1995273
- https://freenode.logbot.info/saltstack-formulas/20190214#c1995487

I'll explain this inline and in further comments.

myii · 2019-02-15T05:07:45Z

template/config.sls


 include:
  - template.install

 template-config:
  file.managed:
    - name: {{ template.config }}
-    - source: {{ files_switch('template', ['example.tmpl'] }}
+    - source: {{ files_switch(['example.tmpl', 'example.tmpl.jinja']) }}


No requirement to provide the prefix here any more. Done just once in the files_switch macro.

myii · 2019-02-15T05:08:29Z

template/config.sls

    - mode: 644
    - user: root
    - group: root
+    - template: jinja


Using a jinja template in order to show the source of the output file for debugging purposes.

myii · 2019-02-15T05:10:32Z

template/files/default/example.tmpl

@@ -1,3 +1,6 @@
-# Managed by saltstack
+########################################################################
+# File managed by Salt at <{{ source }}>.


This will show the source of the file in the header of the managed file.

This header is used in other formulas such as the nfs-formula.

It's useful enough in normal cases, just to know where the source template resides. In the TOFS system, I believe this is indispensable, since there are so many potential templates a file could have used.

myii · 2019-02-15T05:11:06Z

template/files/default/example.tmpl.jinja

@@ -0,0 +1,6 @@
+########################################################################
+# File managed by Salt at <{{ source }}>.


Adding this file for further debugging. Using source again as before.

myii · 2019-02-15T05:11:25Z

template/macros.jinja

@@ -1,8 +1,7 @@
-{%- macro files_switch(prefix,


Removed the need for prefix.

@myii, I'm looking into this to incorporate at the systemd-formula,
but I use it here to distinguish multiple files:
systemd:networkd (has tofs)
systemd:timesyncd (has tofs)
but both should have a different prefix
otherwise the files will be in the same place....

myii · 2019-02-15T05:12:35Z

template/macros.jinja

                       default_files_switch=['id', 'os_family'],
                       indent_width=6) %}
-  {#
+  {#-


Without the -, it leaves an unnecessary blank line in the debug output:

- source: - salt://template/files/ABC/example.tmpl

myii · 2019-02-15T05:13:07Z

template/macros.jinja

-  {%- set path_prefix = prefix|replace(':', '/') %}
-  {%- set files_switch_list = salt['pillar.get'](prefix ~ ':files_switch',
-                                           default_files_switch) %}
+  {%- set formula = 'template' %}


Define the formula name once and use it multiple times below.

Isn't there some clever way to get this automatically? Something built in?

Just remembered: tpldir.

myii · 2019-02-15T05:13:57Z

template/macros.jinja

-  {%- set files_switch_list = salt['pillar.get'](prefix ~ ':files_switch',
-                                           default_files_switch) %}
+  {%- set formula = 'template' %}
+  {%- set path_prefix = salt['config.get'](formula ~ ':tofs:path_prefix', formula) %}


Using config.get to make the path_prefix configurable or use formula name as the default.

myii · 2019-02-15T05:14:19Z

template/macros.jinja

-                                           default_files_switch) %}
+  {%- set formula = 'template' %}
+  {%- set path_prefix = salt['config.get'](formula ~ ':tofs:path_prefix', formula) %}
+  {%- set files_switch_list = salt['config.get'](


Now with config.get instead of pillar.get.

myii · 2019-02-15T05:15:03Z

template/macros.jinja

-                                            file.lstrip('/')]) %}
+      {%- set url = '- salt://' ~ '/'.join([
+          path_prefix,
+          salt['config.get'](formula ~ ':tofs:dirs:files', 'files'),


Using config.get to make the 'files' directory configurable or use 'files' by default.

myii · 2019-02-15T05:15:27Z

template/macros.jinja

+    {%- set url = '- salt://' ~ '/'.join([
+        path_prefix,
+        salt['config.get'](formula ~ ':tofs:dirs:files', 'files'),
+        salt['config.get'](formula ~ ':tofs:dirs:default', 'default'),


Using config.get to make the 'default' directory configurable or use 'default' by default.

I find the '- salt://' weird. I would have done salt:// and add the dash in the loop, to have the 'url' really be an url, not an item in a YAML list

maybe the same for salt://' ~ '/', why not use salt:/// directly?

That particular section was just direct refactoring, didn't look at it in any detail.

So looking now, this is being used to populate the list under the source of file.managed, thus requiring the dash - prefixed.

myii · 2019-02-15T05:20:32Z

So by default, with no pillar, etc. set:

template-config:
  file.managed:
    - name: /etc/template.d/custom-ubuntu.conf
    - source: 
      - salt://template/files/ABC/example.tmpl
      - salt://template/files/ABC/example.tmpl.jinja
      - salt://template/files/Debian/example.tmpl
      - salt://template/files/Debian/example.tmpl.jinja
      - salt://template/files/default/example.tmpl
      - salt://template/files/default/example.tmpl.jinja
    - mode: 644
    - user: root
    - group: root
    - template: jinja

With the following pillar set:

template:
  tofs:
    path_prefix: template_alt
    dirs:
      files: files_alt
      default: default_alt
  files_switch:
    - id
    - osfinger
    - os
    - os_family

We get:

template-config:
  file.managed:
    - name: /etc/template.d/custom-ubuntu.conf
    - source:
      - salt://template_alt/files_alt/ABC/example.tmpl
      - salt://template_alt/files_alt/ABC/example.tmpl.jinja
      - salt://template_alt/files_alt/Ubuntu-16.04/example.tmpl
      - salt://template_alt/files_alt/Ubuntu-16.04/example.tmpl.jinja
      - salt://template_alt/files_alt/Ubuntu/example.tmpl
      - salt://template_alt/files_alt/Ubuntu/example.tmpl.jinja
      - salt://template_alt/files_alt/Debian/example.tmpl
      - salt://template_alt/files_alt/Debian/example.tmpl.jinja
      - salt://template_alt/files_alt/default_alt/example.tmpl
      - salt://template_alt/files_alt/default_alt/example.tmpl.jinja
    - mode: 644
    - user: root
    - group: root
    - template: jinja

myii · 2019-02-15T05:22:34Z

Looking at the pillar:

template:
  tofs:
    path_prefix: template_alt
    dirs:
      files: files_alt
      default: default_alt
  files_switch:
    - id
    - osfinger
    - os
    - os_family

May be advantageous to merge these under one key. Maybe something like:

template:
  tofs:
    files_switch:
      - id
      - osfinger
      - os
      - os_family
    path_prefix: template_alt
    dirs:
      files: files_alt
      default: default_alt

myii · 2019-02-15T05:38:14Z

Haven't done this yet. The final (really) crazy suggestion but for ultimate configurability:

    - source: {{ files_switch(['example.tmpl', 'example.tmpl.jinja']) }}

Make the file list configurable via. config.get as well(!)

Meaning something like:

    - source: {{ files_switch(
                    salt['config.get'](
                        formula ~ ':tofs:files:template-config',
                        ['example.tmpl', 'example.tmpl.jinja']
                    )
              ) }}

So that's <formula>:tofs:files:<state-id>.
We'd need to define formula at a higher level so that it could be used both here and in the files_switch macro.

With an example pillar:

template:
  tofs:
    ...
    files:
      template-config:
        - 'example_alt.tmpl'
        - 'example_alt.tmpl.jinja'

Actually, just tried it and it worked. So I'll push it here as a second commit so that you can try it for yourselves.

myii · 2019-02-15T05:42:02Z

So with the second commit and the additions to the pillar as mentioned in the previous comment, we get:

    - source:
      - salt://template_alt/files_alt/ABC/example_alt.tmpl
      - salt://template_alt/files_alt/ABC/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/Ubuntu-16.04/example_alt.tmpl
      - salt://template_alt/files_alt/Ubuntu-16.04/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/Ubuntu/example_alt.tmpl
      - salt://template_alt/files_alt/Ubuntu/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/Debian/example_alt.tmpl
      - salt://template_alt/files_alt/Debian/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/default_alt/example_alt.tmpl
      - salt://template_alt/files_alt/default_alt/example_alt.tmpl.jinja
    - mode: 644
    - user: root
    - group: root
    - template: jinja

myii · 2019-02-15T05:43:22Z

template/config.sls

@@ -3,14 +3,20 @@

 {%- from "template/map.jinja" import template with context %}
 {%- from "template/macros.jinja" import files_switch with context %}
+{%- set formula = 'template' %}


This is ugly but just for testing in this WIP.

Just remembered: tpldir.

myii · 2019-02-15T05:44:33Z

Obviously, this is only a WIP. If going forwards with any of the changes, the documentation and pillar.example would also need to be updated.

daks · 2019-02-15T08:38:22Z

May be advantageous to merge these under one key.

I think merging all keys under the tofs key makes more sense

myii · 2019-02-15T08:47:28Z

May be advantageous to merge these under one key.

I think merging all keys under the tofs key makes more sense

@daks I'm being cautious here because the TOFS pattern is already being used in other formulas. So this change will cause blowback there. Leaving it open for discussion for now even though I'd prefer if it could be merged going forward.

myii · 2019-02-15T13:22:40Z

@baby-gnu Based on your comment, added commit dfbdb3e, which has allowed the files_switch macro to be simplified to use just the one loop rather than two. Removes more duplication.

myii · 2019-02-15T13:31:26Z

template/macros.jinja

-                                            grains.get(grain),
-                                            file.lstrip('/')]) %}
+      {%- if fs %}
+        {%- set fs_dir = salt['config.get'](fs, fs) %}


If the "grain" isn't found, it uses it as a literal value in the path instead. This has two noticeable benefits:

Allows for additional path matching from the pillar.

Easier to debug when not working, since the unmatched value will be shown directly in the URL.

myii · 2019-02-15T14:28:46Z

@baby-gnu Using tpldir makes the files_switch macro fully-portable -- drop-in to any formula without any modifications required. So the last remaining task is to decide about the pillar layout as mentioned above. While it will be a hassle in the existing formulas (zabbix, systemd, etc.), going forward I believe it is more appropriate to merge it all under one key, as mentioned in my comment above -- @daks concurred with that suggestion.

myii · 2019-02-15T14:56:35Z

Example of dfbdb3e in action, in regards to using any custom path (any/path/can/be/used/here in the example below). With the pillar:

template:
  lookup:
    master: template-master
  tofs:
    ...
  files_switch:
    - any/path/can/be/used/here
    - id
    - osfinger
    - os
    - os_family

Results in:

template-config:
  file.managed:
    - name: /etc/template.d/custom-ubuntu.conf
    - source:
      - salt://template/files/any/path/can/be/used/here/example.tmpl
      - salt://template/files/any/path/can/be/used/here/example.tmpl.jinja
      - salt://template/files/ABC/example.tmpl
      - salt://template/files/ABC/example.tmpl.jinja
      - salt://template/files/Ubuntu-16.04/example.tmpl
      - salt://template/files/Ubuntu-16.04/example.tmpl.jinja
      - salt://template/files/Ubuntu/example.tmpl
      - salt://template/files/Ubuntu/example.tmpl.jinja
      - salt://template/files/Debian/example.tmpl
      - salt://template/files/Debian/example.tmpl.jinja
      - salt://template/files/default/example.tmpl
      - salt://template/files/default/example.tmpl.jinja
    - mode: 644
    - user: root
    - group: root
    - template: jinja

myii · 2019-02-15T15:32:28Z

Pushed two more commits re: pillar.example.

The first commit adds the whole tofs key alongside files_switch from before.
The second commits shows the proposal of files_switch being demoted into the tofs key, including the minor change to macros.jinja.

myii · 2019-02-15T23:18:16Z

Added 7ea0f5a to fix the bug reported in the discussion from here.

For each additional state in the sls, additional default lines were rendered before this fix:

template-config:
  file.managed:
    ...
    - source:
      - salt://template/files/ABC/example.tmpl
      - salt://template/files/ABC/example.tmpl.jinja
      - salt://template/files/Debian/example.tmpl
      - salt://template/files/Debian/example.tmpl.jinja
      - salt://template/files/default/example.tmpl
      - salt://template/files/default/example.tmpl.jinja
    ...

formula.autofs.config.master:
  file.managed:
    ...
    - source:
      - salt://template/files/ABC/auto.master
      - salt://template/files/Debian/auto.master
      - salt://template/files/default/auto.master
      - salt://template/files/default/auto.master
    ...

formula.autofs.config.home:
  file.managed:
    ...
    - source:
      - salt://template/files/ABC/auto.homeLdap
      - salt://template/files/Debian/auto.homeLdap
      - salt://template/files/default/auto.homeLdap
      - salt://template/files/default/auto.homeLdap
      - salt://template/files/default/auto.homeLdap
    ...

formula.autofs.config.away:
  file.managed:
    ...
    - source:
      - salt://template/files/ABC/auto.awayLdap
      - salt://template/files/Debian/auto.awayLdap
      - salt://template/files/default/auto.awayLdap
      - salt://template/files/default/auto.awayLdap
      - salt://template/files/default/auto.awayLdap
      - salt://template/files/default/auto.awayLdap
    ...

aboe76 · 2019-02-18T17:31:44Z

I think this needs a rebase..

myii · 2019-02-18T17:34:17Z

@aboe76 Rebasing is not a problem. We still need to finalise what we want to do about the remaining tasks, particularly:

Pillar structure resolved (discussion required) -- see comment below.

PR is based on the proposed pillar.example scheme.

@baby-gnu What are your thoughts about this?

myii · 2019-02-18T19:22:37Z

OK, consensus on using template:tofs:files_switch instead of template:files_switch. Some refs:

* As discussed between comments: - https://freenode.logbot.info/saltstack-formulas/20190214#c1995273 - https://freenode.logbot.info/saltstack-formulas/20190214#c1995487 * Squashed original PR with these commits present: - Improve TOFS implementation for increased configurability - Improve TOFS to allow custom files selection as well - Simplify `files_switch` macro to use only one loop - Make TOFS pattern fully-portable by using `tpldir` - Update `pillar.example` with current scheme for TOFS - Update `pillar.example` with the proposed scheme for TOFS - Fix bug where duplicate default lines are produced by the loop - feat(tofs): fix to work with old & new pillars and `tpldir` context * Already tested and applied over existing TOFS in `systemd-formula`: - saltstack-formulas/systemd-formula#17

myii · 2019-02-23T23:34:12Z

After resolving saltstack-formulas/systemd-formula#17, transferred the changes back to this PR and now ready to go.

There is one stage left that can be covered by a future PR:

Update all documentation and inline comments as required.

aboe76 · 2019-02-23T23:41:02Z

merged it!

myii · 2019-02-23T23:41:21Z

@aboe76 Thanks!

saltstack-formulas-travis · 2019-02-23T23:52:25Z

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

myii added enhancement bump:minor WIP labels Feb 15, 2019

myii requested review from baby-gnu and aboe76 February 15, 2019 05:06

myii commented Feb 15, 2019

View reviewed changes

myii requested a review from javierbertoli February 15, 2019 05:45

myii mentioned this pull request Feb 15, 2019

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

Open

71 tasks

myii commented Feb 15, 2019

View reviewed changes

myii self-assigned this Feb 15, 2019

myii force-pushed the PR_improve-TOFS branch from 1270a51 to 5afb5c0 Compare February 15, 2019 15:31

myii requested a review from vutny February 15, 2019 16:46

myii mentioned this pull request Feb 15, 2019

feat(tofs): allow TOFS for master+minion configuration saltstack-formulas/salt-formula#398

Merged

aboe76 approved these changes Feb 18, 2019

View reviewed changes

myii mentioned this pull request Feb 23, 2019

feat(kitchen+travis): conduct tests on a wider range of platforms #38

Merged

myii force-pushed the PR_improve-TOFS branch from 1063ba8 to 068a94d Compare February 23, 2019 23:30

myii changed the title ~~WIP: Improve TOFS implementation for increased configurability~~ Improve TOFS implementation for increased configurability Feb 23, 2019

myii removed the WIP label Feb 23, 2019

myii mentioned this pull request Feb 23, 2019

TOFSv2: Update all documentation and inline comments as required #44

Closed

aboe76 merged commit 249d6a7 into saltstack-formulas:master Feb 23, 2019

myii deleted the PR_improve-TOFS branch February 23, 2019 23:41

saltstack-formulas-travis added the released label Feb 23, 2019

		@@ -0,0 +1,6 @@
		########################################################################
		# File managed by Salt at <{{ source }}>.

Improve TOFS implementation for increased configurability #28

Improve TOFS implementation for increased configurability #28

Conversation

myii commented Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daks Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myii commented Feb 15, 2019

myii commented Feb 15, 2019

myii commented Feb 15, 2019

myii commented Feb 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myii commented Feb 15, 2019

daks commented Feb 15, 2019 • edited Loading

myii commented Feb 15, 2019

myii commented Feb 15, 2019

Choose a reason for hiding this comment

myii commented Feb 15, 2019

myii commented Feb 15, 2019

myii commented Feb 15, 2019

myii commented Feb 15, 2019 • edited Loading

aboe76 commented Feb 18, 2019

myii commented Feb 18, 2019 • edited by aboe76 Loading

myii commented Feb 18, 2019

myii commented Feb 23, 2019

aboe76 commented Feb 23, 2019

myii commented Feb 23, 2019

saltstack-formulas-travis commented Feb 23, 2019

myii commented Feb 15, 2019 •

edited

Loading

daks Feb 15, 2019 •

edited

Loading

daks commented Feb 15, 2019 •

edited

Loading

myii commented Feb 15, 2019 •

edited

Loading

myii commented Feb 18, 2019 •

edited by aboe76

Loading