feature(tofs): lookup files directory in “tpldir” hierarchy #37

baby-gnu · 2019-05-20T11:25:05Z

Without user configuration, the “libtofs.jinja” lookup the “files”
directory under “tplroot” with is the base directory of the current
formula unless the “v1_path_prefix” is set.

Now, we lookup the directory in each sub-directory from the
sub-directory of the current SLS (“tpldir”) to the top level directory
of the formula (“tplroot”).

This permit to support natively the multi-components formula.

baby-gnu · 2019-05-20T11:34:32Z

This result in the following

       networkd:
         file.recurse:
           - name: /etc/systemd/network
           - user: root
           - group: root
           - template: jinja
           - source: 
             - salt://systemd/networkd/files/any/path/can/be/used/here/network
             - salt://systemd/networkd/files/salt-formula.ci.local/network
             - salt://systemd/networkd/files/Ubuntu-18.04/network
             - salt://systemd/networkd/files/Ubuntu/network
             - salt://systemd/networkd/files/Debian/network
             - salt://systemd/networkd/files/default/network
             - salt://systemd/files/any/path/can/be/used/here/network
             - salt://systemd/files/salt-formula.ci.local/network
             - salt://systemd/files/Ubuntu-18.04/network
             - salt://systemd/files/Ubuntu/network
             - salt://systemd/files/Debian/network
             - salt://systemd/files/default/network
           - clean: True
           - dir_mode: 755
           - file_mode: 644
           - include_empty: True
           - listen_in:
             - service: networkd

Without user configuration, the “libtofs.jinja” lookup the “files” directory under “tplroot” with is the base directory of the current formula unless the “v1_path_prefix” is set. Now, we lookup the directory in each sub-directory from the sub-directory of the current SLS (“tpldir”) to the top level directory of the formula (“tplroot”). This permit to support natively the multi-components formula.

aboe76 · 2019-05-20T20:43:50Z

@baby-gnu shall I merge first the other PR, before this one?

baby-gnu · 2019-05-21T07:57:18Z

@aboe76 if I understood correctly, @myii asked to do this one first.

myii · 2019-05-21T13:52:51Z

@baby-gnu Sorry for the delay, I'll be checking this PR shortly. @aboe76 As laid out here, the plan is to benefit from the improvement in this formula and then "backport" it to the template-formula.

myii

Excellent PR, approved really. The requested changes are only aesthetic.

myii · 2019-05-21T15:24:38Z

docs/TOFS_pattern.rst

+
+For example, the following ``formula.component.config`` SLS:
+
+.. code-block::


Suggested change

.. code-block::

.. code-block:: sls

myii · 2019-05-21T15:25:17Z

docs/TOFS_pattern.rst

+
+.. code-block::
+
+   {%- from "formula/libtofs.jinja" import files_switch with context -%}


Suggested change

{%- from "formula/libtofs.jinja" import files_switch with context -%}

{%- from "formula/libtofs.jinja" import files_switch with context %}

myii · 2019-05-21T15:26:29Z

docs/TOFS_pattern.rst

+                    )
+                 }}
+
+will be rendered on a ``Debian`` named ``salt-formula.ci.local`` as:


Suggested change

will be rendered on a ``Debian`` named ``salt-formula.ci.local`` as:

will be rendered on a ``Debian`` minion named ``salt-formula.ci.local`` as:

myii · 2019-05-21T15:26:33Z

docs/TOFS_pattern.rst

+
+will be rendered on a ``Debian`` named ``salt-formula.ci.local`` as:
+
+.. code-block::


Suggested change

.. code-block::

.. code-block:: sls

myii · 2019-05-21T15:26:38Z

docs/TOFS_pattern.rst

+.. code-block::
+
+   formula configuration file:
+


Suggested change

myii · 2019-05-21T15:28:52Z

@baby-gnu One further question: what about removing the if block for v1_path_prefix? It's no longer used in the formula, or anywhere else actually.

To be clear, I mean removing v1_path_prefix completely.

myii · 2019-05-21T16:31:17Z

@baby-gnu @aboe76 Another issue that we need to finalise. When this functionality was first produced in saltstack-formulas/template-formula#86, it produced the source list in the current order, e.g.:

    - source:
      - salt://systemd/files/ABC/network
      - salt://systemd/files/Debian/network
      - salt://systemd/files/default/network
      - salt://systemd/networkd/files/ABC/network
      - salt://systemd/networkd/files/Debian/network
      - salt://systemd/networkd/files/default/network

After my saltstack-formulas/template-formula#86 (comment), @baby-gnu reversed the order as requested, to now produce:

    - source:
      - salt://systemd/networkd/files/ABC/network
      - salt://systemd/networkd/files/Debian/network
      - salt://systemd/networkd/files/default/network
      - salt://systemd/files/ABC/network
      - salt://systemd/files/Debian/network
      - salt://systemd/files/default/network

In the same PR, @daks raised concerns about I/O costs (saltstack-formulas/template-formula#86 (comment)).

So my question in, which order is preferable:

sub-component => main component (using |reverse)
main component => sub-component

While number 1 makes sense, in reality those sub-component lookups would be wasted in every formula that doesn't use sub-components (i.e. all of them so far). Another option is to leave this libtofs.jinja customised, so that it works better for systemd-formula. What do you all think?

aboe76 · 2019-05-21T18:16:48Z

@myii I think if you document comment the changes necessary to switch between:

sub-component => main component (using |reverse)
main component => sub-component

and let the maintainer who implements tofs be the just, if one or the other is necessary.
libtofs.jinja is already a io burden for the sake of flexibility. Having a optimized tofs will be important to other users...

baby-gnu · 2019-05-24T13:34:07Z

@baby-gnu One further question: what about removing the if block for v1_path_prefix? It's no longer used in the formula, or anywhere else actually.

To be clear, I mean removing v1_path_prefix completely.

It can be done but this is a breaking change.

For example, I personnaly always “hook” my personal SLS under the formula namespace (and I think it's the good thing to do), like

gitfs_remotes:
  - https://myserver/my-systemd-config.git
  - https://myserver/systemd-formula.git

my-systemd-config has the same directory tree as systemd-formula (containing only my own files) , so my own repository comes first when salt try to lookup a file under salt://systemd/.

My SLS import looks like this:

{%- from "systemd/map.jinja" import systemd with context %}
{%- from "systemd/libtofs.jinja" import files_switch with context %}

If another user does the same thing as me, she will have troubles if she used the v1_path_prefix in her own SLS.

If everybody agree for this breaking changes, I'll remove it.

Regards.

baby-gnu · 2019-05-24T13:50:47Z

While number 1 makes sense, in reality those sub-component lookups would be wasted in every formula that doesn't use sub-components (i.e. all of them so far). Another option is to leave this libtofs.jinja customised, so that it works better for systemd-formula.

I agree to avoid the automatic sub-component lookup to avoid unnecessary I/O and let it configurable, i.e. close this pull request without merge, but the v1_path_prefix could be better renamed component or just path_prefix

timesyncd:
  file.managed:
    - name: /etc/systemd/timesyncd.conf
    - user: root
    - group: root
    - mode: 644
    - template: jinja
    - source: {{ files_switch(['timesyncd.conf'],
                              lookup='timesyncd',
                              component='timesyncd'
                              )
              }}

The libtofs.jinja should be modified accordingly (and using |join() instead of string concatenation).

myii · 2019-05-24T18:08:48Z

@baby-gnu As for this specific PR, I believe it is an important and excellent contribution. Let's leave v1_path_prefix for the time being and then deprecate it at a later date. The ordering is fine as well (for this formula), so I'd like to go ahead and merge (after your comments about the small changes I mentioned in my review).

myii · 2019-05-26T16:44:57Z

@baby-gnu Merged, let's get this important contribution out there. I'll suggest the changes above in a separate PR. Thanks for this!

saltstack-formulas-travis · 2019-05-26T16:52:34Z

🎉 This PR is included in version 0.12.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

docs(tofs): apply remaining comments from PR #37

## [0.12.1](v0.12.0...v0.12.1) (2019-05-27) ### Documentation * **tofs:** apply remaining comments from PR [#37](#37) ([d665676](d665676))

myii · 2019-05-28T20:20:41Z

@baby-gnu It's a good thing we didn't remove v1_path_prefix -- this was needed when we've been looking at using TOFS in ng formulas! @sticky-note is looking at using it in php-formula now.

baby-gnu force-pushed the feature/tofs-lookup-files-in-tpldir-hierarchy branch from 68762f5 to 8d5b5ea Compare May 20, 2019 11:41

baby-gnu mentioned this pull request May 20, 2019

Feature/tofs lookup files in tpldir hierarchy saltstack-formulas/template-formula#86

Merged

aboe76 requested a review from myii May 20, 2019 20:48

myii requested changes May 21, 2019

View reviewed changes

myii merged commit ba08792 into saltstack-formulas:master May 26, 2019

myii added a commit to myii/systemd-formula that referenced this pull request May 26, 2019

docs(tofs): apply remaining comments from PR saltstack-formulas#37

d665676

saltstack-formulas-travis added the released label May 26, 2019

myii mentioned this pull request May 26, 2019

docs(tofs): apply remaining comments from PR #37 #38

Merged

aboe76 added a commit that referenced this pull request May 27, 2019

Merge pull request #38 from myii/docs/update-TOFS

fc56ca4

docs(tofs): apply remaining comments from PR #37

saltstack-formulas-travis pushed a commit that referenced this pull request May 27, 2019

chore(release): 0.12.1 [skip ci]

3879d07

## [0.12.1](v0.12.0...v0.12.1) (2019-05-27) ### Documentation * **tofs:** apply remaining comments from PR [#37](#37) ([d665676](d665676))

myii mentioned this pull request May 28, 2019

Improve TOFS documentation based on recent queries, PRs, etc. saltstack-formulas/template-formula#128

Open

4 tasks

baby-gnu deleted the feature/tofs-lookup-files-in-tpldir-hierarchy branch June 13, 2019 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(tofs): lookup files directory in “tpldir” hierarchy #37

feature(tofs): lookup files directory in “tpldir” hierarchy #37

baby-gnu commented May 20, 2019

baby-gnu commented May 20, 2019

aboe76 commented May 20, 2019

baby-gnu commented May 21, 2019

myii commented May 21, 2019

myii left a comment

myii May 21, 2019

myii May 21, 2019

myii May 21, 2019

myii May 21, 2019

myii May 21, 2019

myii commented May 21, 2019 •

edited

Loading

myii commented May 21, 2019

aboe76 commented May 21, 2019

baby-gnu commented May 24, 2019

baby-gnu commented May 24, 2019

myii commented May 24, 2019

myii commented May 26, 2019

saltstack-formulas-travis commented May 26, 2019

myii commented May 28, 2019


		For example, the following ``formula.component.config`` SLS:

		.. code-block::


		.. code-block::

		{%- from "formula/libtofs.jinja" import files_switch with context -%}

	will be rendered on a ``Debian`` named ``salt-formula.ci.local`` as:
	will be rendered on a ``Debian`` minion named ``salt-formula.ci.local`` as:

feature(tofs): lookup files directory in “tpldir” hierarchy #37

feature(tofs): lookup files directory in “tpldir” hierarchy #37

Conversation

baby-gnu commented May 20, 2019

baby-gnu commented May 20, 2019

aboe76 commented May 20, 2019

baby-gnu commented May 21, 2019

myii commented May 21, 2019

myii left a comment

Choose a reason for hiding this comment

myii May 21, 2019

Choose a reason for hiding this comment

myii May 21, 2019

Choose a reason for hiding this comment

myii May 21, 2019

Choose a reason for hiding this comment

myii May 21, 2019

Choose a reason for hiding this comment

myii May 21, 2019

Choose a reason for hiding this comment

myii commented May 21, 2019 • edited Loading

myii commented May 21, 2019

aboe76 commented May 21, 2019

baby-gnu commented May 24, 2019

baby-gnu commented May 24, 2019

myii commented May 24, 2019

myii commented May 26, 2019

saltstack-formulas-travis commented May 26, 2019

myii commented May 28, 2019

myii commented May 21, 2019 •

edited

Loading