add noop to variables_in_scope list

[sgran]

Hi,

We have a bunch of custom types that trigger lint warnings when we do:

sgran@gnm40439:~$ cat t.pp
define da () {
file { $title: noop => $noop }
}

da { '/tmp/test2': noop => true }

sgran@gnm40439:~$ puppet apply ~/t.pp
notice: Finished catalog run in 0.13 seconds

sgran@gnm40439:~$ puppet-lint ~/t.pp
WARNING: top-scope variable being used without an explicit namespace on line 2

Patch below:

--- a/puppet-lint/lib/puppet-lint/plugins/check_classes.rb 2012-03-28 13:35:14.000000000 +0100
+++ b/puppet-lint/lib/puppet-lint/plugins/check_classes.rb 2012-03-28 13:34:58.000000000 +0100
@@ -91,7 +91,7 @@
check 'variable_scope' do
(class_indexes + defined_type_indexes).each do |idx|
object_tokens = tokens[idx[:start]..idx[:end]]

•  variables_in_scope = ['name', 'title', 'module_name', 'environment', 'clientcert', 'clientversion', 'servername', 'serverip', 'serverversion', 'caller_module_name']
  

•  variables_in_scope = ['name', 'title', 'module_name', 'environment', 'clientcert', 'clientversion', 'servername', 'serverip', 'serverversion', 'caller_module_name', 'noop']
  

  referenced_variables = []
  header_end_idx = object_tokens.index { |r| r.first == :LBRACE }
  lparen_idx = object_tokens[0..header_end_idx].index { |r| r.first == :LPAREN }

Cheers,

[nanliu]

That looks wrong, shouldn't it be:

define da ($noop) {
  file { $name: noop => $noop }
}

[sgran]

Well, no. That would imply that you'd have to pass noop on all calls to da, which would be unusual. You could set ($noop = false), but then you get this:

steve@varinia:~$ puppet apply t.pp
warning: noop is a metaparam; this value will inherit to all contained resources

It seems to me that the puppet upstream considers noop a metaparam and special, and it's puppet-lint that is at fault for not recognizing that. It already has a list of special variables, after all - I'm just asking for one more to be added.

Cheers,

[nanliu]

Sorry, I've misread the original code, in this case you don't need to pass metaparameters noop => $noop, for example if you add require metaparameter the attribute should be added to all resources in the define.

define da {
  file { $name: 
    ensure => file,
  }
}

da { '/tmp/bar':
   noop => true, # you don't need to pass $noop or declare the attribute noop.
}

$ puppet apply da.pp
notice: /Stage[main]//Da[/tmp/bar]/File[/tmp/bar]/ensure: current_value absent, should be file (noop)

So I'm not sure noop should be added.

[sgran]

OK, I'll accept that as we're transitioning away from dynamically scoped variables, this one may be messy. I get warnings from puppet about dynamic scoping if I don't make it explicit, and warning from puppet-lint if I do. I'll live with it for now and revisit it once we're no longer using dynamic scoping.

Cheers,