Bring up ext_pillar rendering errors as well #31380
Conversation
|
cc @regilero |
| ''' | ||
| Extract the sls pillar files from the matches and render them into the | ||
| pillar | ||
| ''' | ||
| if errors is None: | ||
| errors = [] | ||
| pillar = copy.copy(self.pillar_override) | ||
| errors = [] |
There was a problem hiding this comment.
You're resetting errors to an empty list here. Looking at the code above, I believe this is not the desired behavior...
There was a problem hiding this comment.
no, this line is a reliquat from previous code, it should go off, yes., ill update it.
There was a problem hiding this comment.
@s0undt3ch updated. nice catchup btw.
kiorky
force-pushed
the
p
branch
4 times, most recently
from
b0bc1dd
to
31063ef
Compare
|
Hi @kiorky I agree with the suggestion that this should not go into a point release. Could you please close this and re-submit against the develop branch? Thanks. |
cachedout
added
the
Pending-Discussion
|
@cachedout may i let you cherrypick & test this on develop, and do any additional fixup commit that i can review ? |
|
(To be clear, our fork is now based on 2015.8, for now) |
|
So tell me if you want from me to initiate the PR against develop or if you want to do it yourself. |
|
BTW, @cachedout @s0undt3ch after reflexion, Idea is now to clearly distinguish if this is a so-evil changeset or not, for me it's good, but indeed the more expert eyes, the better. |
|
Hi @kiorky I see what you're saying here. It's reasonable to classify the lack of error notification a bug, especially in a pillar as you mention. I will merge this on this branch. Thanks for the discussion. |
Bring up ext_pillar rendering errors as well
i dont know if that can go in 2015.8 considering the non trivial change.
Idea is to also bring the errors from ext_pillars inside the _errors subdict for pillar to bring errors as with static sls files.
Indeed, for now, the errors show up inside logs, but up then, as this is just a log, anything will be ignored, sls will execute and etc, even if the pillar is lacking critical information to run