search

Foodcritic / foodcritic

Lint tool for Chef cookbooks.
http://foodcritic.io
MIT License
408 stars 153 forks source link

Fix node_method? to check chef_node_methods instead of chef_dsl_methods #793

Closed TravisWhitehead closed 5 years ago

TravisWhitehead commented 5 years ago

Edit: lamont-granquist pointed out that my original fix (described below) is wrong. The actual problem was that node_method? was checking chef_dsl_methods when it should have been chef_node_methods. This PR now fixes that method, removes an old workaround stemming from this bug from chef_dsl_metadata/Rakefile, and regenerates the chef_dsl_metadata/ files.

In #715 there is a false-positive FC019 occurring when using node.force_default. This PR proposes a solution that seems to prevent this false-positive, though based on the FIXME already present in the Rakefile I get the feeling that this may not be an ideal solution. This solution is inspired by the fix in #364 (which was addressing a similar bug in #181).

As I've discussed in #715, it seems that force_default must be present in the dsl_methods list in these generated chef_dsl_metadata/chef_X.Y.Z.json files, otherwise this false-positive FC019 occurs.

I would welcome some input from the Foodcritic community as to whether this is an appropriate fix, or if there's something better we can do.

lamont-granquist commented 5 years ago

so #181 should have fixed this. if there is a problem here it is that something is using chef_dsl_methods when it should be using chef_node_methods, which needs to get fixed. this seems pretty obviously the wrong way to fix this.

lamont-granquist commented 5 years ago

like why does def node_method? in lib/foodcritic/api use chef_dsl_methods and not chef_node_methods?

lamont-granquist commented 5 years ago

yeah, as far as i can tell when this got written only chef_dsl_methods existed:

https://github.com/Foodcritic/foodcritic/commit/935a712cee7bc93b129109da1f0503ac04819caf

the node_dsl_methods were introduced later and FC019 was never updated to use it:

https://github.com/Foodcritic/foodcritic/commit/6a776d969d4b84373635f4e4e60b40991a7c2a1e

the mistake of including the node methods in the dsl methods dates back to when that was originally created and seems to be a straight up thinko as far as i can see:

https://github.com/Foodcritic/foodcritic/commit/1685d977948888f888d935fc7ec827bee4e04321

so node_method? should be changed to use chef_node_methods, the Chef::Node should actually be removed from chef_dsl_methods, and then any remaining newly exposed issue should get cleaned up.

TravisWhitehead commented 5 years ago

Thank you for clarifying the source of the actual bug and the fix, as well as the quick response!

I've updated my branch with what I believe is the fix you described. It does resolve the false-positive that I was experiencing.

tas50 commented 5 years ago

Thanks @TravisWhitehead