search

saltstack-formulas / openssh-formula

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

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

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://github.com/saltstack-formulas/libvirt-formula/pull/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

Testing checklist

Additional context

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

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.

myii commented 4 years ago

Merged, excellent work @baby-gnu!

myii commented 4 years ago

@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 commented 4 years ago

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

The release is available on GitHub release

Your semantic-release bot :package::rocket:

myii commented 4 years ago

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