FC020 triggered on non-ruby shell command

[eherot]

user user_to_delete do
  action :remove
  only_if "/usr/bin/id #{user_to_delete} > /dev/null"
  supports :manage_home => true
end

[eherot]

another example:

execute "add_jetty_group" do
  command "/usr/sbin/groupadd #{jetty_settings['group']}"
  not_if "/usr/sbin/groupmod #{jetty_settings['group']}"
end

[acrmp]

Hi Eric,

I'll have a look at this one as well at the same time. This rule by its nature is going to give some false positives at present - we may need to rethink it.

Cheers,

Andrew.

[mconigliaro]

Another one:

execute "net groupmap add ntgroup='Domain Admins' unixgroup='root' rid=512 type=d" do
  not_if "net groupmap list ntgroup='Domain Admins'"
end

[acrmp]

Thanks Mike.

The problem is that the not_if string here is syntactically correct Ruby. We need a rethink on how to solve this one - just adding special cases for os commands feels broken.

[eherot]

acrmp, and I suspect it wouldn't work since there is the occasional overlap. Certainly if a full path is specified at the beginning of a command, though, it should probably be considered an external command.

Presumably chef itself has to understand whether you're trying to type ruby or external shell commands. Is there any way to take advantage of that here?

[acrmp]

The reason this rule exists is to identify when you have done the wrong thing and passed a ruby expression in a string when it should be a block. Chef only knows which it is based on how you pass it in.

I like your idea of looking for a path though.

[mconigliaro]

Would it be easier to approach this problem from the other direction? i.e. eval() the string to test whether it's valid Ruby code?

[acrmp]

We could, but running eval on the string may have potentially harmful side effects.

Practically, we're also not running within the context of a Chef run so don't have access to variables declared by the user or Chef built-ins that may be referenced.

[mkocher]

This one has been biting me a lot recently, and what's odd to me is the direction of the rule. If you forget to make it a block, it's almost certain that your shell will blow up. Not desirable, but you'll know soon enough. If you surround a shell command with curly brackets, foodcritic will be happy, it'll be syntactically correct, and the block will return truthy every time. I'd love foodcritic to warn me if the only thing in a conditional ruby block is a string.

I've started writing my not_it shell commands as not_if { system("ls foo") } which is unfortunate but passes linting. (it breaks in user resources though, as the user provider defines a system method.)

[acrmp]

Hi Matthew,

That's a really interesting point. This is the scenario where you enclose the command in a string and a block. I can't think of a case where it would make sense for the only expression in the block to be a string - this looks like a good candidate for a new rule.

We'd want to match against any of these:

# expression is a string literal
not_if { "ls foo" }

# expression is a string with embedded sub-expressions
only_if { "ls #{node['foo']['path']}" }
not_if { "ls #{foo.method()}" }

We wouldn't match against these:

# expression is a method call
only_if { foo.bar }

# we wouldn't know that this expression evaluates to a string
not_if { foo.to_s }

# in theory we could pick up that the last expression in the block here
# is a string but I'd keep it simple for now
only_if { foo.bar; "ls foo" }

Does this look about right?

Cheers,

Andrew.

[mkocher]

That looks perfect. Would have certainly saved me time in the past.

[acrmp]

Hi guys,

I've pushed foodcritic 1.3.0 to rubygems which should make FC020 work for the specific examples given here. This still needs a more satisfactory solution though.

@mkocher The new rule described has been implemented as FC026.

Thanks,

Andrew.

[acrmp]

Hi guys,

I'm closing this issue as FC020 was removed in foodcritic 1.5.1 due to the number of false positives.