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.

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

Allow for changing the default merging strategy. #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mmulqueen
Copy link

We have been using Pillarstack for a week or so now and it's great, it's a real improvement from Salt's built in pillar, but still quite simple. For us, it's made sense to use a merge-first strategy throughout - it gives us layers of increasingly generic defaults.

I found that I was having to put __: merge-first in all the files though and I kept forgetting. This is a patch to be able to change the default and take the human error out of the equation.

Configured in the salt master config like this:

ext_pillar:
  - stack: /path/to/stack.cfg?default_strategy=merge-first

Hopefully this is a useful feature. I feel like the way of configuring it is a bit hackish, but considering how you're already using args, kwargs in ext_pillar, I thought this was the cleanest way of doing it. Or could perhaps switch to a dict style instead of URL parameter style.

@bbinet
Copy link
Owner

bbinet commented May 10, 2019

Thanks @mmulqueen, this is a nice improvement.

But I would rather configure the default strategy in salt config instead of the URL parameter style config which is actually a bit hackish.
We could for example set the following in salt-master config file:

pillarstack:
  default_strategy: merge-first

(Salt-master config should be available in __opts__ dict in pillarstack).

@@ -120,22 +121,25 @@ def _merge_dict(stack, obj):
stack_k = stack[k]
stack[k] = _cleanup(v)
v = stack_k
# Stop it getting double inverted later on.
if default_strategy == "merge-first":
default_strategy = "merge-last"
Copy link
Owner

Choose a reason for hiding this comment

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

what's the purpose of the above lines?

@mmulqueen
Copy link
Author

mmulqueen commented Jun 24, 2019 via email

@mmulqueen
Copy link
Author

@bbinet No rush at all, but is there any more info/explanation you need on this from me?

@mmulqueen
Copy link
Author

@bbinet We've been successfully using this in prod all summer and it seems to be working well and as we'd expect. Is there anything else I can do to help complete this pull request?

@bbinet
Copy link
Owner

bbinet commented Oct 7, 2019

Sorry @mmulqueen for my silence.
I think I now understand your comment, but then it means this is not really a default strategy as it will only replace the top level strategy (not every level).
So I think it should not be name "default strategy", but rather "top level strategy", right?

Also, I don't really like the overwrite of the default_strategy variable: I think this could be easily avoided if _merge_dict keeps a default value for default_strategy=None (aka toplevel_strategy): then only the first call to _merge_dict can change the toplevel strategy.

Does it make sense? What do you think?

@bbinet
Copy link
Owner

bbinet commented Oct 7, 2019

For the lists, it was actually a "default strategy", not only "top level strategy" so this is not consistent with the dicts...

@bbinet bbinet closed this Oct 7, 2019
@bbinet bbinet reopened this Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants