FC035 - too broad a condition?

[miketheman]

While it is very common to use the node object directly from within templates it’s often clearer within a recipe to pass just the attributes that the template needs. It’s also good practice to perform any calculation on node attributes in the recipe rather than in the template erb.

In many complex cases, passing all known node attributes to the template may be overloading the recipe needlessly.

For instance, in https://github.com/opscode-cookbooks/ntp/blob/master/templates/default/ntp.conf.erb there are 15 invocations of node (9 distinct).

One could argue that placing 9 variables, including their logic to check whether they exist, in the recipe, bloats the code considerably. See: https://github.com/opscode-cookbooks/ntp/blob/master/recipes/default.rb#L38-44

I agree that performing complex calculation in the template is a bad idea, but for checking the existence of an attribute and performing an action is preferable to the recipe checking the conditional, passing a variable, and then the template checking for the conditional, and acting on that.

Here's an example of doing the bulk of the conditional in the template: https://github.com/DataDog/chef-datadog/blob/master/templates/default/datadog.conf.erb#L19-135

Duplicating all the conditional logic before it's passed as a variable template seems like clutter, and induces a second update per variable added.

[acrmp]

Hi Mike,

Thanks for the feedback. I did think that this rule might be controversial (there's a note to that effect in the CHANGELOG) and shipped it in order to elicit feedback.

• There is no specific requirement to separate each scalar out into its own variable. You could pass the equivalent of node['ntp'] to the template.
• The individual datadog template example looks like a candidate for being changed to loop over key value pairs and outputting whatever is passed in, rather than explicitly checking for the presence of known keys.

Cheers,

Andrew.

[miketheman]

Hey Andrew,

Thanks for the update - I probably should have checked the README. :) Controversial, indeed.

Point 1 makes sense - pass the main node object, then evaluate the sub-keys. Cool.

Point 2 - I don't know if I clearly understand this. Is there an example of this somewhere I could reference?

[acrmp]

Point 2 - this makes sense if you have a structure that is close to your config file format. In the simplest case I'm thinking of something like this:
https://github.com/fnichol/chef-rvm/blob/94842638f2c20933f18a751d6629c63058b49ec8/templates/default/rvmrc.erb#L14-16

If you are working with data structures provided by others then you might need to do some processing to use this approach, at which point it's a judgement call as to which approach results in simpler and easier to read code.

[kcbraunschweig]

My understanding is that in the opscode training the specifically say to avoid using template variable parameters to pass in node data but only to use this for other types of data like databags or other calculations. One reason for this is that the value of the node attribute will be read at compile-time when it is passed in. If ruby in a later cookbook changes that node attribute you care about before run-time when your template is rendered, you may not get the value you intended. If you directly access the node object in the template, you'll get the latest possible binding.

[miketheman]

An example of where this might be an issue is here: https://gist.github.com/3491505 Definitely a Bad Idea, but demonstrates the problem.

A node variable is set to one thing before the template resource is called, then set to something else after the resource.

[acrmp]

@kcbraunschweig - Great point, thanks for sharing this.
@miketheman - Thanks for the example.

Full disclosure: I have a nasty cold at the moment.

It seems a bit wrong to me to reference the node attribute everywhere because it might be modified elsewhere during the run. As a user of the cookbook I'd probably prefer if this was explicit.

If I was trying to delay the evaluation in plain old Ruby I might reach for a proc, which would look like this:

 file "/tmp/thing.erb" do
-  content '<%= node["mything"] %>'
+  content '<%= @foo.call %>'
 end

 template "/tmp/mything" do
   source "/tmp/thing.erb"
+  variables({'foo' => lambda{node["mything"]}})
   local true
 end

You could also take advantage of the fact that Proc#call can be called via square brackets to avoid the .call in the template. I'm not sure if this is really more readable though.

It seems at minimum we need a warning in the rule documentation on the difference in behaviour.

What do you guys think? I can certainly see that "just use node attributes everywhere" is a simpler message.

[atomic-penguin]

Hi Andrew, hope you feel better soon.

I ran into this recently while working on a backlog of patches for gitlab cookbook. Of course my Travis tests failed when it pulled in the gem with the FC035 rule. I went through and made the changes so I'd be better informed about what changes this rule entails.

The two reasons I have read for this rule in this thread are:

1. Guard compile-time evaluation of template variables by passing in explicit variables.
2. Principle of don't repeat yourself. An author could pass in a well designed struct, and loop over the data therefore reducing the overall complexity of the code, or template.

Number one may be a problem, in some cases. As @kcbraunschweig points out, it is a necessity of design, with the ability to override attributes properly. It seems like this is a guard for something that isn't really a problem, when node attributes are accessed directly.

Number two only works if the structure was designed in such a way. Frankly, I haven't seen many cookbooks with complex hash-like structures with the exception of a few like fnichol's chef-rvm which you pointed out as an example. The most obvious way to appease this foodcritic alert is by repeating yourself and declaring variable => attribute for all data used in any given template.

So, just in my own use case for gitlab, that adds about 40 lines of recipe code while not really reducing the complexity of any given template. Some of this variable declaration in recipes, must be repeated as attributes such as node['fqdn'] are used in almost every template to point out the file is generated for a given node. Possibly the most annoying thing about this alert is having to repeatedly declare instance variables for templates, just so one can reference commonly used Ohai attributes such as hostname or fqdn without triggering the alert. I think this is probably the more common case than a well designed hash-like structure to be looped over for most cookbooks.

+ 40 lines recipes/
- a few characters templates/

[acrmp]

Thanks for the considered feedback @atomic-penguin.

I think the consensus then is that this rule is not adding a lot of value for the trouble it causes. I'll release a new version with this rule removed shortly.

[miketheman]

Don't get me wrong - I think it may very well be that there is good value to using some sort of conditional style, such as any attribute that is not a node attribute should probably be pre-computed in the template resource.

Since defeating this rule is probably as simple as passing the entire node object to the template as a variable (not a good idea), maybe another approach to this rule is needed.

[acrmp]

I've pushed 1.6.0 to rubygems which is 1.5.1 with FC035 removed.

@miketheman - Can you open a separate issue with ideas for what this rule might look like so we can continue the discussion over there? Thanks.