search

saltstack-formulas / libvirt-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
19 stars 43 forks source link

style(map.jinja): it's prettier with the Jinja open mark not indented #77

Closed baby-gnu closed 4 years ago

baby-gnu commented 4 years ago

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?

No.

Related issues and/or pull requests

https://tree.taiga.io/project/myii-saltstack-formulas/task/243

Describe the changes you're proposing

As done with libsaltcli.jinja, with huge bloc of Jinja, it's a little bit more readable by getting the indentation between the Jinja open mark and the code.

Pillar / config required to test the proposed changes

No change

Debug log showing how the proposed changes work

No change

Documentation checklist

Testing checklist

Additional context

pull-assistant[bot] commented 4 years ago
Score: 0.86

Best reviewed: commit by commit

Optimal code review plan (2 commits squashed)

     style(map.jinja): it's prettier with the Jinja open mark not indented
style(libtofs): it's... ... style(libtofs): use ...
> Squashed 2 commits: > - style(libtofs): it's prettier with the Jinja open mark not indented > - style(libtofs): use Black-inspired Jinja formatting
     style(map): use Black-inspired Jinja formatting      fix(libtofs): remove trailing coma on macro parameters list

Powered by Pull Assistant. Last update c81943b ... 36589e4. Read the comment docs.

daks commented 4 years ago

OK, this is an important choice to be made. I'm used to indent Jinja like it was 'before' and not like the 'new' one. I'm not sure I like this change.

daks commented 4 years ago

Can be interesting to see also this change for a state file where Jinja directives contains YAML and see which indent method looks 'better'. I like be able to quickly see the level of indentation to know where I am in a loop/conditional/... like in Python.

baby-gnu commented 4 years ago

Can be interesting to see also this change for a state file where Jinja directives contains YAML and see which indent method looks 'better'.

I do not use this “new” style for Jinja/YAML, only for pure Jinja. You can see libsaltcli.jinja for another example.

myii commented 4 years ago

@daks It's interesting how we all see things. Over time, I've become very opinionated on how I like my Jinja and it's boiled down to four things. In roughly descending order of importance:

  1. What's closest to Python indentation (not the tag as much as the actual code)
  2. What's easiest to read (especially over long blocks of Jinja)
  3. What works best with my editor's folding methods (aligned with the YAML of the file, which is the premium content of the file, Jinja being the second-rate passenger!)
  4. What is rendered best in the actual outputs (obviously Salt's debug output but even the file templates which use Jinja, since I've been surprised to find spurious whitespace throughout)

In terms of files which are only Jinja, then obviously number 3 doesn't apply. What I've found is the fastest read I can get on these files while maintaining the indentation is like the change that is being proposed here. I can't scan anywhere near as quickly when the starting tag keeps interfering with my view!

But again, all of this is opinionated but I must stress that most of this has been dictated by actual functionality. We're going to have to figure things out. @dafyddj did suggest to me that we really need an org-wide Jinja style guide, to make these decisions once and for all.

myii commented 4 years ago

Here's an interesting "before" and "after" example of salt/formulas.sls. Open in two tabs and keep switching between the tabs to appreciate the difference:

The "after" may not be wonderful but it is so much better than the "before", purely because it was impossible to tell which scope you were in when working with the "before" -- I made numerous errors with that file when developing that feature prior to that.

daks commented 4 years ago

In this example the 'before' is not correctly indented for me. I quickly formatted the way I like see it here https://friendpaste.com/4LO2NyYio8PLCiqBOLm3JY

but not sure how to indent lines 31, 34 or 48, 50 and beyond. (but the problem may be the same with this other method)

I think the problem is also having too much Jinja in salt code. As said in official salt doc 'Easy on the Jinja!' https://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html#easy-on-the-jinja

daks commented 4 years ago

Can be interesting to see also this change for a state file where Jinja directives contains YAML and see which indent method looks 'better'.

I do not use this “new” style for Jinja/YAML, only for pure Jinja. You can see libsaltcli.jinja for another example.

okay, but then it means we will have two Jinja formatting method

baby-gnu commented 4 years ago

I think the problem is also having too much Jinja in salt code. As said in official salt doc 'Easy on the Jinja!

I agree with you, mixing lot of Jinja in YAML files or configuration file template is a bad idea.

But I found this less problematic for Jinja only files like map.jinja. The only issue with this map.jinja is that Jinja is not as readable as Python because of the markers ({% and %}).

In which case, putting spaces between {% and the aligned statement help my eyes a lot to follow the code. When {% is at the first column of an idented line, my eye manage to forget it and better focus on the statement.

To compare, you can look at map.jinja before and after:

okay, but then it means we will have two Jinja formatting method

I personally consider map.jinja of a different type than sls files and configuration file templates. So, I see no problem of having different formatting methods for different type of files ;-)

daks commented 4 years ago

I think the problem is also having too much Jinja in salt code. As said in official salt doc 'Easy on the Jinja!

I agree with you, mixing lot of Jinja in YAML files or configuration file template is a bad idea.

But I found this less problematic for Jinja only files like map.jinja. The only issue with this map.jinja is that Jinja is not as readable as Python because of the markers ({% and %}).

yes, Jinja only files are less problematic because we can consider them like 'black boxes', we know what they do but not how :)

I tend to consider them like that because reading how it really works takes me a lot of 'time/concentration' (yes I really don't like Jinja except parsimoniously). YMMV.

In which case, putting spaces between {% and the aligned statement help my eyes a lot to follow the code. When {% is at the first column of an idented line, my eye manage to forget it and better focus on the statement.

To compare, you can look at map.jinja before and after:

* [before](https://raw.githubusercontent.com/saltstack-formulas/libvirt-formula/ad923eefebec10a64f9943e230dda28cc3241c7d/libvirt/map.jinja)

* [after](https://raw.githubusercontent.com/saltstack-formulas/libvirt-formula/c2808ed5960b4a459cc34b6017f3b7d8cfe93d41/libvirt/map.jinja)

That comparison seems better. Still not sure which I prefer but OK. Let's see others' thoughts.

okay, but then it means we will have two Jinja formatting method

I personally consider map.jinja of a different type than sls files and configuration file templates. So, I see no problem of having different formatting methods for different type of files ;-)

ok :)

myii commented 4 years ago

To add to the conversation, here's a Black version of libtofs.jinja (have modified to use an indent of 2, though):

{%- macro files_switch(
      source_files,
      lookup=None,
      default_files_switch=["id", "os_family"],
      indent_width=6,
      use_subpath=False
    ) %}
{#-
    Returns a valid value for the "source" parameter of a "file.managed"
    state function. This makes easier the usage of the Template Override and
    Files Switch (TOFS) pattern.
    Params:
      * source_files: ordered list of files to look for
      * lookup: key under "<tplroot>:tofs:source_files" to prepend to the
        list of source files
      * default_files_switch: if there's no config (e.g. pillar)
        "<tplroot>:tofs:files_switch" this is the ordered list of grains to
        use as selector switch of the directories under
        "<path_prefix>/files"
      * indent_width: indentation of the result value to conform to YAML
      * use_subpath: defaults to `False` but if set, lookup the source file
        recursively from the current state directory up to `tplroot`
    Example (based on a `tplroot` of `xxx`):
    If we have a state:
      Deploy configuration:
        file.managed:
          - name: /etc/yyy/zzz.conf
          - source: {{ files_switch(
                         ["/etc/yyy/zzz.conf", "/etc/yyy/zzz.conf.jinja"],
                         lookup="Deploy configuration",
                       ) }}
          - template: jinja
    In a minion with id=theminion and os_family=RedHat, it's going to be
    rendered as:
      Deploy configuration:
        file.managed:
          - name: /etc/yyy/zzz.conf
          - source:
            - salt://xxx/files/theminion/etc/yyy/zzz.conf
            - salt://xxx/files/theminion/etc/yyy/zzz.conf.jinja
            - salt://xxx/files/RedHat/etc/yyy/zzz.conf
            - salt://xxx/files/RedHat/etc/yyy/zzz.conf.jinja
            - salt://xxx/files/default/etc/yyy/zzz.conf
            - salt://xxx/files/default/etc/yyy/zzz.conf.jinja
          - template: jinja
#}
{#-   Get the `tplroot` from `tpldir` #}
{%-   set tplroot = tpldir.split("/")[0] %}
{%-   set path_prefix = salt["config.get"](tplroot ~ ":tofs:path_prefix", tplroot) %}
{%-   set files_dir = salt["config.get"](tplroot ~ ":tofs:dirs:files", "files") %}
{%-   set files_switch_list = salt["config.get"](
        tplroot ~ ":tofs:files_switch", default_files_switch
      ) %}
{#-   Lookup source_files (v2), files (v1), or fallback to an empty list #}
{%-   set src_files = salt["config.get"](
        tplroot ~ ":tofs:source_files:" ~ lookup,
        salt["config.get"](tplroot ~ ":tofs:files:" ~ lookup, []),
      ) %}
{#-   Append the default source_files #}
{%-   set src_files = src_files + source_files %}
{#-   Only add to [""] when supporting older TOFS implementations #}
{%-   set path_prefix_exts = [""] %}
{%-   if use_subpath and tplroot != tpldir %}
{#-     Walk directory tree to find {{ files_dir }} #}
{%-     set subpath_parts = tpldir.lstrip(tplroot).lstrip("/").split("/") %}
{%-     for path in subpath_parts %}
{%-       set subpath = subpath_parts[0 : loop.index] | join("/") %}
{%-       do path_prefix_exts.append("/" ~ subpath) %}
{%-     endfor %}
{%-   endif %}
{%-   for path_prefix_ext in path_prefix_exts | reverse %}
{%-     set path_prefix_inc_ext = path_prefix ~ path_prefix_ext %}
{#-     For older TOFS implementation, use `files_switch` from the config #}
{#-     Use the default, new method otherwise #}
{%-     set fsl = salt["config.get"](
          tplroot ~ path_prefix_ext | replace("/", ":") ~ ":files_switch",
          files_switch_list,
        ) %}
{#-     Append an empty value to evaluate as `default` in the loop below #}
{%-     if "" not in fsl %}
{%-       set fsl = fsl + [""] %}
{%-     endif %}
{%-     for fs in fsl %}
{%-       for src_file in src_files %}
{%-         if fs %}
{%-           set fs_dirs = salt["config.get"](fs, fs) %}
{%-         else %}
{%-           set fs_dirs = salt["config.get"](
                tplroot ~ ":tofs:dirs:default", "default"
              ) %}
{%-         endif %}
{#-         Force the `config.get` lookup result as a list where necessary #}
{#-         since we need to also handle grains that are lists #}
{%-         if fs_dirs is string %}
{%-           set fs_dirs = [fs_dirs] %}
{%-         endif %}
{%-         for fs_dir in fs_dirs %}
{#-           strip empty elements by using a select #}
{%-           set url = (
                [
                  "- salt:/",
                  path_prefix_inc_ext.strip("/"),
                  files_dir.strip("/"),
                  fs_dir.strip("/"),
                  src_file.strip("/"),
                ]
                | select
                | join("/")
              ) %}
{{ url | indent(indent_width, true) }}
{%-         endfor %}
{%-       endfor %}
{%-     endfor %}
{%-   endfor %}
{%- endmacro %}
myii commented 4 years ago

@baby-gnu And here's a Black version of map.jinja (have modified to use an indent of 2, though):

# -*- coding: utf-8 -*-
# vim: ft=jinja

{#- Get the `tplroot` from `tpldir` #}
{%- set tplroot = tpldir.split("/")[0] %}
{%- from tplroot ~ "/libsaltcli.jinja" import cli with context %}

{#- Where to lookup parameters source files #}
{%- set map_sources_dir = tplroot | path_join("parameters") %}

{#- Load defaults first to allow per formula default map.jinja configuration #}
{%- set _defaults_filename = map_sources_dir | path_join("defaults.yaml") %}
{%- do salt["log.debug"](
      "map.jinja: initialise parameters from "
      ~ _defaults_filename
    ) %}
{%- import_yaml _defaults_filename as default_settings %}

{#- List of sources to lookup for parameters #}
{%- do salt["log.debug"]("map.jinja: lookup 'map_jinja' configuration sources") %}
{#- Fallback to previously used grains plus minion `id` #}
{%- set map_sources = [
      "osarch",
      "os_family",
      "os",
      "osfinger",
      "config_get_lookup",
      "config_get",
      "id",
    ] %}
{#- Configure map.jinja from defaults.yaml #}
{%- set map_sources = default_settings | traverse(
      "values:map_jinja:sources",
      map_sources,
    ) %}

{#- Lookup global sources #}
{%- set map_sources = salt["config.get"]("map_jinja:sources", map_sources) %}
{#- Lookup per formula sources #}
{%- set map_sources = salt["config.get"](
      tplroot ~ ":map_jinja:sources",
      map_sources,
    ) %}

{%- do salt["log.debug"](
      "map.jinja: load parameters with sources from "
      ~ map_sources
    ) %}

{#- Work around assignment inside for loop #}
{#- load configuration values used in `config.get` merging strategies #}
{%- set _config = {
      "stack": default_settings.get("values", {}),
      "merge_strategy": salt["config.get"](tplroot ~ ":strategy", None),
      "merge_lists": salt["config.get"](tplroot ~ ":merge_lists", False),
    } %}

{#- the `config.get` merge option only works for `minion` or `local` salt command types #}
{%- if cli in ["minion", "local"] %}
{%-   do _config.update(
        {
          "merge_opt": {"merge": _config["merge_strategy"]},
          "merge_msg": ", merge: strategy='" ~ _config["merge_strategy"] ~ "'",
        }
      ) %}
{#- the `config.get` merge option is not available for `ssh` or `unknown` salt command types #}
{%- else %}
{%-   if _config["merge_strategy"] %}
{%-     do salt["log.error"](
          "map.jinja: the 'merge' option of 'config.get' is skipped when the salt command type is '"
          ~ cli
          ~ "'"
        ) %}
{%-   endif %}
{%-   do _config.update(
        {
          "merge_opt": {},
          "merge_msg": "",
        }
      ) %}
{%- endif %}

{#- process each `map.jinja` source #}
{%- for map_source in map_sources %}
{%-   if map_source  in ["config_get", "config_get_lookup"] %}
{%-     set _config_key = {
          "config_get": tplroot,
          "config_get_lookup": tplroot ~ ":lookup",
        }.get(map_source) %}
{%-     set _config_type = {
          "config_get": "configuration",
          "config_get_lookup": "lookup",
        }.get(map_source) %}

{%-     do salt["log.debug"](
          "map.jinja: retrieve formula "
          ~ _config_type
          ~ " with 'config.get'"
          ~ _config["merge_msg"]
        ) %}
{%-     set _config_get = salt["config.get"](
          _config_key, default={}, **_config["merge_opt"]
        ) %}

{#-     `slsutil.merge` defaults to `smart` instead of `None` for `config.get` #}
{%-     set _strategy = _config["merge_strategy"] | default("smart", boolean=True) %}
{%-     do salt["log.debug"](
          "map.jinja: merge formula "
          ~ _config_type
          ~ " retrieved with 'config.get'"
          ~ ", merge: strategy='"
          ~ _strategy
          ~ "', lists='"
          ~ _config["merge_lists"]
          ~ "'"
        ) %}
{%-     do _config.update(
          {
            "stack": salt["slsutil.merge"](
              _config["stack"],
              _config_get,
              strategy=_strategy,
              merge_lists=_config["merge_lists"],
            )
          }
        ) %}
{%-   else %}
{#-     Lookup the grain/pillar/... #}
{#-     Fallback to use the source name as a direct filename #}
{%-     set map_values = salt["config.get"](map_source, []) %}

{#-     Mangle `map_source` to use it as literal path #}
{%-     if map_values | length == 0 %}
{%-       set map_source_parts = map_source.split("/") %}
{%-       set map_source = map_source_parts[0:-1] | join("/") %}
{%-       set map_values = map_source_parts[-1].rstrip(".yaml") %}
{%-     endif %}

{#-     Some configuration return list #}
{%-     if map_values is string %}
{%-       set map_values = [map_values] %}
{%-     endif %}

{%-     for map_value in map_values %}
{%-       set yamlfile = map_sources_dir | path_join(
            map_source,
            map_value ~ ".yaml",
          ) %}
{%-       do salt["log.debug"]("map.jinja: load parameters from file " ~ yamlfile) %}
{%-       load_yaml as loaded_values %}
{%-         include yamlfile ignore missing %}
{%-       endload %}

{%-       if loaded_values %}
{#-         Merge loaded values on the stack #}
{%-         do salt["log.debug"]("map.jinja: merge parameters from " ~ yamlfile) %}
{%-         do _config.update(
              {
                "stack": salt["slsutil.merge"](
                  _config["stack"],
                  loaded_values.get("values", {}),
                  strategy=loaded_values.get("strategy", "smart"),
                  merge_lists=loaded_values.get("merge_lists", False)
                  | to_bool,
                )
              }
            ) %}
{%-       endif %}
{%-     endfor %}
{%-   endif %}
{%- endfor %}

{%- do salt["log.debug"]("map.jinja: save parameters in variable 'libvirt_settings'") %}
{%- set libvirt_settings = _config["stack"] %}
myii commented 4 years ago

@baby-gnu OK, that worked:

Had to remove one trailing comma after the last macro parameter (modified above). The files for that build look like this:

saltstack-formulas-travis commented 4 years ago

:tada: This PR is included in version 3.7.6 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

myii commented 4 years ago

@baby-gnu Just for your information, the libtofs.jinja changes have been propagated across all of the formulas where relevant:

There's quite a few formulas using it these days!