Skip to content

Commit

Permalink
fix(map.jinja): use pillar.get for salt-ssh
Browse files Browse the repository at this point in the history
  • Loading branch information
@myii
myii committed May 12, 2019
1 parent 6f8b9e9 commit 976fe11
Showing 1 changed file with 27 additions and 3 deletions.
30 changes: 27 additions & 3 deletions template/map.jinja
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,41 @@
# -*- coding: utf-8 -*-
# vim: ft=jinja

{#- Determine the type of command being run
* min: standard call via. master
* cll: `salt-call`
* ssh: `salt-ssh`
* unk: unknown call #}

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019

Member

I'd rather use "minion", "local", "ssh" and "unknown" here, because it's so explicit you won't need to keep that nice definitions table in your mind.

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019

Member

But that's just a question of style not function.

This comment has been minimized.

Sign in to view

Copy link
@myii

myii May 15, 2019

Author Member

@alxwr But I like the 3-character codes! In this case, since there's little to be gained by being succinct, your comment makes sense.

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019 •
edited
Loading

Member

@myii: I like 3-char codes too. But when I come back after 3 months they look to me like assembler. :-)

This comment has been minimized.

Sign in to view

Copy link
@myii

myii May 15, 2019

Author Member

No, that was me just teasing. My initial thinking was to avoid long Jinja conditionals but that didn't transpire in the end. If these abbreviations were going to be used throughout the formula, it could be justified but it really doesn't benefit here.
{%- if salt['config.get']('__cli') == 'salt-minion' %}
{%- set cli = 'min' %}
{%- elif salt['config.get']('__cli') == 'salt-call' %}
{%- if salt['config.get']('root_dir') == '/' %}
{%- set cli = 'cll' %}
{%- else %}
{%- set cli = 'ssh' %}
{%- endif %}
{%- else %}
{%- set cli = 'unk' %}
{%- endif %}

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019

Member

@myii Nice work. Maybe we can extract this into a libdetect.jinja?

This comment has been minimized.

Sign in to view

Copy link
@myii

myii May 15, 2019

Author Member

I would love to but Jinja macros don't offer us this facility... unless you know how to bend it to suit our purposes?

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019

Member

Well, we do that with map.jinja all the time:

{% from "stuff/map.jinja" import stuff with context %}

I'm thinking of:

{% from "stuff/libdetect.jinja" import salt_cli_type with context %}
{#- Get the `tplroot` from `tpldir` #}
{%- set tplroot = tpldir.split('/')[0] %}
{#- Start imports as #}
{%- import_yaml tplroot ~ "/defaults.yaml" as default_settings %}
{%- import_yaml tplroot ~ "/osfamilymap.yaml" as osfamilymap %}
{%- import_yaml tplroot ~ "/osmap.yaml" as osmap %}
{%- import_yaml tplroot ~ "/osfingermap.yaml" as osfingermap %}
{%- set _lookup = salt['config.get']('template:lookup', default={}) %}
{%- set _config = salt['config.get']('template', default={}) %}

{%- set template = salt['grains.filter_by'](default_settings, default='template',
{#- Get lookup and config/pillar depending on type of command being run #}
{%- if cli == 'min' or cli == 'cll' %}
{%- set _lookup = salt['config.get'](tplroot ~ ':lookup', default={}, merge='recurse') or {} %}
{%- set _config = salt['config.get'](tplroot, default={}, merge='recurse') or {} %}

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019

Member

@myii please make merge='recurse' configurable. I agree that the default should be recurse, but one should be able to turn it off easily. Maybe config_get_merge_strategy is a good name for a the setting.

This comment has been minimized.

Sign in to view

Copy link
@myii

myii May 15, 2019

Author Member

Yes, as I mentioned in my last comment, I agree this needs to be done. However, I'm in two minds about what strategy to use. merge=None is the default and would be more performant. It also means less head-scratching, trying to work out where configuration elements are coming in from. I've tested merge=recurse with an entire stack consisting of SDB (YAML), minion config, grains and PillarStack and it works really well but almost too well, if you know what I mean.

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019 •
edited
Loading

Member

almost too well

lol, I know what you mean.

I think this is a classic case of us not being God and therefore not knowing which use-cases the future will bring. :-)
In my experience there's less surprise/confusion, when I stick to the default the underlying library already established.
{%- else %}
{%- set _lookup = salt['pillar.get'](tplroot ~ ':lookup', default={}, merge=True) or {} %}
{%- set _config = salt['pillar.get'](tplroot, default={}, merge=True) or {} %}
{%- endif %}

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019

Member

@myii I like this. :-)
Maybe when config.get gets fixed in a future version we can reactivate it for salt-ssh.

This comment has been minimized.

Sign in to view

Copy link
@myii

myii May 15, 2019

Author Member

Hopefully... since we're always supporting three major versions of Salt, this code is going to be around for a long time after the fix is released.

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019

Member

But not forever! :-)

{%- set template = salt['grains.filter_by'](default_settings, default=tplroot,

This comment has been minimized.

Sign in to view

Copy link
@alxwr

alxwr May 15, 2019

Member

Nice catch!
merge=salt['grains.filter_by'](osfamilymap, grain='os_family',
merge=salt['grains.filter_by'](osmap, grain='os',
merge=salt['grains.filter_by'](osfingermap, grain='osfinger',
Expand Down

0 comments on commit 976fe11

Please sign in to comment.