Merge pull request #8455 from chef/btm/MSYS-979

Fix for cron resource get confused by environment/property mismatch

2 files changed, 137 insertions, 22 deletions

diff --git a/lib/chef/provider/cron.rb b/lib/chef/provider/cron.rb
index 79245df475..00c04a73aa 100644
--- a/lib/chef/provider/cron.rb
+++ b/lib/chef/provider/cron.rb
@@ -32,6 +32,7 @@ class Chef
       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 98c525762b..6bfc18c359 100644
--- a/spec/unit/provider/cron_spec.rb
+++ b/spec/unit/provider/cron_spec.rb
@@ -690,33 +690,136 @@ describe Chef::Provider::Cron do
     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