feat(map): update to v4 and add config.get lookup from multiple roots #186

baby-gnu · 2020-07-21T15:41:09Z

PR progress checklist (to be filled in by reviewers)

Changes to documentation are appropriate (or tick if not required)
Changes to tests are appropriate (or tick if not required)
Reviews completed

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 tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

saltstack-formulas/libvirt-formula#79

Describe the changes you're proposing

Use the new v4 map.jinja and maintain the compatibility with user pillars by configuring a new map_jinja:config_get_roots parameter.

The map.jinja use the generic exported name mapdata as in the latest libvirt-formula.

Pillar / config required to test the proposed changes

It's already provided by test/salt/pillar/default.sls.

Debug log showing how the proposed changes work

       [INFO    ] Fetching file from saltenv 'base', ** done ** 'openssh/map.jinja'
       [DEBUG   ] In saltenv 'base', looking at rel_path 'openssh/libsaltcli.jinja' to resolve 'salt://openssh/libsaltcli.jinja'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openssh/libsaltcli.jinja' to resolve 'salt://openssh/libsaltcli.jinja'
       [DEBUG   ] LazyLoaded log.debug
       [DEBUG   ] [libsaltcli] the salt command type has been identified to be: local
       [DEBUG   ] map.jinja: initialise parameters from openssh/parameters/defaults.yaml
       [DEBUG   ] In saltenv 'base', looking at rel_path 'openssh/parameters/defaults.yaml' to resolve 'salt://openssh/parameters/defaults.yaml'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openssh/parameters/defaults.yaml' to resolve 'salt://openssh/parameters/defaults.yaml'
       [DEBUG   ] map.jinja: lookup 'map_jinja' configuration sources
       [DEBUG   ] LazyLoaded config.get
       [DEBUG   ] key: map_jinja:sources, ret: _|-
       [DEBUG   ] key: openssh:map_jinja:sources, ret: _|-
       [DEBUG   ] map.jinja: load parameters with sources from ['osarch', 'os_family', 'os', 'osfinger', 'config_get_lookup', 'config_get', 'id']
       [DEBUG   ] map.jinja: initialise 'config.get' roots with 'tplroot' openssh
       [DEBUG   ] key: map_jinja:config_get_roots, ret: _|-
       [DEBUG   ] key: openssh:map_jinja:config_get_roots, ret: _|-
       [DEBUG   ] map.jinja: load parameters with 'config.get' from roots [u'openssh', u'sshd_config', u'ssh_config']
       [DEBUG   ] key: openssh:strategy, ret: _|-
       [DEBUG   ] key: openssh:merge_lists, ret: _|-
       [DEBUG   ] map.jinja: load parameters from file openssh/parameters/osarch/x86_64.yaml
       [DEBUG   ] Could not find file 'salt://openssh/parameters/osarch/x86_64.yaml' in saltenv 'base'
       [DEBUG   ] map.jinja: load parameters from file openssh/parameters/os_family/Arch.yaml
       [DEBUG   ] In saltenv 'base', looking at rel_path 'openssh/parameters/os_family/Arch.yaml' to resolve 'salt://openssh/parameters/os_family/Arch.yaml'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openssh/parameters/os_family/Arch.yaml' to resolve 'salt://openssh/parameters/os_family/Arch.yaml'
       [DEBUG   ] map.jinja: merge parameters from openssh/parameters/os_family/Arch.yaml
       [DEBUG   ] LazyLoaded slsutil.merge
       [DEBUG   ] map.jinja: load parameters from file openssh/parameters/os/Arch.yaml
       [DEBUG   ] Could not find file 'salt://openssh/parameters/os/Arch.yaml' in saltenv 'base'
       [DEBUG   ] key: osfinger, ret: _|-
       [DEBUG   ] map.jinja: load parameters from file openssh/parameters/osfinger.yaml
       [DEBUG   ] Could not find file 'salt://openssh/parameters/osfinger.yaml' in saltenv 'base'
       [DEBUG   ] map.jinja: retrieve 'openssh:lookup' with 'config.get', merge: strategy='None'
       [DEBUG   ] key: openssh:lookup, ret: _|-
       [DEBUG   ] map.jinja: merge 'openssh:lookup' retrieved with 'config.get', merge: strategy='smart', lists='False'
       [DEBUG   ] map.jinja: retrieve 'sshd_config:lookup' with 'config.get', merge: strategy='None'
       [DEBUG   ] key: sshd_config:lookup, ret: _|-
       [DEBUG   ] map.jinja: merge 'sshd_config:lookup' retrieved with 'config.get', merge: strategy='smart', lists='False'
       [DEBUG   ] map.jinja: retrieve 'ssh_config:lookup' with 'config.get', merge: strategy='None'
       [DEBUG   ] key: ssh_config:lookup, ret: _|-
       [DEBUG   ] map.jinja: merge 'ssh_config:lookup' retrieved with 'config.get', merge: strategy='smart', lists='False'
       [DEBUG   ] map.jinja: retrieve 'openssh' with 'config.get', merge: strategy='None'
       [DEBUG   ] map.jinja: merge 'openssh' retrieved with 'config.get', merge: strategy='smart', lists='False'
       [DEBUG   ] map.jinja: retrieve 'sshd_config' with 'config.get', merge: strategy='None'
       [DEBUG   ] map.jinja: merge 'sshd_config' retrieved with 'config.get', merge: strategy='smart', lists='False'
       [DEBUG   ] map.jinja: retrieve 'ssh_config' with 'config.get', merge: strategy='None'
       [DEBUG   ] map.jinja: merge 'ssh_config' retrieved with 'config.get', merge: strategy='smart', lists='False'
       [DEBUG   ] map.jinja: load parameters from file openssh/parameters/id/7c13b93cdef4.yaml
       [DEBUG   ] Could not find file 'salt://openssh/parameters/id/7c13b93cdef4.yaml' in saltenv 'base'
       [DEBUG   ] map.jinja: save parameters in variable 'mapdata'

Documentation checklist

Updated the README (e.g. Available states).
Updated pillar.example.

Testing checklist

Included in Kitchen (i.e. under state_top).
Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
Updated the relevant test pillar.

Additional context

pull-assistant · 2020-07-21T15:41:11Z

Best reviewed: commit by commit

Optimal code review plan

feat(map): update to v4 “map.jinja”

feat(map): config.get lookups from configurable roots

chore: add breaking change message for new map.jinja

Powered by Pull Assistant. Last update df477b2 ... a0af21a. Read the comment docs.

The `map.jinja` now exports a single variable called `mapdata`. We extract the `openssh`, `sshd_config` and `ssh_config` from it to minimize the changes to `.sls` files.

We avoid compatibility break with user pillars by looking up configuration values using `config.get` in configurable roots. We provide a new parameter `map_jinja:config_get_roots` in the formula `parameters/defaults.yaml`to retrives values not only from `tplroot=openssh` but from `sshd_config` and `ssh_config` too. We need to update the `_mapdata` reference files to include the new `map_jinja:config_get_roots`.

BREAKING CHANGE: `map.jinja` has been upgraded from using `pillar.get` to `config.get`.

myii · 2020-08-01T13:51:43Z

Merged, excellent work @baby-gnu!

myii · 2020-08-01T13:56:51Z

@baby-gnu Something worth thinking about (for v6 of map.jinja?!): how about using Salt targetting grammar instead? See this from the discussion with @OrangeDog in the Slack channel.

https://freenode.logbot.info/saltstack-formulas/20200730#c4572975-c4573065

- - -

16:13 OrangeDog[m] Is it possible to make it less ambiguous? There's nothing stopping there being a grain called config_get_lookup , or a function os.family

16:13 myii nebuchadnezzar: Ready to be merged from your end?

16:13 OrangeDog[m] For example, see the targeting grammar

16:14 nebuchadnezzar myii: yes

16:15 myii OK, I'll do that once it's green -- thanks for all of your efforts!

16:15 nebuchadnezzar see you tomorrow ;-)

16:16 myii[m] OrangeDog: Any suggestions? We did have some other ideas but they're lost somewhere in GitHub Discussions (not searchable yet).

16:18 OrangeDog[m] grain:os_family , G!os_family , grains[os_family], salt:config.get , module:config.get, F!config.get , config.get() , salt[config.get]

16:19 OrangeDog[m] (edited) ... , G!os_family , grains[os_family], salt:config.get , module:config.get, F!config.get , ... => ... , G@os_family , grains[os_family], salt:config.get , module:config.get, F⊙cg , ...

saltstack-formulas-travis · 2020-08-01T13:59:35Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

myii · 2020-08-01T14:02:07Z

Note: Checked this across all platforms in Travis and only failing on CentOS-6, as expected from our discussions in Slack/IRC:

https://travis-ci.org/github/myii/openssh-formula/builds/713956410

baby-gnu requested a review from a team as a code owner July 21, 2020 15:41

feat(map): update to v4 “map.jinja”

df477b2

The `map.jinja` now exports a single variable called `mapdata`. We extract the `openssh`, `sshd_config` and `ssh_config` from it to minimize the changes to `.sls` files.

baby-gnu force-pushed the feature/v4-map.jinja branch 2 times, most recently from 986518d to 2adbb2d Compare July 31, 2020 09:20

baby-gnu force-pushed the feature/v4-map.jinja branch from 2adbb2d to ad4385b Compare July 31, 2020 11:00

chore: add breaking change message for new map.jinja

a0af21a

BREAKING CHANGE: `map.jinja` has been upgraded from using `pillar.get` to `config.get`.

myii approved these changes Aug 1, 2020

View reviewed changes

myii merged commit db67ce6 into saltstack-formulas:master Aug 1, 2020

saltstack-formulas-travis added the released label Aug 1, 2020

baby-gnu deleted the feature/v4-map.jinja branch August 21, 2020 13:35

This was referenced Aug 21, 2020

fix(map): path_join can be used only for local file access #190

Merged

feat(map): use targeting like syntax for configuration #191

Merged

docs(map): describe the use for users and authors #194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(map): update to v4 and add config.get lookup from multiple roots #186

feat(map): update to v4 and add config.get lookup from multiple roots #186

baby-gnu commented Jul 21, 2020 •

edited

Loading

pull-assistant bot commented Jul 21, 2020 •

edited

Loading

myii commented Aug 1, 2020

myii commented Aug 1, 2020

saltstack-formulas-travis commented Aug 1, 2020

myii commented Aug 1, 2020

feat(map): update to v4 and add config.get lookup from multiple roots #186

feat(map): update to v4 and add config.get lookup from multiple roots #186

Conversation

baby-gnu commented Jul 21, 2020 • edited Loading

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?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

pull-assistant bot commented Jul 21, 2020 • edited Loading

myii commented Aug 1, 2020

myii commented Aug 1, 2020

saltstack-formulas-travis commented Aug 1, 2020

myii commented Aug 1, 2020

baby-gnu commented Jul 21, 2020 •

edited

Loading

Does this PR introduce a `BREAKING CHANGE`?

pull-assistant bot commented Jul 21, 2020 •

edited

Loading