To namespace, or not to namespace, that is the question

[Micah Anderson]

Hello, puppet users

Reviewing a friend's puppet module, puppet-lint shows he didn't use the explicit namespace on variables. He told me that he used the Style Guide[1] and the Beginner's Guide to Modules[2] which are a little unclear about the right way to do this, so I wanted to write to clarify this issue.

The Guide to Modules describes using a params.pp file and having a class inherits directive to pass variables to the anchored classes, like in the puppetlabs' ntp module. 

The style guide says the following: "You must scope all variables except for local or inherited variables. Scope inherited variables, when appropriate, for clarity. You should not mask/shadow inherited variables." We interpreted this paragraph differently: he says because they are inherited it is not needed to scope the variables, but I say that for clarity they should be scoped. If it is better to scope them all the time, why is there the possibility of using not scoped variables with class inheritance in the first place? 

I've seen people on #puppet, and on this list, say that every variable should be scoped for consistency and clarity, but this isn't explicitly said in those guides. In this case, variables aren't being mixed from different scopes, so there isn't a clarity issue. In fact the argument is that if you explicitly scope every variable, but there is no other module where variables are coming from, then you need to read every variable to check to see if there is a variable coming from another scope (::module1::foo vs. ::module2:bar), but if everyone has the same scope then you are unnecessarily reading more things to check for another scope, when there isn't one. Technically, you can just use the variable short name when using 'inherits'... but its frowned on, but that frowning isn't detailed anywhere in the style guides or docs.

https://docs.puppetlabs.com/guides/scope_and_puppet.html#qualify-your-variables says: Whenever you need to refer to a variable in another class, give the variable an explicit namespace: instead of simply referring to $packagelist, use $git::core::packagelist. This is a win in readability — any casual observer can tell exactly where the variable is being set, without having to model your code in their head — and it saves you from accidentally getting the value of some completely unrelated $packagelist variable. For complete clarity and consistency you will probably want to do this even when it isn’t absolutely neccessary.

This last sentence seems to implore you to do it when its not necessary because it is more clear and consistent everywhere, but this is only really detailed in the context of setting variables and dynamic scope, which isn't there anymore, modules are different as they are always inside the class.

So, clarification needed only when you are calling it from another place, or clarification everywhere (eg. for someone else who is reading the code)? 

More confusing is that it seems like these types of params modules are sometimes discouraged on this list, although the guides seem to use these params and inherits models (and this style is often used by the puppetlabs modules). So should we avoid this type of module, or not? Can we get a more clarity around this issue?

If the best practices is actually different from what these guides show, then why aren't these updated to reflect that? Personally, because inheritance is tricky, dynamic scoping was problematic, and namespace changes have been hard to keep track of over time, I like to namespace everything... but I've been doing this for a while. New people might not think this is such a big deal now.

According to this puppet-lint issue it should not use inherited variables but then scope them: https://github.com/rodjek/puppet-lint/issues/304

As it works in both cases, We wonder which should be the default way of writing puppet modules.

The controversial part is related to the following code:

    

# init.pp    

class myservice (

  $service_variable = $myservice::params::service_variable

) inherits myservice::params {

  anchor { 'myservice::begin': } ->

  class { '::myservice::install': } ->

  anchor { 'myservice::end': }

}

# params.pp

class myservice::params {

  $service_variable = 'variable'

}

# install.pp

class myservice::install inherits myservice {

  file { $service_variable:

      ensure => 'file'

  }

}

Should $service_variable be changed to $myservice::params::service_variable (or even $::myservice::params::service_variable) If that's the case, can we update the Guide to Modules or give more clarity to the style guide?

[1]: https://docs.puppetlabs.com/guides/style_guide.html

[2]: https://docs.puppetlabs.com/guides/module_guides/bgtm.html

[jcbollinger]

On Thursday, February 18, 2016 at 4:03:32 PM UTC-6, Micah Anderson wrote:

Hello, puppet users

Reviewing a friend's puppet module, puppet-lint shows he didn't use the explicit namespace on variables. He told me that he used the Style Guide[1] and the Beginner's Guide to Modules[2] which are a little unclear about the right way to do this, so I wanted to write to clarify this issue.

The Guide to Modules describes using a params.pp file and having a class inherits directive to pass variables to the anchored classes, like in the puppetlabs' ntp module. 

We had another discussion here, just a few days ago, about the BGTM and its recommendations about inheritance.  It didn't address the specific questions you raise here, but it does touch on related ones.  It may be worth a read.  Especially so because the discussion called out something I would consider a significant flaw in that document's recommendations, and that bears on you question.

 

The style guide says the following: "You must scope all variables except for local or inherited variables. Scope inherited variables, when appropriate, for clarity. You should not mask/shadow inherited variables."

I have no problem with the recommendation to scope inherited variables for clarity, though I guess your question comes down to the "when appropriate" qualifier.  With respect to shadowing, the guide is somewhat out of touch with common practice as it pertains to the Params Class pattern, in conjunction with which it is entirely conventional for class parameters to shadow variables of an inherited class.  As far as shadowing inherited variables goes in other contexts, I don't object to the Guide's recommendation, but I think it is largely moot if you follow good conventions for the (non-)use of class inheritance.

 

We interpreted this paragraph differently: he says because they are inherited it is not needed to scope the variables, but I say that for clarity they should be scoped.

You are both right, of course.  Supposing that you follow the Guide's recommendation to avoid shadowing inherited variables, you don't need to qualify their names when you reference them.  It nevertheless does make your code clearer to qualify them.  This therefore boils down to a question of style, and the style guide you referenced (which is by no means authoritative) is not helpful there, in a narrow sense.  If this will be a matter of friction in your shop then you would be well advised to start your own local Puppet style document, either as an addendum to the one you referenced, or even as a complete replacement.

The bigger problem, as I see it, is that you are making any significant use of class inheritance in the first place.  The Guide is flawed and runs counter to prevailing community opinion and to the Style Guide in recommending such a practice except for implementing the Params Class pattern, and maybe for enabling resource overrides.  I talk more about these in the other thread I linked.

 

If it is better to scope them all the time, why is there the possibility of using not scoped variables with class inheritance in the first place? 

Because that's the way class inheritance has always worked.  It is an old feature, dating from a time when Puppet was less structured, and common practices were different and less developed.  Use of class inheritance fell out of favor for most purposes in Puppet 3, and I suspect that the main reason that it was retained at all in Puppet 4 was the common use of the Params Class pattern, which depends on it.

 

I've seen people on #puppet, and on this list, say that every variable should be scoped for consistency and clarity, but this isn't explicitly said in those guides.

So?  A guide, by nature, is not authoritative.  That a rule or practice is not documented in a particular guide does not mean it is a poor idea.  In fact, that a rule or practice is documented in a particular guide does not necessarily mean it's a good idea.  The BGTM is a useful starting point, but I would not advise taking it as an ending point.  I personally take issue with some aspects of it.

 

In this case, variables aren't being mixed from different scopes, so there isn't a clarity issue.

It depends how technical you want to be, and exactly what you mean by that.  If you refer to Puppet's scoping rules, you will find that indeed inherited variables do belong to a different scope than the local one into which they are inherited.  That's why their namespace qualifier is different from that for variables declared in the local scope.  Class inheritance creates a parent / child relationship between two scopes, so inherited variables are "in scope" -- accessible without qualification -- but they do belong to a different scope than the local one.

Whether you consider that to create a clarity problem is up to you, but I think it does.

 

In fact the argument is that if you explicitly scope every variable, but there is no other module where variables are coming from, then you need to read every variable to check to see if there is a variable coming from another scope (::module1::foo vs. ::module2:bar), but if everyone has the same scope then you are unnecessarily reading more things to check for another scope, when there isn't one. Technically, you can just use the variable short name when using 'inherits'... but its frowned on, but that frowning isn't detailed anywhere in the style guides or docs.

Well, the style guide does say

Generally, inheritance should be avoided when alternatives are viable.

and

Class inheritance should only be used for myclass::params parameter defaults. Other use cases can be accomplished through the addition of parameters or conditional logic.

 
If you adhere to that then for the most part it moots the question.  I account the fact that the BGTM suggests a more permissive approach to class inheritance as a flaw in the BGTM.

https://docs.puppetlabs.com/guides/scope_and_puppet.html#qualify-your-variables says: Whenever you need to refer to a variable in another class, give the variable an explicit namespace: instead of simply referring to $packagelist, use $git::core::packagelist. This is a win in readability — any casual observer can tell exactly where the variable is being set, without having to model your code in their head — and it saves you from accidentally getting the value of some completely unrelated $packagelist variable. For complete clarity and consistency you will probably want to do this even when it isn’t absolutely neccessary.

This last sentence seems to implore you to do it when its not necessary because it is more clear and consistent everywhere, but this is only really detailed in the context of setting variables and dynamic scope, which isn't there anymore, modules are different as they are always inside the class.

That document is pretty old -- it's about how, in Puppet 2.7, to prepare for scoping changes scheduled for Puppet 3.  The latest release is Puppet 4.3.  In any case, it is unreasonable to interpret those comments to be about dynamic scope when they are drawn from a discussion of how to prepare for the static scoping system that was then about to be introduced.

So, clarification needed only when you are calling it from another place, or clarification everywhere (eg. for someone else who is reading the code)? 

More confusing is that it seems like these types of params modules are sometimes discouraged on this list, although the guides seem to use these params and inherits models (and this style is often used by the puppetlabs modules). So should we avoid this type of module, or not? Can we get a more clarity around this issue?

I am not aware of the params class pattern getting poor press here any time recently.  If it ever did in the past, then I'm one of the more likely people to have made such comments, and the worst I think you could induce me to say about the matter now is that acceptance of that pattern is a fait accompli.  There are alternatives beginning to surface, some of which probably are superior, but at this point I don't think it is reasonable to actively discourage its use.

What you may have seen discouraged is use of class inheritance for any purpose other than implementing the params class pattern, in its strict form.  Do understand that the point of the params class pattern, and the necessity of using class inheritance in implementing it, is entirely bound up in the fact that params class variables are intended for use as default values of class parameters.  In a faithful implementation of the pattern, variables of the params class are not used for any other purpose.

 

If the best practices is actually different from what these guides show, then why aren't these updated to reflect that?

Because PL has only limited resources, because they cannot be aware of everything all the time, and because best practices are neither universally agreed nor constant over time.  If you would like PL to devote some attention to these -- for instance to inconsistencies between the BGTM and the Style Guide -- then you can improve the likelihood that it will happen sooner rather than later by filing a ticket against the docs.

 

Personally, because inheritance is tricky, dynamic scoping was problematic, and namespace changes have been hard to keep track of over time, I like to namespace everything... but I've been doing this for a while. New people might not think this is such a big deal now.

According to this puppet-lint issue it should not use inherited variables but then scope them: https://github.com/rodjek/puppet-lint/issues/304

If you read the comments on that issue, there seems to be some uncertainty about its validity.  In any case, puppet-lint is not a PL product.  Its authors' view of appropriate style is not necessarily 100% aligned with PL's.

 

As it works in both cases, We wonder which should be the default way of writing puppet modules.

The controversial part is related to the following code:

    

# init.pp    

class myservice (

  $service_variable = $myservice::params::service_variable

) inherits myservice::params {

  anchor { 'myservice::begin': } ->

  class { '::myservice::install': } ->

  anchor { 'myservice::end': }

}

# params.pp

class myservice::params {

  $service_variable = 'variable'

}

# install.pp

class myservice::install inherits myservice {

  file { $service_variable:

      ensure => 'file'

  }

}

Should $service_variable be changed to $myservice::params::service_variable (or even $::myservice::params::service_variable) If that's the case, can we update the Guide to Modules or give more clarity to the style guide?

Class myservice::install should be modified so that it does not inherit from class myservice.  It may then reference $myservice::service_variable by its qualified name, or it may declare a class parameter by which the variable's value is provided to it.  If the latter alternative is chosen, then that implies that the class is private to the module -- it should not be declared directly by code outside the module.

Note, too, that this is not only a stylistic point.  If class myservice::install inherits from class myservice (or declares it, or relies upon it to have been declared)  then it is impossible to declare myservice::install without also declaring everything else in myservice.  That may present a problem in practice, depending on the nature and details of the module.


John