Merge pull request #2024 from opscode/jdmundrawala/windows_env_path

Windows path fix

2 files changed, 116 insertions, 34 deletions

diff --git a/spec/functional/resource/env_spec.rb b/spec/functional/resource/env_spec.rb
index 9002d4b727..24fe5e1dff 100755
--- a/spec/functional/resource/env_spec.rb
+++ b/spec/functional/resource/env_spec.rb
@@ -24,6 +24,8 @@ describe Chef::Resource::Env, :windows_only do
     let(:chef_env_test_mixed_case) { 'chefENVtest' }
     let(:env_value1) { 'value1' }
     let(:env_value2) { 'value2' }
+
+    let(:env_value_expandable) { '%SystemRoot%' }
     let(:test_run_context) {
       node = Chef::Node.new
       node.default['platform'] = 'windows'
@@ -71,6 +73,14 @@ describe Chef::Resource::Env, :windows_only do
         test_resource.run_action(:create)
         expect(ENV[chef_env_test_lower_case]).to eq(env_value2)
       end
+
+      it 'should not expand environment variables if the variable is not PATH' do
+        expect(ENV[chef_env_test_lower_case]).to eq(nil)
+        test_resource.key_name(chef_env_test_lower_case)
+        test_resource.value(env_value_expandable)
+        test_resource.run_action(:create)
+        expect(ENV[chef_env_test_lower_case]).to eq(env_value_expandable)
+      end
     end
 
     context "when the modify action is invoked" do
@@ -102,6 +112,42 @@ describe Chef::Resource::Env, :windows_only do
         test_resource.run_action(:modify)
         expect(ENV[chef_env_test_lower_case]).to eq(env_value2)
       end
+
+      it 'should not expand environment variables if the variable is not PATH' do
+        test_resource.key_name(chef_env_test_lower_case)
+        test_resource.value(env_value1)
+        test_resource.run_action(:create)
+        expect(ENV[chef_env_test_lower_case]).to eq(env_value1)
+        test_resource.value(env_value_expandable)
+        test_resource.run_action(:modify)
+        expect(ENV[chef_env_test_lower_case]).to eq(env_value_expandable)
+      end
+
+      context 'when using PATH' do
+        let(:random_name) { Time.now.to_i }
+        let(:env_val) { "#{env_value_expandable}_#{random_name}"}
+        let(:path_before) { test_resource.provider_for_action(test_resource.action).env_value('PATH') }
+
+        it 'should expand PATH' do
+          path_before.should_not include(env_val)
+          test_resource.key_name('PATH')
+          test_resource.value("#{path_before};#{env_val}")
+          test_resource.run_action(:create)
+          ENV['PATH'].should_not include(env_val)
+          ENV['PATH'].should include("#{random_name}")
+        end
+
+        after(:each) do
+          # cleanup so we don't flood the path
+          test_resource.key_name('PATH')
+          test_resource.value(path_before)
+          test_resource.run_action(:create)
+          if test_resource.provider_for_action(test_resource.action).env_value('PATH') != path_before
+            raise 'Failed to cleanup after ourselves'
+          end
+        end
+      end
+
     end
 
     context "when the delete action is invoked" do
diff --git a/spec/unit/provider/env/windows_spec.rb b/spec/unit/provider/env/windows_spec.rb
index 329448ac05..2ea137c1d9 100644
--- a/spec/unit/provider/env/windows_spec.rb
+++ b/spec/unit/provider/env/windows_spec.rb
@@ -19,49 +19,85 @@
 require 'spec_helper'
 
 describe Chef::Provider::Env::Windows, :windows_only do
-  before do
-    @node = Chef::Node.new
-    @events = Chef::EventDispatch::Dispatcher.new
-    @run_context = Chef::RunContext.new(@node, {}, @events)
-    @new_resource = Chef::Resource::Env.new("CHEF_WINDOWS_ENV_TEST")
-    @new_resource.value("foo")
-    @provider = Chef::Provider::Env::Windows.new(@new_resource, @run_context)
-    @provider.stub(:env_obj).and_return(double('null object').as_null_object)
-  end
+  let(:node) { Chef::Node.new }
+  let(:events) {Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
 
-  describe "action_create" do
-    before do
-      ENV.delete('CHEF_WINDOWS_ENV_TEST')
-      @provider.key_exists = false
-    end
+  context 'when environment variable is not PATH' do
+    let(:new_resource) {
+      new_resource = Chef::Resource::Env.new("CHEF_WINDOWS_ENV_TEST")
+      new_resource.value("foo")
+      new_resource
+    }
+    let(:provider) { 
+      provider = Chef::Provider::Env::Windows.new(new_resource, run_context) 
+      provider.stub(:env_obj).and_return(double('null object').as_null_object)
+      provider
+    }
 
-    it "should update the ruby ENV object when it creates the key" do
-      @provider.action_create
-      expect(ENV['CHEF_WINDOWS_ENV_TEST']).to eql('foo')
-    end
-  end
+    describe "action_create" do
+      before do
+        ENV.delete('CHEF_WINDOWS_ENV_TEST')
+        provider.key_exists = false
+      end
 
-  describe "action_modify" do
-    before do
-      ENV['CHEF_WINDOWS_ENV_TEST'] = 'foo'
+      it "should update the ruby ENV object when it creates the key" do
+        provider.action_create
+        expect(ENV['CHEF_WINDOWS_ENV_TEST']).to eql('foo')
+      end
     end
 
-    it "should update the ruby ENV object when it updates the value" do
-      @provider.should_receive(:compare_value).and_return(true)
-      @new_resource.value("foobar")
-      @provider.action_modify
-      expect(ENV['CHEF_WINDOWS_ENV_TEST']).to eql('foobar')
+    describe "action_modify" do
+      before do
+        ENV['CHEF_WINDOWS_ENV_TEST'] = 'foo'
+      end
+
+      it "should update the ruby ENV object when it updates the value" do
+        provider.should_receive(:compare_value).and_return(true)
+        new_resource.value("foobar")
+        provider.action_modify
+        expect(ENV['CHEF_WINDOWS_ENV_TEST']).to eql('foobar')
+      end
+
+      describe "action_delete" do
+        before do
+          ENV['CHEF_WINDOWS_ENV_TEST'] = 'foo'
+        end
+
+        it "should update the ruby ENV object when it deletes the key" do
+          provider.action_delete
+          expect(ENV['CHEF_WINDOWS_ENV_TEST']).to eql(nil)
+        end
+      end
     end
   end
 
-  describe "action_delete" do
-    before do
-      ENV['CHEF_WINDOWS_ENV_TEST'] = 'foo'
-    end
+  context 'when environment is PATH' do
+    describe "for PATH" do
+      let(:system_root) {'%SystemRoot%'}
+      let(:system_root_value) { 'D:\Windows' }
+      let(:new_resource) {
+        new_resource = Chef::Resource::Env.new('PATH')
+        new_resource.value(system_root)
+        new_resource
+      }
+      let(:provider) { 
+        provider = Chef::Provider::Env::Windows.new(new_resource, run_context) 
+        provider.stub(:env_obj).and_return(double('null object').as_null_object)
+        provider
+      }
 
-    it "should update the ruby ENV object when it deletes the key" do
-      @provider.action_delete
-      expect(ENV['CHEF_WINDOWS_ENV_TEST']).to eql(nil)
+      before do
+        stub_const('ENV', {'PATH' => ''})
+      end
+
+      it "replaces Windows system variables" do
+        provider.should_receive(:compare_value).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)
+      end
     end
+
   end
 end