warn if manifest has parameter called 'environment'

[alexjfisher]

To prevent issues like voxpupuli/puppet-letsencrypt#63

[igalic]

and theforeman/puppet-foreman#464

[igalic]

this is mainly about classes. (obviously?)
In functions and defined types it doesn't matter, unless they issue hiera() themselves.

[rnelson0]

Can someone explain why it's causing a failure? Is that a prohibited parameter name, and does puppet parser validate not catch that? Puppet-lint tried to only handle formatting style issues rather than syntax, so I want to make sure this is fixed in the correct project. If it is a style issue, then we need to link to or create a style guide reference for it. (It could always be a plugin but it would not be in core then)

[igalic]

if you have a hiera.yaml such as

# managed by puppet
---
:backends:
- yaml
- file

:logger: console

:hierarchy:
  - "fqdn/%{::fqdn}"
  - "roles/%{role}"
  - "vm_profiles/%{vm_profile}"
  - "vm_parents/%{vm_parent}"
  - repos
  - tlsdata
  - users
  - common

:yaml:
  :datadir: "/etc/puppetlabs/code/environments/%{::environment}/hieradata"

:file:
  :datadir: "/etc/puppetlabs/code/environments/%{::environment}/hierafiles"

:merge_behavior: deeper

then, as soon as puppet's execution flow enters a class which has a top-scope parameter called environment, hiera() calls (implicit or explicit) will start to using it:
If the environment defaults to undef, under --debug will you see that now your lookups go to /etc/puppetlabs/code/environments//hieradata/repos.yaml, or rather, fail to go.

[rnelson0]

Wow, that seems like a pretty serious bug! Has anyone opened a ticket with Puppet yet?

[pgassmann]

That means that local class parameter variables override global variables in hiera? That's bad! Looks like a Puppet or hiera lookup bug. Does this happen with all versions of puppet/hiera?

[igalic]

yup

[hlindberg]

The right interpolation to use here is ::environment to prevent this problem. The documentation at puppet has a warning about this as well.
All interpolated variables in a hiera.yaml should use :: prefix as they will otherwise (potentially) hit a local variable in calling scope. (There are legit use cases for that). Hiera has supported that for a long time. The lookup version in Puppet 4.9.0 will also support that (it does not do that now).

[hlindberg]

While I would not mind making $environment a reserved variable name it was not possible due to what seemed to be quite frequent use as class param.

[rnelson0]

@hlindberg It sounds like currently there is nothing to do for puppet-lint as the syntax exists currently - but a hiera-lint, if it existed, should check for hierarchy values that do not begin with $::. Correct?

We can try getting a style guide update added but that's only going to help for environment, not any other local variable/global var or fact conflicts. @igalic I think maybe this would be best for now as a standalone plugin module under voxpupuli namespace.

[hlindberg]

@rnelson0 correct.

[alexjfisher]

@igalic Am I missing something? You are already correctly using ::environment in your hiera.yaml
Despite this, you're clashing with a local variable environment from the calling scope?? This isn't supposed to happen, so there is a bug somewhere?

[hlindberg]

It is a bug for sure if interpolation of ::environment in normal operations like a compile or apply is giving you the value of a local variable from calling scope.

[rnelson0]

I'm going to close this out as it looks like a possible bug in puppet or hiera. Thanks, all!