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

Revert "Make possible to reference previous pillars from subsequent pillars, as they specified in the top file" #42383

Merged
merged 1 commit into from
Jul 19, 2017
Merged

Revert "Make possible to reference previous pillars from subsequent pillars, as they specified in the top file" #42383

merged 1 commit into from
Jul 19, 2017

Conversation

AndreiPashkin
Copy link
Contributor

@AndreiPashkin AndreiPashkin commented Jul 19, 2017

Reverts #37003

I noticed a problem in #37003: in this place, functions that are passed in
the constructor are ignored, which is not right.
Let's revert the previous PR.

I see the way to fix it - remove functions parameter from Pillar interface,
which would affect Pillar, PillarCache, RemotePillar,
RemotePillar, AsyncPillar, AsyncRemotePillar classes as
well as the factories: get_pillar, get_async_pillar. It would encapsulate modules loading within Pillar classes.

@cachedout, @terminalmage, @s0undt3ch, @thatch45, what are your thoughts on this?
Are there any fundamental objections against removing functions parameter?

The only place where this parameter is actually used is here.

@ghost
Copy link

ghost commented Jul 19, 2017

@AndrewPashkin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @terminalmage, @s0undt3ch and @thatch45 to be potential reviewers.

@AndreiPashkin
Copy link
Contributor Author

AndreiPashkin commented Jul 19, 2017

By the way this issue also affects ext_pillar_first setting.

With #37003 reverted it ceases to work as it's currently documented. Previously it was meant to be only for targeting:
https://github.com/saltstack/salt/blob/6135dfa4ddb2c5e70dc348da773a6ff5b8eb9ea7/doc/ref/configuration/master.rst#ext_pillar_first

But now the phrase about targeting is removed and from the docs it now follows that the external pillars could be referenced from the local pillars if the setting is True which is not correct:
https://github.com/saltstack/salt/blob/3e3952600922c3a1a9d08c6de9890349572ae184/doc/ref/configuration/master.rst#ext_pillar_first

It works only because my PR was merged but changes to the docs for this setting and my PR are different and not connected PRs.

So to make it work as it is documented now - functions parameter also should be removed. Or the specification for this setting should be that it is only for targeting.

p.s.
I opened a related issue long time ago:
#37905

@AndreiPashkin
Copy link
Contributor Author

AndreiPashkin commented Jul 19, 2017

It is also possible to replace functions parameter with a similar parameter which instead of LazyLoader would be just a dict of functions, so they wouldn't be bound to pillars.

@cachedout cachedout merged commit c9972fe into saltstack:develop Jul 19, 2017
@cachedout
Copy link
Contributor

Thanks for catching this, @AndrewPashkin

@terminalmage any comments on the question posed above?

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