Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve consul_pillar configuration parsing #49903

Merged
merged 12 commits into from
Oct 16, 2018
Merged

Improve consul_pillar configuration parsing #49903

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
80466c1
Merge pull request #1 from saltstack/develop
Oct 2, 2018
bd63077
improved consul_pillar config parsing
Oct 4, 2018
0f323eb
updated test_pillar_nest test
Oct 4, 2018
384d65d
made sure fetch_tree behaves consistently when called externally
Oct 5, 2018
e5876aa
Merge branch 'develop' into consul_pillar2
Oct 8, 2018
2332ab3
Correctly handle a blank root path
Oct 9, 2018
286bd06
Merge branch 'consul_pillar2' of github.com:FraaJad/salt into consul_…
Oct 9, 2018
6d384f8
moved path parsing into consul_fetch function to dedup code.
Oct 9, 2018
b138d5e
protect against possible path of None
Oct 12, 2018
834a982
made more concise
Oct 12, 2018
4958a73
Merge branch 'develop' into consul_pillar2
Oct 12, 2018
8f63ee7
Merge branch 'develop' into consul_pillar2
Oct 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions salt/pillar/consul_pillar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,18 +187,18 @@ def ext_pillar(minion_id,
if minion_id not in minions:
return {}

root_re = re.compile('root=(\S*)') # pylint: disable=W1401
root_re = re.compile('(?<!_)root=(\S*)') # pylint: disable=W1401
match = root_re.search(temp)
if match:
opts['root'] = match.group(1)
opts['root'] = match.group(1).rstrip('/')
temp = temp.replace(match.group(0), '')
else:
opts['root'] = ""

pillar_root_re = re.compile('pillar_root=(\S*)') # pylint: disable=W1401
match = pillar_root_re.search(temp)
if match:
opts['pillar_root'] = match.group(1)
opts['pillar_root'] = match.group(1).rstrip('/')
temp = temp.replace(match.group(0), '')
else:
opts['pillar_root'] = ""
Expand Down Expand Up @@ -237,7 +237,7 @@ def ext_pillar(minion_id,

pillar = {}
branch = pillar
keys = opts['pillar_root'].rstrip('/').split('/')
keys = opts['pillar_root'].split('/')

for i, k in enumerate(keys):
if i == len(keys) - 1:
Expand Down Expand Up @@ -267,7 +267,8 @@ def fetch_tree(client, path, expand_keys):
are folders. Take the remaining data and send it to be formatted
in such a way as to be used as pillar data.
'''
_, items = consul_fetch(client, path)
absolute_path = path.rstrip('/') + '/'
_, items = consul_fetch(client, absolute_path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nooo, inside the consul_fetch. It should look on its own if the path is right or not. If you reuse it elsewhere, you don't know if trailing slash is needed or not, so you will do it every time you call it, and thus duplicate the code.

Please move that rstrip+/ inside the function. 😉

ret = {}
has_children = re.compile(r'/$')

Expand All @@ -276,9 +277,9 @@ def fetch_tree(client, path, expand_keys):
if items is None:
return ret
for item in reversed(items):
key = re.sub(r'^' + path + '/?', '', item['Key'])
key = re.sub(r'^' + re.escape(absolute_path), '', item['Key'])
if key != '':
log.debug('key/path - %s: %s', path, key)
log.debug('path/key - %s: %s', absolute_path, key)
log.debug('has_children? %r', has_children.search(key))
if has_children.search(key) is None:
ret = pillar_format(ret, key.split('/'), item['Value'], expand_keys)
Expand Down
3 changes: 1 addition & 2 deletions tests/unit/pillar/test_consul_pillar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,8 @@ def test_pillar_nest(self):
with patch.dict(consul_pillar.__salt__, {'grains.get': MagicMock(return_value=({}))}):
with patch.object(consul_pillar, 'consul_fetch', MagicMock(return_value=('2232', PILLAR_DATA))):
pillar_data = consul_pillar.ext_pillar(
'testminion', {}, 'consul_config root=test-shared/ pillar_root=nested-key/'
'testminion', {}, 'consul_config pillar_root=nested-key/ root=test-shared/ '
)
consul_pillar.consul_fetch.assert_called_once_with('consul_connection', 'test-shared/')
assert sorted(pillar_data['nested-key']) == ['sites', 'user']
self.assertNotIn('blankvalue', pillar_data['nested-key']['user'])

Expand Down