refactor(tofs): avoid using “salt['config.get']” for formula writers #77

baby-gnu · 2019-03-11T14:19:59Z

We can hide the call to “salt['config.get']” in the macro by only
asking for a namespace where to lookup the “source_files”.

docs/TOFS_pattern.rst (Example): document the use of the new
“namespace” parameter.
template/macros.jinja: add a new “namespace” parameter.
Lookup files override under
“:tofs:sources_files:” and fallback to
“source_files” parameter.
template/config/file.sls (template-config-file-file-managed): use
the new “namespace” parameter

baby-gnu · 2019-03-11T15:11:53Z

Hello.

I think I can rewrite the commit message since it actually breaks things. Maybe there is a better way of doing it, it's open to suggestion.

Regards

myii · 2019-03-11T17:40:59Z

@baby-gnu Just had a brief glance -- this looks like a very good idea. What break things -- just the commit message or something else?

template/macros.jinja

myii · 2019-03-12T02:18:03Z

@baby-gnu I've had a chance to look at this and test it out and I definitely agree that this is the right way to go forwards -- thanks for the contribution! However, I'd just like a couple of things cleaned up, as I've mentioned in my comments above. I also want to ask @aboe76 to have a look, so that we can be confident that we're heading in the right direction.

I've tested before and after with the following in my pillar (includes some extra test states for files_switch):

  tofs:
    files_switch:
      - any/path/can/be/used/here
      - id
      - osfinger
      - os
      - os_family
    path_prefix: template_alt
    dirs:
      files: files_alt
      default: default_alt
    source_files:
      template-config-file-file-managed:
        - 'example_alt_source.tmpl'
        - 'example_alt.tmpl.jinja'
      formula.autofs.config.master:
        - 'auto_alt.master'
      formula.autofs.config.home:
        - 'auto_alt.homeLdap'
      formula.autofs.config.away:
        - 'auto_alt.awayLdap'

I've got the same results before and after:

    - source:
      - salt://template_alt/files_alt/any/path/can/be/used/here/example_alt_source.tmpl
      - salt://template_alt/files_alt/any/path/can/be/used/here/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/ABC/example_alt_source.tmpl
      - salt://template_alt/files_alt/ABC/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/Ubuntu-16.04/example_alt_source.tmpl
      - salt://template_alt/files_alt/Ubuntu-16.04/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/Ubuntu/example_alt_source.tmpl
      - salt://template_alt/files_alt/Ubuntu/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/Debian/example_alt_source.tmpl
      - salt://template_alt/files_alt/Debian/example_alt.tmpl.jinja
      - salt://template_alt/files_alt/default_alt/example_alt_source.tmpl
      - salt://template_alt/files_alt/default_alt/example_alt.tmpl.jinja

    - source:
      - salt://template_alt/files_alt/any/path/can/be/used/here/auto_alt.master
      - salt://template_alt/files_alt/ABC/auto_alt.master
      - salt://template_alt/files_alt/Ubuntu-16.04/auto_alt.master
      - salt://template_alt/files_alt/Ubuntu/auto_alt.master
      - salt://template_alt/files_alt/Debian/auto_alt.master
      - salt://template_alt/files_alt/default_alt/auto_alt.master

    - source:
      - salt://template_alt/files_alt/any/path/can/be/used/here/auto_alt.homeLdap
      - salt://template_alt/files_alt/ABC/auto_alt.homeLdap
      - salt://template_alt/files_alt/Ubuntu-16.04/auto_alt.homeLdap
      - salt://template_alt/files_alt/Ubuntu/auto_alt.homeLdap
      - salt://template_alt/files_alt/Debian/auto_alt.homeLdap
      - salt://template_alt/files_alt/default_alt/auto_alt.homeLdap

    - source:
      - salt://template_alt/files_alt/any/path/can/be/used/here/auto_alt.awayLdap
      - salt://template_alt/files_alt/ABC/auto_alt.awayLdap
      - salt://template_alt/files_alt/Ubuntu-16.04/auto_alt.awayLdap
      - salt://template_alt/files_alt/Ubuntu/auto_alt.awayLdap
      - salt://template_alt/files_alt/Debian/auto_alt.awayLdap
      - salt://template_alt/files_alt/default_alt/auto_alt.awayLdap

So I'm happy with this implementation.

myii

Two changes:

{#-
Avoid using files, which can be mixed up with the files in tofs:dir:files.
1. I've made the suggestions inline to use src_file and src_files for these variables.

Another change to be discussed:

This comment about whether it is worth keeping namespace= and source_files= or not.

You could also fix the typo in the commit message:

forumla => formula.

myii · 2019-03-12T03:05:00Z

template/config/file.sls

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


Using namespace= and source_files= seems to make sense at this stage. However, I think it just makes this longer than in needs to be. Not pushing this issue, just opening it for discussion.

Maybe lookup= is much more meanful than namespace=.

baby-gnu · 2019-03-12T10:28:25Z

@baby-gnu Just had a brief glance -- this looks like a very good idea. What break things -- just the commit message or something else?

The files_switch API is broken, I introduced a new first parameter.

To make it compatible with users calling directly salt['config.get'] we should invert the two parameters like:

{%- macro files_switch(source_files,
                       namespace,
                       default_files_switch=['id', 'os_family'],
                       indent_width=6) %}

This way it can be used as

    - source: {{ files_switch(
                    salt['config.get'](
                        tplroot ~ ':tofs:source_files:template-config-file-file-managed',
                        ['example.tmpl', 'example.tmpl.jinja']
                    )
              ) }}

or

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

It could even be missused (doing the lookup two times) with:

    - source: {{ files_switch(
                    salt['config.get'](
                        tplroot ~ ':tofs:source_files:template-config-file-file-managed',
                        ['example.tmpl', 'example.tmpl.jinja']
                    ),
                    namespace='template-config-file-file-managed',

              ) }}

So, feature or refactor? ;-)

baby-gnu · 2019-03-13T13:19:34Z

I rebased my work, integrating the different comments made.

The default-debian-8-backports test fail seems not related to my work.

I changed the new variable name, position and optionality.

People using the old format are not forced to change, they can still call salt['config.get'] by themself:

    - source: {{ files_switch(
                    salt['config.get'](
                        tplroot ~ ':tofs:source_files:template-config-file-file-managed',
                        ['example.tmpl', 'example.tmpl.jinja']
                    )
                 )
              }}

or they can change and use the new optional lookup argument:

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

There is no error if the formula writer mistakely added the lookup argument without changing the list of files:

    - source: {{ files_switch(
                    salt['config.get'](
                        tplroot ~ ':tofs:source_files:template-config-file-file-managed',
                        ['example.tmpl', 'example.tmpl.jinja']
                    ),
                    lookup='template-config-file-file-managed'
                 )
              }}

Regards.

myii · 2019-03-13T20:54:58Z

Restarted the failing tests and all OK from that end.

template/macros.jinja

myii · 2019-03-13T23:47:41Z

@baby-gnu I think this updated method is definitely an improvement -- thanks for the extra effort you've put in. Please consider my new comments and let's wait for @aboe76 to review before merging.

We can hide the call to “salt['config.get']” in the macro by only asking for a “lookup key” where to lookup the list of “source_files”. * docs/TOFS_pattern.rst (Example): document the use of the new optional “lookup” parameter. * template/macros.jinja: add a new optional “lookup” parameter. Lookup “files override” under the “<tplroot>:tofs:sources_files:<lookup key>” and fallback to “source_files” parameter. * template/config/file.sls (template-config-file-file-managed): use the new “lookup” parameter.

myii · 2019-03-14T09:38:17Z

@baby-gnu I'm really happy with this now -- an excellent refactor. Thanks for your work and your patience. Would you be willing to submit a follow-up PR to update docs/TOFS_pattern.rst?

@aboe76 I'm happy with this to be merged, if you are OK with the changes as well.

baby-gnu · 2019-03-14T09:42:11Z

Would you be willing to submit a follow-up PR to update docs/TOFS_pattern.rst?

I should have missed something @myii because I updated the example in docs/TOFS_pattern.rst.

myii · 2019-03-14T10:00:01Z

@baby-gnu You're right, it has been updated! I didn't look at the whole file, so I thought there may be more changes needed.

aboe76 · 2019-03-14T19:58:59Z

@baby-gnu and @myii, you are both right, this looks like an improvement and I like the lookup part, this is a better pattern to use. LGTM

myii · 2019-03-15T08:31:35Z

UPDATE: Please leave this table for the time being, until the libtofs.jinja idea is finalised. The table can then be reproduced in that PR when it is submitted.

@baby-gnu As discussed on IRC/Matrix, list of formulas to be updated:

Formula	Assignees	Status	Done
systemd-formula	@baby-gnu	Small update	-
lighttpd-formula		Small update	-
stack-formula	@myii	Small update	-
zabbix-formula		From v1	-
varnish-formula		From v1	-

@babilen5 Sorry for the false ping, somehow got you instead of @baby-gnu!

alxwr · 2019-03-19T09:01:28Z

So macros.jinja now becomes a library which can be just dropped inside any formula? :-) I'm all for that. Maybe we want a libtofs.jinja? I really like the idea of being able to update standard functionality by copy-pasting the macro files I need into specific formulas. Copy-paste keeps the workload low while avoiding inter-formula dependencies.

baby-gnu · 2019-03-19T09:33:33Z

Hello.

So macros.jinja now becomes a library which can be just dropped inside any formula? :-) I'm all for that. Maybe we want a libtofs.jinja?

I like the idea, but for systemd-formula it looks like there are some specific things to lookup files under the sub-directories for each component.

I did not took enough time to look at it but I think a libtofs.jinja is a great idea since it meaningful.

Regards.

myii · 2019-03-21T04:16:52Z

@alxwr I really like the idea of a libtofs.jinja, keeping macros in separate files for ease of reuse. @baby-gnu Your concerns for the systemd-formula are well-founded -- I've got some progress to share, so I'm going to merge this PR in the meantime. I'll put something up and ping you all soon.

To all, thanks for your work, reviews and ideas.

* saltstack-formulas/template-formula#77

saltstack-formulas-travis · 2019-03-22T21:15:21Z

🎉 This PR is included in version 1.2.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

baby-gnu force-pushed the feature/simplify-files_switch-call branch from 041d261 to 8003492 Compare March 11, 2019 14:36

myii changed the title ~~refactor(tofs): avoid using “salt['config.get']” for forumla writers~~ refactor(tofs): avoid using “salt['config.get']” for formula writers Mar 12, 2019

myii reviewed Mar 12, 2019

View reviewed changes

template/macros.jinja Outdated Show resolved Hide resolved

myii reviewed Mar 12, 2019

View reviewed changes

template/macros.jinja Outdated Show resolved Hide resolved

myii reviewed Mar 12, 2019

View reviewed changes

template/macros.jinja Outdated Show resolved Hide resolved

myii requested changes Mar 12, 2019

View reviewed changes

myii requested a review from aboe76 March 12, 2019 02:19

myii reviewed Mar 12, 2019

View reviewed changes

baby-gnu force-pushed the feature/simplify-files_switch-call branch 3 times, most recently from d8292e2 to 62d1d24 Compare March 13, 2019 13:02

myii reviewed Mar 13, 2019

View reviewed changes

template/macros.jinja Show resolved Hide resolved

myii reviewed Mar 13, 2019

View reviewed changes

template/macros.jinja Outdated Show resolved Hide resolved

myii reviewed Mar 13, 2019

View reviewed changes

template/macros.jinja Outdated Show resolved Hide resolved

baby-gnu force-pushed the feature/simplify-files_switch-call branch from 62d1d24 to 60d43e7 Compare March 14, 2019 09:14

myii approved these changes Mar 14, 2019

View reviewed changes

aboe76 approved these changes Mar 14, 2019

View reviewed changes

myii merged commit 6d2140f into saltstack-formulas:master Mar 21, 2019

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

refactor(tofs): avoid using “salt['config.get']” for formula writers

4bf9453

* saltstack-formulas/template-formula#77

myii mentioned this pull request Mar 21, 2019

refactor(tofs): update with latest changes inc. 100% portable libtofs.jinja saltstack-formulas/systemd-formula#27

Merged

baby-gnu deleted the feature/simplify-files_switch-call branch March 21, 2019 15:39

saltstack-formulas-travis added the released label Mar 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tofs): avoid using “salt['config.get']” for formula writers #77

refactor(tofs): avoid using “salt['config.get']” for formula writers #77

baby-gnu commented Mar 11, 2019

baby-gnu commented Mar 11, 2019

myii commented Mar 11, 2019

myii commented Mar 12, 2019 •

edited

Loading

myii left a comment •

edited

Loading

myii Mar 12, 2019

baby-gnu Mar 12, 2019

baby-gnu commented Mar 12, 2019

baby-gnu commented Mar 13, 2019 •

edited

Loading

myii commented Mar 13, 2019

myii commented Mar 13, 2019

myii commented Mar 14, 2019

baby-gnu commented Mar 14, 2019

myii commented Mar 14, 2019

aboe76 commented Mar 14, 2019

myii commented Mar 15, 2019 •

edited

Loading

alxwr commented Mar 19, 2019

baby-gnu commented Mar 19, 2019

myii commented Mar 21, 2019 •

edited

Loading

saltstack-formulas-travis commented Mar 22, 2019

refactor(tofs): avoid using “salt['config.get']” for formula writers #77

refactor(tofs): avoid using “salt['config.get']” for formula writers #77

Conversation

baby-gnu commented Mar 11, 2019

baby-gnu commented Mar 11, 2019

myii commented Mar 11, 2019

myii commented Mar 12, 2019 • edited Loading

myii left a comment • edited Loading

Choose a reason for hiding this comment

myii Mar 12, 2019

Choose a reason for hiding this comment

baby-gnu Mar 12, 2019

Choose a reason for hiding this comment

baby-gnu commented Mar 12, 2019

baby-gnu commented Mar 13, 2019 • edited Loading

myii commented Mar 13, 2019

myii commented Mar 13, 2019

myii commented Mar 14, 2019

baby-gnu commented Mar 14, 2019

myii commented Mar 14, 2019

aboe76 commented Mar 14, 2019

myii commented Mar 15, 2019 • edited Loading

alxwr commented Mar 19, 2019

baby-gnu commented Mar 19, 2019

myii commented Mar 21, 2019 • edited Loading

saltstack-formulas-travis commented Mar 22, 2019

myii commented Mar 12, 2019 •

edited

Loading

myii left a comment •

edited

Loading

baby-gnu commented Mar 13, 2019 •

edited

Loading

myii commented Mar 15, 2019 •

edited

Loading

myii commented Mar 21, 2019 •

edited

Loading