Merge pull request #2252 from opscode/jdmundrawala/env-splitjdmundrawala/11-env-split

Modified env resource to break values up by delimiter before comparing

3 files changed, 103 insertions, 23 deletions

diff --git a/lib/chef/provider/env.rb b/lib/chef/provider/env.rb
index e857d74d68..600cac1e6b 100644
--- a/lib/chef/provider/env.rb
+++ b/lib/chef/provider/env.rb
@@ -58,20 +58,22 @@ class Chef
       # ==== Returns
       # <true>:: If a change is required
       # <false>:: If a change is not required
-      def compare_value
+      def requires_modify_or_create?
         if @new_resource.delim
           #e.g. check for existing value within PATH
-          not @current_resource.value.split(@new_resource.delim).any? do |val|
-            val == @new_resource.value
+          not new_values.all? do |val|
+            current_values.include? val
           end
         else
           @new_resource.value != @current_resource.value
         end
       end
 
+      alias_method :compare_value, :requires_modify_or_create?
+
       def action_create
         if @key_exists
-          if compare_value
+          if requires_modify_or_create?
             modify_env
             Chef::Log.info("#{@new_resource} altered")
             @new_resource.updated_by_last_action(true)
@@ -91,13 +93,14 @@ class Chef
       #           after we removed the element.
       def delete_element
         return false unless @new_resource.delim #no delim: delete the key
-        if compare_value
+        needs_delete = new_values.any? { |v| current_values.include?(v) }
+        if !needs_delete
           Chef::Log.debug("#{@new_resource} element '#{@new_resource.value}' does not exist")
           return true #do not delete the key
         else
           new_value =
-            @current_resource.value.split(@new_resource.delim).select { |item|
-            item != @new_resource.value
+            current_values.select { |item|
+              not new_values.include?(item)
           }.join(@new_resource.delim)
 
           if new_value.empty?
@@ -122,7 +125,7 @@ class Chef
 
       def action_modify
         if @key_exists
-          if compare_value
+          if requires_modify_or_create?
             modify_env
             Chef::Log.info("#{@new_resource} modified")
             @new_resource.updated_by_last_action(true)
@@ -142,11 +145,23 @@ class Chef
 
       def modify_env
         if @new_resource.delim
-          #e.g. add to PATH
-          @new_resource.value << @new_resource.delim << @current_resource.value
+          values = new_values.reject do |v|
+            current_values.include?(v)
+          end
+          @new_resource.value((values + [@current_resource.value]).join(@new_resource.delim))
         end
         create_env
       end
+
+      # Returns the current values to split by delimiter
+      def current_values
+        @current_values ||= @current_resource.value.split(@new_resource.delim)
+      end
+
+      # Returns the new values to split by delimiter
+      def new_values
+        @new_values ||= @new_resource.value.split(@new_resource.delim)
+      end
      end
   end
 end
diff --git a/spec/unit/provider/env/windows_spec.rb b/spec/unit/provider/env/windows_spec.rb
index 2ea137c1d9..84582d8b4d 100644
--- a/spec/unit/provider/env/windows_spec.rb
+++ b/spec/unit/provider/env/windows_spec.rb
@@ -53,7 +53,7 @@ describe Chef::Provider::Env::Windows, :windows_only do
       end
 
       it "should update the ruby ENV object when it updates the value" do
-        provider.should_receive(:compare_value).and_return(true)
+        provider.should_receive(:requires_modify_or_create?).and_return(true)
         new_resource.value("foobar")
         provider.action_modify
         expect(ENV['CHEF_WINDOWS_ENV_TEST']).to eql('foobar')
@@ -92,7 +92,7 @@ describe Chef::Provider::Env::Windows, :windows_only do
       end
 
       it "replaces Windows system variables" do
-        provider.should_receive(:compare_value).and_return(true)
+        provider.should_receive(:requires_modify_or_create?).and_return(true)
         provider.should_receive(:expand_path).with(system_root).and_return(system_root_value)
         provider.action_modify
         expect(ENV['PATH']).to eql(system_root_value)
diff --git a/spec/unit/provider/env_spec.rb b/spec/unit/provider/env_spec.rb
index 0bc5117e48..f8803f9bb6 100644
--- a/spec/unit/provider/env_spec.rb
+++ b/spec/unit/provider/env_spec.rb
@@ -88,20 +88,20 @@ describe Chef::Provider::Env do
 
     it "should check to see if the values are the same if the key exists" do
       @provider.key_exists = true
-      @provider.should_receive(:compare_value).and_return(false)
+      @provider.should_receive(:requires_modify_or_create?).and_return(false)
       @provider.action_create
     end
 
     it "should call modify_env if the key exists and values are not equal" do
       @provider.key_exists = true
-      @provider.stub(:compare_value).and_return(true)
+      @provider.stub(:requires_modify_or_create?).and_return(true)
       @provider.should_receive(:modify_env).and_return(true)
       @provider.action_create
     end
 
     it "should set the new_resources updated flag when it updates an existing value" do
       @provider.key_exists = true
-      @provider.stub(:compare_value).and_return(true)
+      @provider.stub(:requires_modify_or_create?).and_return(true)
       @provider.stub(:modify_env).and_return(true)
       @provider.action_create
       @new_resource.should be_updated
@@ -147,20 +147,20 @@ describe Chef::Provider::Env do
     end
 
     it "should call modify_group if the key exists and values are not equal" do
-      @provider.should_receive(:compare_value).and_return(true)
+      @provider.should_receive(:requires_modify_or_create?).and_return(true)
       @provider.should_receive(:modify_env).and_return(true)
       @provider.action_modify
     end
 
     it "should set the new resources updated flag to true if modify_env is called" do
-      @provider.stub(:compare_value).and_return(true)
+      @provider.stub(:requires_modify_or_create?).and_return(true)
       @provider.stub(:modify_env).and_return(true)
       @provider.action_modify
       @new_resource.should be_updated
     end
 
     it "should not call modify_env if the key exists but the values are equal" do
-      @provider.should_receive(:compare_value).and_return(false)
+      @provider.should_receive(:requires_modify_or_create?).and_return(false)
       @provider.should_not_receive(:modify_env)
       @provider.action_modify
     end
@@ -198,9 +198,31 @@ describe Chef::Provider::Env do
       @provider.delete_element.should eql(true)
       @new_resource.should be_updated
     end
+
+    context "when new_resource's value contains the delimiter"  do
+      it "should return false if all the elements are deleted" do
+        # This indicates that the entire key needs to be deleted
+        @new_resource.value("C:/foo/bin;C:/bar/bin")
+        @provider.delete_element.should eql(false)
+        @new_resource.should_not be_updated  # This will be updated in action_delete
+      end
+
+      it "should return true if any, but not all, of the elements are deleted" do
+        @new_resource.value("C:/foo/bin;C:/notbaz/bin")
+        @provider.should_receive(:create_env)
+        @provider.delete_element.should eql(true)
+        @new_resource.should be_updated
+      end
+
+      it "should return true if none of the elements are deleted" do
+        @new_resource.value("C:/notfoo/bin;C:/notbaz/bin")
+        @provider.delete_element.should eql(true)
+        @new_resource.should_not be_updated
+      end
+    end
   end
 
-  describe "compare_value" do
+  describe "requires_modify_or_create?" do
     before(:each) do
       @new_resource.value("C:/bar")
       @current_resource = @new_resource.clone
@@ -208,25 +230,68 @@ describe Chef::Provider::Env do
     end
 
     it "should return false if the values are equal" do
-      @provider.compare_value.should be_false
+      @provider.requires_modify_or_create?.should be_false
     end
 
     it "should return true if the values not are equal" do
       @new_resource.value("C:/elsewhere")
-      @provider.compare_value.should be_true
+      @provider.requires_modify_or_create?.should be_true
     end
 
     it "should return false if the current value contains the element" do
       @new_resource.delim(";")
       @current_resource.value("C:/bar;C:/foo;C:/baz")
 
-      @provider.compare_value.should be_false
+      @provider.requires_modify_or_create?.should be_false
     end
 
     it "should return true if the current value does not contain the element" do
       @new_resource.delim(";")
       @current_resource.value("C:/biz;C:/foo/bin;C:/baz")
-      @provider.compare_value.should be_true
+      @provider.requires_modify_or_create?.should be_true
+    end
+
+    context "when new_resource's value contains the delimiter"  do
+      it "should return false if all the current values are contained" do
+        @new_resource.value("C:/biz;C:/baz")
+        @new_resource.delim(";")
+        @current_resource.value("C:/biz;C:/foo/bin;C:/baz")
+        @provider.requires_modify_or_create?.should be_false
+      end
+
+      it "should return true if any of the new values are not contained" do
+        @new_resource.value("C:/biz;C:/baz;C:/bin")
+        @new_resource.delim(";")
+        @current_resource.value("C:/biz;C:/foo/bin;C:/baz")
+        @provider.requires_modify_or_create?.should be_true
+      end
+    end
+  end
+
+  describe "modify_env" do
+    before(:each) do
+      @provider.stub(:create_env).and_return(true)
+      @new_resource.delim ";"
+
+      @current_resource = Chef::Resource::Env.new("FOO")
+      @current_resource.value "C:/foo/bin"
+      @provider.current_resource = @current_resource
+    end
+
+    it "should not modify the variable passed to the resource" do
+      new_value = "C:/bar/bin"
+      passed_value = new_value.dup
+      @new_resource.value(passed_value)
+      @provider.modify_env
+      passed_value.should == new_value
+    end
+
+    it "should only add values not already contained when a delimiter is provided" do
+      @new_resource.value("C:/foo;C:/bar;C:/baz")
+      @new_resource.delim(";")
+      @current_resource.value("C:/foo/bar;C:/bar;C:/baz")
+      @provider.modify_env
+      @new_resource.value.should eq("C:/foo;C:/foo/bar;C:/bar;C:/baz")
     end
   end
 end