Clarify why this check is needed

[natemccurdy]

edit: Leading double colons are only needed on Puppet < 4.0.0 and/or when not using parser=future. https://tickets.puppetlabs.com/browse/PUP-121

It's important to point out that this check is only really needed under one circumstance. In all other circumstances it is perfectly acceptable to not use :: when declaring a class.

The only time that a class should be fully scoped is when the containing class's last name-segment matches the class being declared. For example:

Wrong Way

# This will fail because Puppet will be unable to find the wordpress class.
class profile::wordpress {
  include wordpress
}

Correct Way

# This will work, and the double colons are why it works.
class profile::wordpress {
  include ::wordpress
}

[jyaworski]

I was actually bitten by this once, in production. It was embarrassing.

[rnelson0]

It should also be note that as of puppet 4.something (not sure if 4.0.0 or sometime after), puppet only uses absolute class names. include apache will always include the apache class and never profile::apache or similar.

[liamjbennett]

I can't speak for puppet 4 but the following presentation also suggests that there are performance gains to be had in using the absolute namespace

https://www.slideshare.net/mobile/ripienaar/puppet-performance-profiling

[nibalizer]

@natemccurdy would you be willing to patch the readme with the 'why this check is needed?' Otherwise I feel like there is consensus on it being needed and why and I'm inclinded to resolve the issue

[natemccurdy]

@nibalizer Sure thing. Before I do though, I've got to double check on which versions of Puppet this is necessary.

At first pass, the consensus is that ::class_name is only needed on Puppet < 4.3'ish.

[reidmv]

In Puppet 4.0.0 and newer, the old relative namespacing for classes has been called out as a bug and fixed. See https://tickets.puppetlabs.com/browse/PUP-121. This means there is no need or reason to

include ::class_name

Doing that should in fact be discouraged as it provides no benefit, and becomes unnecessary mental noise. In the future parser and Puppet ≥ 4.0.0, it is correct to use

include class_name

Using an absolute namespace operator when referencing a class name is a workaround for undesirable behavior and is only necessary in versions of Puppet that had this bug. Namely, puppet < 4.0.0 using the old parser.

[wyardley]

Now that Vox modules are mostly 4.x only, should there be a version of this check that only requires it in places where it might be actually needed (variables?)

I personally prefer the appearance of

include foo

vs

include '::foo'

While I've already spent a lot of time resolving these warnings in the past, since it seems pretty clear that with Puppet >=4, it's not really necessary, I feel like either this check should be made looser (requiring it only in places where it might actually resolve ambiguity), or else we should just stop using it.

Some discussion in:
voxpupuli/modulesync_config#369

[reidmv]

In modern Puppet (≥4.0.0) is is MORE CORRECT to use

include foo

and LESS CORRECT to use

include '::foo'

The linter should (maybe) not penalize include '::foo', but it definitely shouldn't encourage it.

I would be strongly in support of removing it as a default check, or even removing it entirely.

[ffrank]

Can we go so far as to ERROR on Puppet 3 when :: is missing and WARN on Puppet 4+ when the :: is used?

[natemccurdy]

@ffrank As per my original comment, I think that erroring on Puppet 3 when :: is missing is misleading. It's really not a pattern that people should've ever needed to follow but they had to because of a bug.

The bug is that on Puppet 3, including bar from a class called foo::bar would fail.

[ekohl]

It looks like there are 4 options:

• conservative: always require leading colons (implemented now)
• current: require leading colons on puppet 3, ignore on puppet 4+
• modern: only care about puppet 4+ and warn about leading colons
• strict: only care about puppet 4+ and error on leading colons

Right now I consider most users are still in the early/mid stages of dropping Puppet 3. They may have some Puppet 3 somewhere and having one consistent style that works everywhere is a good thing. That means for now I think the current conservative behaviour should be the default but it would be good to have the options configurable for those who want to move to modern standards.

[rnelson0]

I think it makes most sense to simply only use an absolute classname check with puppet 3, and users on puppet 4+ should remove this plugin entirely. The plugin can't really detect this from the code, since it's just a linter, and doing something like checking the gem/bundle/system to determine what puppet is in use adds a dependency that isn't there, and may not match the version in use on the actual target systems anyway.

As such, I don't see any real need for the plugin in modern puppet development, and development can be frozen at the current status.

[ekohl]

@rnelson0 that makes sense. It also makes sense to build a new plugin that does the strict checking so Puppet 4+ users can clean up again but I don't have time for that.

[ffrank]

Ugh, good point. Here I was somehow assuming that puppet-lint relied on Puppet for the lexing. But no. It comes with its own lexer. Kudos to @rodjek, but it's definitely true that we cannot make any assumptions about the version of Puppet that a given piece of manifest code has been written for.

As such, I agree with @rnelson0 - we should just be very explicit in telling users of Puppet 4+ that they should not install this plugin.

[wyardley]

fwiw, puppet-lint already checks variables for top scope namespace I think (even when this gem isn't installed)

% puppet-lint xyz.pp 
WARNING: top-scope variable being used without an explicit namespace on line 117
WARNING: top-scope variable being used without an explicit namespace on line 121
%  gem list | grep puppet-lint
puppet-lint (2.3.0)
puppet-lint-classes_and_types_beginning_with_digits-check (0.1.2)
puppet-lint-leading_zero-check (0.1.1)
puppet-lint-trailing_comma-check (0.3.2)
puppet-lint-unquoted_string-check (0.3.0)
puppet-lint-variable_contains_upcase (1.2.0)
puppet-lint-version_comparison-check (0.2.1)

[rnelson0]

Yes, that's done here for variables that do not match a parameter/variable in the current class.

I've added a note in #13 about when you should use this plugin. I hope this addresses the concerns about why this plugin exists. If so, I think w e can close this out now.

[natemccurdy]

Thanks for making that change @rnelson0 👍

I'm happy with closing this ticket now that your patch is merged in.