Salt: 0.17.4: New conflict / duplicate detection breaks include / exclude

[ceralena]

We use a combination of jinja includes and proper salt include and exclude directives.

As recently as 0.17.2, this worked:

states/base/bar.tpl:

include:
  - pkgs.git

states/base/foo.sls:

{% include "bar.tpl" %}

include:
  - pkgs.mercurial

As of 0.17.4, these states fail like so:

vagrant@dev:~$ sudo salt-call state.sls foo
[INFO    ] Loading fresh modules for state activity
[INFO    ] Fetching file ** skipped **, latest already in cache 'salt://foo.sls'
[INFO    ] Fetching file ** done ** 'bar.tpl'
[CRITICAL] Rendering SLS "base:foo" failed: Conflicting ID "include"
local:
    Data failed to compile:
----------
    Rendering SLS "base:foo" failed: Conflicting ID "include"

Where this issue has existed in the past (many releases ago), I've seen the suggestion that we can all these include: directives and rely on the highstate. Unfortunately, this would mean that we can't use state.sls to install states selectively, nor during development. Is this still the canonical solution? Is this a deliberate side-effect of a recent change, e.g. https://github.com/saltstack/salt/issues/8053?

[ceralena]

Possibly related:

https://github.com/saltstack/salt/issues/7526

https://github.com/saltstack/salt/issues/9390

[basepi]

I haven't actually tested this yet, but I'm thoroughly surprised this ever worked. After the jinja executes, you would end up with the following:

include:
  - pkgs.git

include:
  - pkgs.mercurial

This is, as far as I can tell, bad YAML. It would overwrite the first include with the second. Whether we detect this and return a conflicting ID or not, I feel like in either case it would never execute pkgs.git. Have you seen different behavior?

For reference:

>>> yaml.load('include:\n    foo\n\n\n\ninclude:\n    bar')
{'include': 'bar'}

[thatch45]

So this is rendering correctly, and the error is correct, so this is clearly not a bug, for the reasons described by @basepi. but we could add into the yaml loader the magic to reconcile this into a single include list if detected, the question is, should we do it?

[basepi]

I vote no. More magic is not the right answer -- with a small tweak (removing the include: part of your tpl file) you could easily combine those includes, even with the jinja there. I think that's the best answer.

[thedrow]

I'd vote yes but salt simply does not work that way so -1 from me.

[kmwhite]

I have to agree with @basepi on this one. As a user, less magic is better, imho. Instead of templating, one could also include a set of common includes. Such as:

include:
  - pkgs.common #has the contents of bar.tpl
  - pkgs.mercurial

Minor change to your formulas and still should work, no? Worked in my tests... The tools to do this are already there..

[thatch45]

ok, no it is! Thanks for the feedback. We do SOME yaml magic, but we try to keep it at a minimum :)

[clinta]

In regards to the surprise to how this ever worked, it's documented as working here: https://github.com/saltstack/salt/issues/2310#issuecomment-9685688

So apparently some magic was removed. If magic isn't desired, then can it be possible to assign a unique ID to an include? Something like:

include_one:
  include:
    - one

include_two:
  include:
    - two

Edit, I just realized that I was only getting conflict errors because I had two include lines in one sls. Once I fixed that, it appears that merging includes as indicated in the post I linked still works. But I think the comment is good for anyone finding this via google like me. There is magic for includes that merges them, just not when there's two includes in the same sls, which is what DanielBryan had.