FC020 triggered on non-ruby shell command #30

Closed
eherot opened this Issue May 9, 2012 · 13 comments

Comments

Projects
None yet
4 participants
@eherot

eherot commented May 9, 2012

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

This comment has been minimized.

Show comment Hide comment
@eherot

eherot May 11, 2012

another example:

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

eherot commented May 11, 2012

another example:

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

This comment has been minimized.

Show comment Hide comment
@acrmp

acrmp May 17, 2012

Member

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.

Member

acrmp commented May 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@mconigliaro

mconigliaro May 17, 2012

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

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

This comment has been minimized.

Show comment Hide comment
@acrmp

acrmp May 17, 2012

Member

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.

Member

acrmp commented May 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@eherot

eherot May 17, 2012

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?

eherot commented May 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@acrmp

acrmp May 17, 2012

Member

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.

Member

acrmp commented May 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@mconigliaro

mconigliaro May 17, 2012

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?

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

This comment has been minimized.

Show comment Hide comment
@acrmp

acrmp May 17, 2012

Member

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.

Member

acrmp commented May 17, 2012

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 comment has been minimized.

Show comment Hide comment
@mkocher

mkocher May 19, 2012

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.)

mkocher commented May 19, 2012

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

This comment has been minimized.

Show comment Hide comment
@acrmp

acrmp May 19, 2012

Member

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.

Member

acrmp commented May 19, 2012

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

This comment has been minimized.

Show comment Hide comment
@mkocher

mkocher May 19, 2012

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

mkocher commented May 19, 2012

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

@acrmp

This comment has been minimized.

Show comment Hide comment
@acrmp

acrmp May 21, 2012

Member

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.

Member

acrmp commented May 21, 2012

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

This comment has been minimized.

Show comment Hide comment
@acrmp

acrmp Aug 27, 2012

Member

Hi guys,

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

Member

acrmp commented Aug 27, 2012

Hi guys,

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

@acrmp acrmp closed this Aug 27, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment