State sls must allow include from any environment allowed in top.sls

[tsclausing]

Currently, the include declaration in a state sls file only allows includes from within the same environment: see here. This means that the only way to include files from environment B in environment A is to add environment B's file root to environment A's file roots.

Instead of having to include cross environment via file_roots, the include declaration should just respect environments from top.sls.

I've attempted a fix here:
tsclausing@ba8436a
tsclausing@7ae8f76 <- backwards compatible
tsclausing@213f976 <- more efficient

Looking forward to your feedback. This is my first attempt to contribute, so please be gentle :)

[thatch45]

this does look good, just to be sure, this automatically checks for top files in the declared environments that are included? I am working through the logic and think that this might not be a good idea, unless you are allowing for the top files to be statically assigned to specific environments

[tsclausing]

I'm probably just not familiar enough with the project yet, but I'm confused when you say "top files." I am only familiar with creating a single top file in the base file root. Is this not always the case?

[thatch45]

Ahh, I see my misunderstanding, I thought you were including top files, (which you can do from the main top file)
So I guess my question is still on syntax, when you include an sls file from another environment than the one the sls is in, how is that specified via this patch?

[tsclausing]

There should be no change in syntax. Let's see if I can put together an example:

/etc/salt/master

file_roots:
  base:
    - /srv/salt/base
  dev:
    - /srv/salt/dev
  prod:
    - /srv/salt/prod

/srv/salt/base/top.sls

base:
  '*':
    - core
dev:
  'dev-web*':
    - webserver
prod:
  'prod-web*':
    - webserver

salt:// file server tree

base/top.sls
base/core.sls
base/role/webserver.sls << to be included/extended by role based environments:
dev/webserver.sls
prod/webserver.sls

With this patch, the webserver.sls file in each role environment can include role.webserver from the base environment because base matches on '*'.

include:
  - role.webserver

# extend states from base: role.webserver
# ...

# add new role/environment specific states
# ...

Now, add dev/role/webserver.sls to the file server tree and the include:[role.webserver] in dev/webserver.sls will match it instead of base/role/webserver.sls. That's the fix for backwards compatibility (always match the current environment). In any other case, discovering multiple matches for the include in outside environments will throw the "Ambiguous include: ..." error. Minions matching the prod environment will not see the new dev/role/webserver.sls because they are excluded from that environment in top.sls - they will continue to pull base/role/webserver.sls.

I hope that makes sense. Thank you for taking the time to discuss this. Did that answer the syntax question?

[tsclausing]

This is cleaned up a bit and more efficient: tsclausing@213f976

[thatch45]

I like the capability, but I fear that this will make people start
including things from other environments without explicitly wanting to,
could be update it so that the include would look like this:

include:

• base: role.webserver

So the the environment is explicit? I understand where you are going here,
bt the given syntax really makes me worry about unintentional environment
bleeding.

On Fri, Nov 23, 2012 at 10:57 PM, T. Scot Clausing <[email protected]

wrote:

This is cleaned up a bit and more efficient: tsclausing/salt@213f976tsclausing@213f976

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2683#issuecomment-10674299.

[tsclausing]

I really don't like the idea of - environment: sls.to.include since explicitly stating the environment breaks the intended goal of having sls.to.include be resolved dynamically. Instead, what about a syntax that declared the users intention without being restrictive ... something like:

include:
  - sls.in.current.env
  - xenv: sls.to.resolve

Where - xenv: entries would be dynamically resolved based on matched states, others would match in the current env only. This way, the user is being explicit about their intention, but still allowing the include to be dynamic.

OR

Maybe we can do both. include:

• - env: sls.to.include for explicit environment (fail if env not in matches)
• - _xenv: sls.to.resolve to resolve across environments (fail if sls not in matches or ambiguous)

Thoughts?

[thatch45]

I like the both approach, explicit declaration and dynamic allowance, awesome!

[tsclausing]

New syntax in commit tsclausing@1fa275f allows the following:

include:
  - foo
  - env: bar
  - _xenv: baz

• foo will include foo.sls from the current environment
• bar will include bar.sls from the "env" environment
• baz will include baz.sls from the minion's set of matched environments

Look good? I'll need to update the corresponding docs as well if we decide to move forward.

[thatch45]

Looks awesome, do you think that we need the underscore before the xenv?

[tsclausing]

The underscore is an attempt to follow the convention of _grains/, _modules/, etc - which are "special" names in environments. Since the leading underscore convention implies specialness, users are presumably less likely to create an environment named _xenv while creating an xenv environment is perfectly reasonable.

Is this a good line of thinking, or is there something else to consider?

[thatch45]

This is a good line of thinking! Very nice! Thanks!

[dmargrave]

doesn't actually work. it chokes if you feed it an unordered dict (i.e. env: value or _xenv: value)

[DEBUG ] Results of YAML rendering:
OrderedDict([('include', [OrderedDict([('a', 'tarballs')]), OrderedDict([('a', 'formulae')])]), ('a', OrderedDict([('E@.salt-minion.', [OrderedDict([('match', 'compound')]), 'tarballs', 'formulae/mongodb', 'formulae/mongodb/server'])]))])
[DEBUG ] Loaded no_out as virtual quiet
[DEBUG ] Loaded json_out as virtual json
[DEBUG ] Loaded yaml_out as virtual yaml
[DEBUG ] Loaded pprint_out as virtual pprint
local:

Data failed to compile:

Traceback (most recent call last):

File "/usr/lib/python2.7/dist-packages/salt/state.py", line 2483, in call_highstate
top = self.get_top()
File "/usr/lib/python2.7/dist-packages/salt/state.py", line 2036, in get_top
tops = self.get_tops()
File "/usr/lib/python2.7/dist-packages/salt/state.py", line 1936, in get_tops
for sls in fnmatch.filter(self.avail[saltenv], sls_match):
File "/usr/lib/python2.7/fnmatch.py", line 50, in filter
if not pat in _cache:
TypeError: unhashable type: 'OrderedDict'

[icanhazpython]

Seems like there has been a regression. I'm using 2015.5.5, however the issue seems to be present even in the development branch. @tsclausing had made an edit to state.py (1fa275f) mentioned above back in 2012. It looks like some part of that commit made it into the present-day state.sls. On the 2015.5.5 tag, state.sls file line 2593, you can see part of his code. However it seems to have been pushed down, in that line 2578 is executed first, which fails with the error seen there if the _xenv dict key is used in the include. It looks like the order of that check would need to be shuffled around to allow the _xenv option to work again.