feat(map.jinja): load configuration values from configurable sources #186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I have an issue where the |
It looks to be a side effect when using I'm doing some debugging right now. |
I found the issue in diff --git a/TEMPLATE/libtofs.jinja b/TEMPLATE/libtofs.jinja
index 900e62b..23c5c60 100644
--- a/TEMPLATE/libtofs.jinja
+++ b/TEMPLATE/libtofs.jinja
@@ -78,7 +78,7 @@
{#- Use the default, new method otherwise #}
{%- set fsl = salt['config.get'](
tplroot ~ path_prefix_ext|replace('/', ':') ~ ':files_switch',
- files_switch_list
+ files_switch_list.copy()
) %}
{#- Append an empty value to evaluate as `default` in the loop below #}
{%- if '' not in fsl %}Without this I'm doing a dedicated PR to fix this issue. Regards. |
19 tasks
|
We must merge #187 before continuing this. |
|
If someone have some comments about variable naming and the understandability of the code I'll take them. Regards. |
baby-gnu
force-pushed
the
feature/configurable-map.jinja
branch
7 times, most recently
from
f73f15e
to
7bcdb65
Compare
19 tasks
baby-gnu
changed the title
baby-gnu
force-pushed
the
feature/configurable-map.jinja
branch
from
76a7029
to
baa44d9
Compare
baby-gnu
changed the title
|
I did a little python3 script which split #!/usr/bin/python3
import glob
import os
import re
from string import Template
import sys
from ruamel.yaml import YAML
if not os.path.isfile('defaults.yaml'):
raise Exception('You must run this script in the same directory as defaults.yaml')
yaml = YAML(typ='rt')
yaml.indent(offset=2)
yaml.preserve_quotes = True
yaml.default_flow_style = False
yaml.explicit_start = True
yaml.explicit_end = True
def load_yaml(filename):
with open(filename, 'r') as yaml_file:
yaml_str = yaml.load(yaml_file)
return yaml_str
if not os.path.isdir('parameters'):
os.makedirs('parameters')
yaml_files = glob.glob('os*.yaml')
config_name_map = {'osfamily': 'os_family'}
## defaults.yaml
print("Move 'defaults.yaml' to 'parameters/defaults.yaml'")
defaults = load_yaml('defaults.yaml')
defaults_keys = defaults.keys()
if len(defaults_keys) != 1:
raise Exception(f'Wrong number of top level keys in defaults.yaml, found: {", ".join(defaults.keys())}')
values_items = defaults.popitem()
values = values_items[1]
defaults_header = """# -*- coding: utf-8 -*-
# vim: ft=yaml
"""
with open('parameters/defaults.yaml', 'w') as defaults_fh:
defaults_fh.write(defaults_header)
yaml.dump(values, defaults_fh)
with open('parameters/defaults.yaml', 'r+') as defaults_fh:
# Strip 2 spaces before misaligned comments
lines = []
for line in defaults_fh.readlines():
line = re.sub(r'^(\s*) #',r'\1#', line)
lines.append(line)
defaults_fh.seek(0)
defaults_fh.truncate(0)
defaults_fh.write(''.join(lines))
## os*map.yaml
config_header = Template("""# -*- coding: utf-8 -*-
# vim: ft=yaml
#
# Setup variables specific to salt['config.get']('${config_name}') == ${config_value}.
# You just need to add the key:values for this `${config_name}` that differ
# from ${config_differ}.
#
# If you do not need to provide defaults via the `${config_name}` config,
# you can remove this file or provide at least an empty dict, e.g.
# values: {}
#
# This file will be merged with `salt.slsutil.merge`:
#
# You can select the merging strategy by defining the `strategy` dict
# with one of: `aggregate`, `list`, `overwrite`, `recurse`, `smart`, e.g.
# strategy: 'recurse'
#
# If you use the `recurse` or `overwrite` strategy, you can aggregate
# the lists by defining the `merge_lists` dict with a boolean, e.g.
# merge_lists: 'true'
""")
config_differ_map = {'osarch': '`defaults.yaml`',
'os': '`defaults.yaml` + `<osarch>.yaml` + `<os_family>.yaml`',
'os_family': '`defaults.yaml` + `<osarch>.yaml`',
'os_finger': '`defaults.yaml` + `<osarch>.yaml` + `<os_family>.yaml` + `<osmap>.yaml`'}
for yaml_file in yaml_files:
config_name = yaml_file.replace('map.yaml', '')
# Some YAML files do not have the proper config name
config_name = config_name_map.get(config_name, config_name)
config_dir = os.path.join('parameters', config_name)
if not os.path.isdir(config_dir):
os.makedirs(config_dir)
print(f"Split '{yaml_file}' to 'parameters/{config_name}/'")
yaml_content = load_yaml(yaml_file)
for item in yaml_content:
print(f"-> write file 'parameters/{config_name}/{item}.yaml'")
with open(os.path.join(config_dir, f'{item}.yaml'), 'w') as config_fh:
config_differ = config_differ_map.get(config_name, '`defaults.yaml`')
config_fh.write(config_header.substitute({'config_name': config_name,
'config_value': item,
'config_differ': config_differ}))
values = {'values': yaml_content[item]}
yaml.dump(values, config_fh)Regards. |
baby-gnu
force-pushed
the
feature/configurable-map.jinja
branch
from
dfdf86d
to
f59c484
Compare
|
only the old |
|
@alxwr I've already conducted a fair bit of testing on this new |
I have troubles executing |
Much better, thanks ;-) With |
@baby-gnu You're welcome.
You can get this with |
|
I'll apply few changes before this being able to merge:
Regards. |
baby-gnu
force-pushed
the
feature/configurable-map.jinja
branch
2 times, most recently
from
e59ee58
to
65cc037
Compare
It's not working with |
|
I just push my first implementation of the configuration of merge strategy for It's not working for |
|
Hello @myii. The following patch is working with diff --git a/TEMPLATE/map.jinja b/TEMPLATE/map.jinja
index 5b2edf6..0be483a 100644
--- a/TEMPLATE/map.jinja
+++ b/TEMPLATE/map.jinja
@@ -4,6 +4,23 @@
{#- Get the `tplroot` from `tpldir` #}
{%- set tplroot = tpldir.split('/')[0] %}
+{#- Determine the type of command being run
+ * min: standard call via. master
+ * cll: `salt-call`
+ * ssh: `salt-ssh`
+ * unk: unknown call #}
+{%- 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 %}
+
{#- Where to lookup parameters source files #}
{%- set map_sources_dir = tplroot | path_join('parameters') %}
@@ -45,10 +62,22 @@
{%- set _merge_lists = {'config_get': _config['config_merge_lists'],
'config_get_lookup': _config['lookup_merge_lists']}.get(map_source) %}
+ {%- if cli == 'ssh' %}
+ {%- if _strategy %}
+ {%- do salt['log.error']("map.jinja: the 'merge' option of 'config.get' is skipped with 'salt-ssh'") %}
+ {%- endif %}
+ {%- set merge_opt = {} %}
+ {%- set merge_msg = '' %}
+ {%- else %}
+ {%- set merge_opt = {'merge': _strategy } %}
+ {%- set merge_msg = ", merge: strategy='" ~ _strategy ~ "'" %}
+ {%- endif %}
+
{%- do salt['log.debug']("map.jinja: retrieve formula " ~ _config_type
- ~ " with 'config.get', "
- ~ "merge: strategy='" ~ _strategy ~ "'") %}
- {%- set _config_get = salt['config.get'](_config_key, merge=_strategy, default={}) %}
+ ~ " with 'config.get'"
+ ~ merge_msg
+ ) %}
+ {%- set _config_get = salt['config.get'](_config_key, default={}, **merge_opt) %}
{%- do salt['log.debug']("map.jinja: merge formula " ~ _config_type
~ " retrieved with 'config.get'"Which gives the following logs: What do you think about this? |
|
@baby-gnu Personally, I like it. I'm just concerned we're going to make |
|
The problem with using a |
@baby-gnu That's a shame, we seem to be held back further and further due to @javierbertoli @dafyddj @daks @aboe76 Can we arrange a rough timeline and strategy for approving (or rejecting) this PR? I hate to trouble @baby-gnu with more and more features, only for a potential rejection upon review. How about the idea I've suggested below? @baby-gnu How about an alternative idea to help the review here. You've already tested this with the |
myii
added a commit
to myii/template-formula
that referenced
this pull request
Mar 25, 2020
* To distinguish between:
- `salt-minion`
- `salt-call`
- `salt-ssh`
* Invoked like `map.jinja`:
- `{%- from tplroot ~ "/libsaltcli.jinja" import cli with context %}`
* Based upon work done in PRs: saltstack-formulas#102, saltstack-formulas#114 & saltstack-formulas#115
* Finalised from saltstack-formulas/libvirt-formula#71
* Required by saltstack-formulas#186
|
This has been implemented in the
|
- the test file must include the `map_jinja:sources` - the `bar` role provide two parameters but the lookup override the `winner` - adapt rubocop since `config_spec.rb` is more that 36 lines
baby-gnu
changed the title
Lookup the configuration for `map.jinja` from: 1. builtin default 1. `osarch` grain 2. `os_family` grain 3. `os` grain 4. `osfinger` grain 5. `lookup` table retrived by `config.get` 6. configuration retrived by `config.get` 7. `id` grain 2. `defaults.yaml`: optionally define a formula specific `map_jinja:sources` 3. global configuration lookup `map_jinja:sources` 4. formula specific `<tplroot>:map_jinja:sources` The hard coded default is backward compatible and add the minion id at the end of the list. If an entry does not match a `salt['config.get']` parameter, it's used as a literal file path (with `.yaml` added if it's missing). Each YAML file are formatted with the following top level keys: - `values`: contains the the parameter values - `strategy` (optional): define the merge strategy for the function `salt.slsutils.merge` (aggregate, list, overwrite, recurse, smart). The default is `smart`. - `merge_lists` (optional): boolean to merge lists or overwrite them It's possile to configure `config.get` when looking up the formula configuration. You need to define a subkey `strategy` with the following differences from YAML files: - `strategy` can only be one of `None`, `overwrite` and `recurse`. The default is `None`. - if you use `salt-ssh`, this merge strategy is skipped and and error log message is emitted. The parameters values are merged in the following order: 1. initialize default values from `defaults.yaml` 2. merge the values from each source defined by the ordered list `map_jinja:sources`
* TEMPLATE/parameters/defaults.yaml: remove the top level key `TEMPLATE` and add a `map.jinja:sources` configuration for tests.
baby-gnu
force-pushed
the
feature/configurable-map.jinja
branch
from
9428b8b
to
30961a7
Compare
|
I updated the
|
|
Looks like this has them same problem that I ran into while solving the Pillar problematic in https://github.com/claudekenni/stdlib-formula YAML Data from cached files which have since been deleted on the Master are still being used within the Formula. This is the reason why in my centralized Map I do the part where I compare the Master and Minion files and then delete cached files that don't exist on the master anymore. Here's a rundown on how to duplicate this: First run of the Formula fails because of dummy values Add Role to the Node Add a yaml file for the role with correct values for package and service Successfully apply the state Remove the yaml file for this role and run the state again successfully even though we'd expect it to fail. Remove the cached yaml file for the role and run the state again, this time failing as expected Here is the Debug output from when the cached yaml file still exists |
19 tasks
|
This pull request will be archived and replace by a |
19 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?No.
Users see no difference.
Related issues and/or pull requests
#116
Describe the changes you're proposing
I propose to split all the
os*map.yamlunderparameters/<config>/<configvalue>.yaml.osarch,os_family,os,osfinger, … is split in a separate file instead of a single YAML file per configurationDebianos_familywithout the need to maintain the othersos_family(oros, …) by providing a single file in itsfile_rootsmap:sources(or the formula specific<tplroot>:map:sources), for example:['osarch', 'os_family', 'os', 'osfinger', 'domain', 'roles', 'id']domain/example.net.yaml,domain/example.org.yaml,roles/web.yaml,roles/ssh.yaml,id/minion1.example.org.yamlgitfsPillar / config required to test the proposed changes
Debug log showing how the proposed changes work
Here is the debug output (the new
map.jinjadebug output is prefixed bymap.jinja::Documentation checklist
README(e.g.Available states).pillar.example.Testing checklist
state_top).Additional context