Closed baby-gnu closed 3 years ago
myii
commented
3 years ago @baby-gnu I'm really glad that you've provided this PR. It's unfortunate that I'm in a busier patch than usual, so that I'm unable to review it so far. Hopefully, I'll get time over the coming days. Thanks again!
myii
commented
3 years ago @baby-gnu I finally got a chance to test this on top of the latest changes I've made to the map.jinja verifier via. the ssf-formula (only in my fork so far). The results are good; this PR merges on top cleanly. I've run it across all of the unique platforms and I've found there's only one problematic platform: Ubuntu 16.04.
default-ubuntu-1604-3000-6-py2 failing.local:
Data failed to compile:
----------
Rendering SLS 'base:openvpn._mapdata' failed: Jinja error: do_indent() got an unexpected keyword argument 'first'
/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/libmatchers.jinja(61):
---
[...]
"query_method": query_map[_defaults["query_type"]],
"query_delimiter": _defaults["query_delimiter"],
"query": matcher
}
) %}
{%- do salt["log.debug"]( <======================
log_prefix
~ "use built-in defaults for matcher:\n"
~ parsed | yaml(False) | indent(4, first=True)
) %}salt-image-builder output to see the Jinja version for that pre-salted image; it's Jinja2: 2.8.first was only made avaiable in 2.10: .. versionchanged:: 2.10
Blank lines are not indented by default.
Rename the ``indentfirst`` argument to ``first``.16.04 images and found that only the tiamat and master builds have an appropriate version of Jinja.16.04 instances.Unfortunately, since 16.04 and the versions of Salt being run here will still be maintained for some time, we're going to need a solution for this. Especially since we want to roll out this map.jinja solution to all formulas (over time).
baby-gnu
commented
3 years ago Thanks a lot for this review, I'll look how to make the code compatible with all versions.
baby-gnu
commented
3 years ago Strange, I thought that the new _madata control was proof against ordering and spacing?
Any idea @myii?
baby-gnu
commented
3 years ago Strange, I thought that the new
_madatacontrol was proof against ordering and spacing?
I sorted the _mapdata file since the ruby YAML safe_load and dump do not have option to sort keys.
baby-gnu
commented
3 years ago I rebased the branch with final version of map.jinja, libmapstack.jinja and libmatchers.jinja.
myii
commented
3 years ago I rebased the branch with final version of
map.jinja,libmapstack.jinjaandlibmatchers.jinja.
@baby-gnu The changes haven't been pushed here yet, though?
baby-gnu
commented
3 years ago The changes haven't been pushed here yet, though?
Yes, I pushed my wip branch, now I push this PR.
baby-gnu
commented
3 years ago Hello @myii.
I added:
defaults.yaml.jinja just after defaults.yamlpost_map.yaml.jinja as last fileeach file must return a valid YAML content to be merged by map.jinja, like any other files, the use of .jinja extension permit to circumvent the yamllint errors with Jinja syntax in YAML files.
I think we could event generalise the process: for each <YAML> file to load, load a <YAML>.jinja file juste after.
In this formula, this will result in:
map.jinja configuration load from:
salt://parameters/map_jinja.yamlsalt://parameters/map_jinja.yaml.jinjasalt://openvpn/parameters/map_jinja.yamlsalt://openvpn/parameters/map_jinja.yaml.jinjasalt://openvpn/parameters/defaults.yamlsalt://openvpn/parameters/defaults_jinja.yaml.jinjasalt://openvpn/parameters/osarch/amd64.yamlsalt://openvpn/parameters/osarch/amd64.yaml.jinjasalt://openvpn/parameters/os_family/Debian.yamlsalt://openvpn/parameters/os_family/Debian.yaml.jinjasalt://openvpn/parameters/os/Ubuntu.yamlsalt://openvpn/parameters/os/Ubuntu.yaml.jinjasalt://openvpn/parameters/osfinger/Ubuntu-20.04.yamlsalt://openvpn/parameters/osfinger/Ubuntu-20.04.yaml.jinjasalt["config.get"]("openvpn:lookup"), nothing new heresalt["config.get"]("openvpn"), nothing new heresalt://openvpn/parameters/id/d947a6d5d3d8.yamlsalt://openvpn/parameters/id/d947a6d5d3d8.yaml.jinjaIf you agree, I can make a new commit to show how this will behave.
baby-gnu
commented
3 years ago I made commits to explore the automatic load of .jinja file for each .yaml file.
baby-gnu
commented
3 years ago Now, the post-map.jinja is included without being parsed as a YAML.
It can manipulate the mapdata variable as wanted, for example, I created a user custom post-map.jinja just to have some logs:
{%- do salt["log.debug"]("User defined post map.jinja manipulation") %}Which produce the following log:
[DEBUG ] map.jinja: save parameters in variable 'mapdata'
[DEBUG ] map.jinja: post-processing of 'mapdata'
[DEBUG ] In saltenv 'base', looking at rel_path 'openvpn/post-map.jinja' to resolve 'salt://openvpn/post-map.jinja'
[DEBUG ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/post-map.jinja' to resolve 'salt://openvpn/post-map.jinja'
[DEBUG ] Fetching file from saltenv 'base', ** attempting ** 'salt://openvpn/post-map.jinja'
[DEBUG ] No dest file found
[INFO ] Fetching file from saltenv 'base', ** done ** 'openvpn/post-map.jinja'
[DEBUG ] User defined post map.jinja manipulationWhen no post-map.jinja exists, it logs:
[DEBUG ] map.jinja: save parameters in variable 'mapdata'
[DEBUG ] map.jinja: post-processing of 'mapdata'
[DEBUG ] Could not find file 'salt://openvpn/post-map.jinja' in saltenv 'base'I need to update the map.jinja documentation for the new functionalities.
baby-gnu
commented
3 years ago I updated the documentation, I'll rebase the commits when it's OK to merge.
Thanks.
baby-gnu
commented
3 years ago I think I finished the rebase of that PR.
saltstack-formulas-travis
commented
3 years ago :tada: This PR is included in version 1.0.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
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.
BREAKING CHANGE:
map.jinjanow export a genericmapdatavariableBREAKING CHANGE: The parameters per grains are now under
openvpn/parameters/Related issues and/or pull requests
Describe the changes you're proposing
Use the under reviewing v5
map.jinja:libmatchers.jinja)map.jinjaconfigurable by loading the globalsalt://parameters/map_jinja.yamland the per formulasalt://{{ tplroot }}/parameters/map_jinja.yaml(managed by the newlibmapstack.jinja)libmapstack.jinjamap.jinjaconfiguration to avoid the merge of values fromsalt["config.get"]("openvpn")like it was done previouslyPillar / 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