From 447ea8f370b2bf9ae0e323d0cb293f6592dfe852 Mon Sep 17 00:00:00 2001 From: vijaymmali1990 Date: Mon, 25 Feb 2019 15:29:31 +0530 Subject: - Added a warning in case user is using a `environment` for an entry that can also be specified as a `property` - Added unit test cases - Ensured chefstyle - Added a warning in case user is using a `environment` for an entry that can also be specified as a `property` - Revert changes in order to pass chefstyle - Added a warning in case user is using a `environment` for an entry that can also be specified as a `property` - Added these changes in cron_different? method - Added unit test cases - Added some changes in cron.rb - It now throws error if user passes both environment with the {:SHELL, :HOME, :PATH, :MAILTO} and these properties itself. - It maintains idempotency. - Added Rspecs for this case. - Ensured chef-style. Signed-off-by: vijaymmali1990 --- lib/chef/provider/cron.rb | 22 +++++-- spec/unit/provider/cron_spec.rb | 143 ++++++++++++++++++++++++++++++++++------ 2 files changed, 140 insertions(+), 25 deletions(-) diff --git a/lib/chef/provider/cron.rb b/lib/chef/provider/cron.rb index f25a58a789..00c04a73aa 100644 --- a/lib/chef/provider/cron.rb +++ b/lib/chef/provider/cron.rb @@ -29,9 +29,10 @@ class Chef CRON_ATTRIBUTES = [:minute, :hour, :day, :month, :weekday, :time, :command, :mailto, :path, :shell, :home, :environment].freeze WEEKDAY_SYMBOLS = [:sunday, :monday, :tuesday, :wednesday, :thursday, :friday, :saturday].freeze - CRON_PATTERN = /\A([-0-9*,\/]+)\s([-0-9*,\/]+)\s([-0-9*,\/]+)\s([-0-9*,\/]+|[a-zA-Z]{3})\s([-0-9*,\/]+|[a-zA-Z]{3})\s(.*)/ - SPECIAL_PATTERN = /\A(@(#{SPECIAL_TIME_VALUES.join('|')}))\s(.*)/ - ENV_PATTERN = /\A(\S+)=(\S*)/ + CRON_PATTERN = /\A([-0-9*,\/]+)\s([-0-9*,\/]+)\s([-0-9*,\/]+)\s([-0-9*,\/]+|[a-zA-Z]{3})\s([-0-9*,\/]+|[a-zA-Z]{3})\s(.*)/.freeze + SPECIAL_PATTERN = /\A(@(#{SPECIAL_TIME_VALUES.join('|')}))\s(.*)/.freeze + ENV_PATTERN = /\A(\S+)=(\S*)/.freeze + ENVIRONMENT_PROPERTIES = %w{MAILTO PATH SHELL HOME}.freeze def initialize(new_resource, run_context) super(new_resource, run_context) @@ -192,7 +193,7 @@ class Chef private def set_environment_var(attr_name, attr_value) - if %w{MAILTO PATH SHELL HOME}.include?(attr_name) + if ENVIRONMENT_PROPERTIES.include?(attr_name) current_resource.send(attr_name.downcase.to_sym, attr_value.gsub(/^"|"$/, "")) else current_resource.environment(current_resource.environment.merge(attr_name => attr_value)) @@ -221,7 +222,18 @@ class Chef newcron << "#{v.to_s.upcase}=\"#{new_resource.send(v)}\"\n" if new_resource.send(v) end new_resource.environment.each do |name, value| - newcron << "#{name}=#{value}\n" + if ENVIRONMENT_PROPERTIES.include?(name) + unless new_resource.property_is_set?(name.downcase) + logger.warn("#{new_resource.name}: the environment property contains the '#{name}' variable, which should be set separately as a property.") + new_resource.send(name.downcase.to_sym, value.gsub(/^"|"$/, "")) + new_resource.environment.delete(name) + newcron << "#{name.to_s.upcase}=\"#{value}\"\n" + else + raise Chef::Exceptions::Cron, "#{new_resource.name}: the '#{name}' property is set and environment property also contains the '#{name}' variable. Remove the variable from the environment property." + end + else + newcron << "#{name}=#{value}\n" + end end if new_resource.time newcron << "@#{new_resource.time} #{new_resource.command}\n" diff --git a/spec/unit/provider/cron_spec.rb b/spec/unit/provider/cron_spec.rb index 03e3599b35..2377fe7dbc 100644 --- a/spec/unit/provider/cron_spec.rb +++ b/spec/unit/provider/cron_spec.rb @@ -690,33 +690,136 @@ CRONTAB end context "when there is a crontab with a matching and identical section" do - before :each do - @provider.cron_exists = true - allow(@provider).to receive(:cron_different?).and_return(false) - allow(@provider).to receive(:read_crontab).and_return(<<~CRONTAB) - 0 2 * * * /some/other/command + context "when environment variable is not used" do + before :each do + @provider.cron_exists = true + allow(@provider).to receive(:cron_different?).and_return(false) + allow(@provider).to receive(:read_crontab).and_return(<<~CRONTAB) + 0 2 * * * /some/other/command - # Chef Name: cronhole some stuff - * 5 * * * /bin/true + # Chef Name: cronhole some stuff + SHELL=/bash + * 5 * * * /bin/true - # Another comment -CRONTAB - end + # Another comment + CRONTAB + end - it "should not update the crontab" do - expect(@provider).not_to receive(:write_crontab) - @provider.run_action(:create) + it "should not update the crontab" do + expect(@provider).not_to receive(:write_crontab) + @provider.run_action(:create) + end + + it "should not mark the resource as updated" do + @provider.run_action(:create) + expect(@new_resource).not_to be_updated_by_last_action + end + + it "should log nothing changed" do + expect(logger).to receive(:trace).with("Found cron '#{@new_resource.name}'") + expect(logger).to receive(:trace).with("Skipping existing cron entry '#{@new_resource.name}'") + @provider.run_action(:create) + end end - it "should not mark the resource as updated" do - @provider.run_action(:create) - expect(@new_resource).not_to be_updated_by_last_action + context "when environment variable is used" do + before :each do + @provider.cron_exists = true + allow(@provider).to receive(:read_crontab).and_return(<<~CRONTAB) + 0 2 * * * /some/other/command + + # Chef Name: cronhole some stuff + SHELL=/bash + ENV=environment + 30 * * * * /bin/true + + # Another comment + CRONTAB + end + context "contains an entry that can also be specified as a `property`" do + before :each do + @new_resource.environment = { "SHELL" => "/bash", "ENV" => "environment" } + end + + it "should raise a warning for idempotency" do + expect(logger).to receive(:warn).with("cronhole some stuff: the environment property contains the 'SHELL' variable, which should be set separately as a property.") + @provider.run_action(:create) + end + + it "should not update the crontab" do + expect(@provider).not_to receive(:write_crontab) + @provider.run_action(:create) + end + + it "should not mark the resource as updated" do + expect(@new_resource).not_to be_updated_by_last_action + @provider.run_action(:create) + end + end + + context "contains an entry that cannot be specified as a `property`" do + before :each do + @new_resource.environment = { "ENV" => "environment" } + @new_resource.shell "/bash" + end + + it "should not raise a warning for idempotency" do + expect(logger).not_to receive(:warn).with("cronhole some stuff: the environment property contains the 'SHELL' variable, which should be set separately as a property.") + @provider.run_action(:create) + end + + it "should not update the crontab" do + expect(@provider).not_to receive(:write_crontab) + @provider.run_action(:create) + end + + it "should not mark the resource as updated" do + @provider.run_action(:create) + expect(@new_resource).not_to be_updated_by_last_action + end + end end - it "should log nothing changed" do - expect(logger).to receive(:trace).with("Found cron '#{@new_resource.name}'") - expect(logger).to receive(:trace).with("Skipping existing cron entry '#{@new_resource.name}'") - @provider.run_action(:create) + context "when environment variable is used with property" do + before :each do + @provider.cron_exists = true + allow(@provider).to receive(:read_crontab).and_return(<<~CRONTAB) + 0 2 * * * /some/other/command + + # Chef Name: cronhole some stuff + SHELL=/bash + ENV=environment + 30 * * * * /bin/true + + # Another comment + CRONTAB + end + + context "when environment variable is same as property" do + it "should throw an error" do + @new_resource.shell "/bash" + @new_resource.environment "SHELL" => "/bash" + expect do + @provider.run_action(:create) + end.to raise_error(Chef::Exceptions::Cron, /cronhole some stuff: the 'SHELL' property is set and environment property also contains the 'SHELL' variable. Remove the variable from the environment property./) + end + end + + context "when environment variable is different from property" do + it "should not update the crontab" do + @new_resource.shell "/bash" + @new_resource.environment "ENV" => "environment" + expect(@provider).not_to receive(:write_crontab) + @provider.run_action(:create) + end + + it "should not mark the resource as updated" do + @new_resource.shell "/bash" + @new_resource.environment "ENV" => "environment" + @provider.run_action(:create) + expect(@new_resource).not_to be_updated_by_last_action + end + end end end end -- cgit v1.2.1