From f29d4d489cce17c0c209bf8b7b51a02abcdee1d0 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Wed, 8 Jul 2020 17:13:22 -0700 Subject: [PATCH] Expand NodeInitPackage to catch more bad systemd tests Catch using test in a conditional. There's a small number of these on the supermarket. This also migrates the new 0.87+ corrector API, which allowed me to easily have 2 different kinds of offenses autocorrected. Very cool stuff. Signed-off-by: Tim Smith --- .../cop/chef/modernize/node_init_package.rb | 24 ++++++++++++------- .../chef/modernize/node_init_package_spec.rb | 22 +++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/lib/rubocop/cop/chef/modernize/node_init_package.rb b/lib/rubocop/cop/chef/modernize/node_init_package.rb index b2e8722b7..f2b47ee1d 100644 --- a/lib/rubocop/cop/chef/modernize/node_init_package.rb +++ b/lib/rubocop/cop/chef/modernize/node_init_package.rb @@ -27,18 +27,20 @@ module ChefModernize # ::File.open('/proc/1/comm').chomp == 'systemd' # File.open('/proc/1/comm').gets.chomp == 'systemd' # File.open('/proc/1/comm').chomp == 'systemd' - # File.exist?('/proc/1/comm') && File.open('/proc/1/comm').chomp == 'systemd' - # # IO.read('/proc/1/comm').chomp == 'systemd' # IO.read('/proc/1/comm').gets.chomp == 'systemd' # ::IO.read('/proc/1/comm').chomp == 'systemd' # ::IO.read('/proc/1/comm').gets.chomp == 'systemd' # File.exist?('/proc/1/comm') && File.open('/proc/1/comm').chomp == 'systemd' + # only_if 'test -f /bin/systemctl && /bin/systemctl' # # # good # node['init_package'] == 'systemd' + # only_if { node['init_package'] == 'systemd' } # - class NodeInitPackage < Cop + class NodeInitPackage < Base + extend RuboCop::Cop::AutoCorrector + MSG = "Use node['init_package'] to check for systemd instead of reading the contents of '/proc/1/comm'".freeze def_node_matcher :file_reads_proc_1_comm?, <<-PATTERN @@ -58,18 +60,24 @@ class NodeInitPackage < Cop :== (str "systemd")) PATTERN + def_node_matcher :file_systemd_conditional?, <<~PATTERN + (send nil? {:not_if :only_if} $(str "test -f /bin/systemctl && /bin/systemctl")) + PATTERN + def on_send(node) compare_init_system?(node) do # if there's a ::File.exist?('/proc/1/comm') check first we want to match that as well node = node.parent if node.parent&.and_type? && proc_1_comm_exists?(node.parent.conditions.first) - add_offense(node, location: :expression, message: MSG, severity: :refactor) + add_offense(node.loc.expression, message: MSG, severity: :refactor) do |corrector| + corrector.replace(node, "node['init_package'] == 'systemd'") + end end - end - def autocorrect(node) - lambda do |corrector| - corrector.replace(node.loc.expression, "node['init_package'] == 'systemd'") + file_systemd_conditional?(node) do |conditional| + add_offense(node.loc.expression, message: MSG, severity: :refactor) do |corrector| + corrector.replace(conditional.loc.expression, "{ node['init_package'] == 'systemd' }") + end end end end diff --git a/spec/rubocop/cop/chef/modernize/node_init_package_spec.rb b/spec/rubocop/cop/chef/modernize/node_init_package_spec.rb index 779e8e037..eb235f5a2 100644 --- a/spec/rubocop/cop/chef/modernize/node_init_package_spec.rb +++ b/spec/rubocop/cop/chef/modernize/node_init_package_spec.rb @@ -108,6 +108,28 @@ RUBY end + it "registers an offense with only_if 'test -f /bin/systemctl && /bin/systemctl'" do + expect_offense(<<~RUBY) + only_if 'test -f /bin/systemctl && /bin/systemctl' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use node['init_package'] to check for systemd instead of reading the contents of '/proc/1/comm' + RUBY + + expect_correction(<<~RUBY) + only_if { node['init_package'] == 'systemd' } + RUBY + end + + it "registers an offense with not_if 'test -f /bin/systemctl && /bin/systemctl'" do + expect_offense(<<~RUBY) + not_if 'test -f /bin/systemctl && /bin/systemctl' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use node['init_package'] to check for systemd instead of reading the contents of '/proc/1/comm' + RUBY + + expect_correction(<<~RUBY) + not_if { node['init_package'] == 'systemd' } + RUBY + end + it "does not register an offense when comparing a non-systemd value in '/proc/1/comm'" do expect_no_offenses(<<~RUBY) ::File.open('/proc/1/comm').gets.chomp == 'foo'