Fixed review comments

Signed-off-by: Vasu1105 <vasundhara.jagdale@clogeny.com>

7 files changed, 51 insertions, 72 deletions

diff --git a/lib/chef/provider/package/cab.rb b/lib/chef/provider/package/cab.rb
index f5af1a86b4..4e2054fbc2 100644
--- a/lib/chef/provider/package/cab.rb
+++ b/lib/chef/provider/package/cab.rb
@@ -18,13 +18,13 @@
 
 require "chef/provider/package"
 require "chef/resource/cab_package"
-require "chef/mixin/powershell_out"
+require "chef/mixin/shell_out"
 
 class Chef
   class Provider
     class Package
       class Cab < Chef::Provider::Package
-        include Chef::Mixin::PowershellOut
+        include Chef::Mixin::ShellOut
 
         provides :cab_package, os: "windows"
 
@@ -45,7 +45,10 @@ class Chef
         end
 
         def dism_command(command)
-          powershell_out("dism.exe /Online #{command} /NoRestart", { :timeout => @new_resource.timeout })
+          shellout = Mixlib::ShellOut.new("dism.exe /Online #{command} /NoRestart", { :timeout => @new_resource.timeout })
+          with_os_architecture(nil) do
+            shellout.run_command
+          end
         end
 
         def installed_version
@@ -63,7 +66,7 @@ class Chef
             find_version(stdout)
           else
             # Presuming this won't happen, otherwise we need to handle it
-            raise Chef::Exceptions::Package, "Found mutliple packages installed matching name #{package['name']}, found: #{found_packages.length} matches"
+            raise Chef::Exceptions::Package, "Found multiple packages installed matching name #{package['name']}, found: #{found_packages.length} matches"
           end
         end
 
@@ -102,7 +105,7 @@ class Chef
           section_headers = [ "Package information", "Custom Properties", "Features" ]
           text.each_line do |line|
             if line =~ /Error: (.*)/
-              errors << $1
+              errors << $1.strip
             elsif section_headers.any? { |header| line =~ /^(#{header})/ }
               in_section = $1.downcase.tr(" ", "_")
             elsif line =~ /(.*) ?: (.*)/
@@ -117,9 +120,9 @@ class Chef
             end
           end
           unless errors.empty?
-            if !(errors & ["0x80070003\r", "0x80070002\r"]).empty?
+            if errors.include?("0x80070003") || errors.include?("0x80070002")
               raise Chef::Exceptions::Package, "DISM: The system cannot find the path or file specified."
-            elsif errors.include?("740\r")
+            elsif errors.include?("740")
               raise Chef::Exceptions::Package, "DISM: Error 740: Elevated permissions are required to run DISM."
             else
               raise Chef::Exceptions::Package, "Unknown errors encountered parsing DISM output: #{errors}"
diff --git a/spec/data/templates/chef-seattle20160930-4388-1crv7ef.txt b/spec/data/templates/chef-seattle20160930-4388-1crv7ef.txt
new file mode 100644
index 0000000000..f476ccd704
--- /dev/null
+++ b/spec/data/templates/chef-seattle20160930-4388-1crv7ef.txt
@@ -0,0 +1 @@
+Do do do do, do do do do, do do do do, do do do do
\ No newline at end of file
diff --git a/spec/data/templates/chef-seattle20160930-4388-jjfoae.txt b/spec/data/templates/chef-seattle20160930-4388-jjfoae.txt
new file mode 100644
index 0000000000..f476ccd704
--- /dev/null
+++ b/spec/data/templates/chef-seattle20160930-4388-jjfoae.txt
@@ -0,0 +1 @@
+Do do do do, do do do do, do do do do, do do do do
\ No newline at end of file
diff --git a/spec/data/templates/chef-seattle20160930-4388-umeq2c.txt b/spec/data/templates/chef-seattle20160930-4388-umeq2c.txt
new file mode 100644
index 0000000000..f476ccd704
--- /dev/null
+++ b/spec/data/templates/chef-seattle20160930-4388-umeq2c.txt
@@ -0,0 +1 @@
+Do do do do, do do do do, do do do do, do do do do
\ No newline at end of file
diff --git a/spec/functional/assets/dummy-cab-23448.cab b/spec/functional/assets/dummy-cab-23448.cab
deleted file mode 100644
index 41eea0b77c..0000000000
--- a/spec/functional/assets/dummy-cab-23448.cab
+++ /dev/null
diff --git a/spec/functional/resource/cab_package_spec.rb b/spec/functional/resource/cab_package_spec.rb
deleted file mode 100644
index c0750ed480..0000000000
--- a/spec/functional/resource/cab_package_spec.rb
+++ /dev/null
@@ -1,54 +0,0 @@
-#
-# Author:: Vasundhara Jagdale (<vasundhara.jagdale@msystechnologies.com>)
-# Copyright:: Copyright 2015-2016, Chef Software, Inc.
-# License:: Apache License, Version 2.0
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-#
-
-require "spec_helper"
-require "chef/mixin/powershell_out"
-
-describe Chef::Resource::CabPackage, :windows_only do
-  include Chef::Mixin::PowershellOut
-
-  let(:package_source) { File.join(CHEF_SPEC_ASSETS, "dummy-cab-23448.cab") }
-
-  let(:new_resource) do
-    new_resource = Chef::Resource::CabPackage.new("test-package", run_context)
-    new_resource.source = package_source
-    new_resource
-  end
-
-  let(:installed_version) { proc { stdout = powershell_out!("dism.exe /Online /Get-PackageInfo /PackagePath:\"#{new_resource.source}\" /NoRestart").stdout } }
-
-  context "installing package" do
-    after { remove_package }
-
-    it "installs the package" do
-      new_resource.run_action(:install)
-      expect(new_resource).to be_updated_by_last_action
-    end
-
-    it "raises error if package is not found" do
-      new_resource.source = File.join(CHEF_SPEC_ASSETS, "test.cab")
-      expect { new_resource.run_action(:install) }.to raise_error Chef::Exceptions::Package
-    end
-  end
-end
-
-def remove_package
-  pkg_to_remove = Chef::Resource::CabPackage.new("test-package", run_context)
-  pkg_to_remove.source = package_source
-  pkg_to_remove.run_action(:remove)
-end
diff --git a/spec/unit/provider/package/cab_spec.rb b/spec/unit/provider/package/cab_spec.rb
index 9ce0c77485..5e16795aa8 100644
--- a/spec/unit/provider/package/cab_spec.rb
+++ b/spec/unit/provider/package/cab_spec.rb
@@ -51,13 +51,13 @@ The operation completed successfully
   before do
     new_resource.source = "C:\\Temp\\Test6.1-KB2664825-v3-x64.cab"
     installed_package_list_obj = double(stdout: installed_package_list_stdout)
-    allow(provider).to receive(:powershell_out).with("dism.exe /Online /Get-Packages /NoRestart", { timeout: timeout }).and_return(installed_package_list_obj)
+    allow(provider).to receive(:dism_command).with("/Get-Packages").and_return(installed_package_list_obj)
     package_version_obj = double(stdout: package_version_stdout)
-    allow(provider).to receive(:powershell_out).with("dism.exe /Online /Get-PackageInfo /PackagePath:\"#{new_resource.source}\" /NoRestart", { timeout: timeout }).and_return(package_version_obj)
+    allow(provider).to receive(:dism_command).with("/Get-PackageInfo /PackagePath:\"#{new_resource.source}\"").and_return(package_version_obj)
   end
 
   def allow_package_info(package_path = nil, package_name = nil)
-    get_pakage_info_stdout = <<-EOF
+    get_package_info_stdout = <<-EOF
 Deployment Image Servicing and Management tool
 Version: 6.1.7600.16385
 
@@ -72,15 +72,15 @@ State : Installed
 Dependency : Language Pack
 The operation completed successfully
     EOF
-    get_pakage_info_obj = double(stdout: get_pakage_info_stdout)
+    get_package_info_obj = double(stdout: get_package_info_stdout)
     if package_path
-      allow(provider).to receive(:powershell_out).with("dism.exe /Online /Get-PackageInfo /PackagePath:\"#{package_path}\" /NoRestart", { timeout: timeout }).and_return(get_pakage_info_obj)
+      allow(provider).to receive(:dism_command).with("/Get-PackageInfo /PackagePath:\"#{package_path}\"").and_return(get_package_info_obj)
     else
-      allow(provider).to receive(:powershell_out).with("dism.exe /Online /Get-PackageInfo /PackageName:\"#{package_name}\" /NoRestart", { timeout: timeout }).and_return(get_pakage_info_obj)
+      allow(provider).to receive(:dism_command).with("/Get-PackageInfo /PackageName:\"#{package_name}\"").and_return(get_package_info_obj)
     end
   end
 
-  def allow_get_packags
+  def allow_get_packages
     get_packages_stdout = <<-EOF
 Deployment Image Servicing and Management tool
 Version: 6.1.7600.16385
@@ -107,7 +107,7 @@ Install Time : 11/21/2010 3:40 AM
 The operation completed successfully.
     EOF
     get_packages_obj = double(stdout: get_packages_stdout)
-    allow(provider).to receive(:powershell_out).with("dism.exe /Online /Get-Packages /NoRestart", { timeout: timeout }).and_return(get_packages_obj)
+    allow(provider).to receive(:dism_command).with("/Get-Packages").and_return(get_packages_obj)
   end
 
   describe "#load_current_resource" do
@@ -142,7 +142,7 @@ The operation completed successfully.
   describe "#installed_version" do
     it "returns the current installed version of package" do
       allow_package_info(new_resource.source, nil)
-      allow_get_packags
+      allow_get_packages
       allow_package_info(nil, "Package_for_KB2664825~31bf3856ad364e35~amd64~~6.1.3.0")
       expect(provider.installed_version).to eql("6.1.3.0")
     end
@@ -158,7 +158,7 @@ The operation completed successfully.
 
     it "removes packages if package is installed" do
       allow_package_info(new_resource.source, nil)
-      allow_get_packags
+      allow_get_packages
       allow_package_info(nil, "Package_for_KB2664825~31bf3856ad364e35~amd64~~6.1.3.0")
       provider.load_current_resource
       expect(provider.installed_version).not_to eql(nil)
@@ -179,7 +179,7 @@ The operation completed successfully.
 
     it "does not install package if already installed" do
       allow_package_info(new_resource.source, nil)
-      allow_get_packags
+      allow_get_packages
       allow_package_info(nil, "Package_for_KB2664825~31bf3856ad364e35~amd64~~6.1.3.0")
       provider.load_current_resource
       expect(provider.installed_version).not_to eql(nil)
@@ -188,4 +188,31 @@ The operation completed successfully.
       expect(new_resource).not_to be_updated_by_last_action
     end
   end
+
+  context "Invalid package source" do
+    def package_version_stdout
+      package_version_stdout = <<-EOF
+
+Deployment Image Servicing and Management tool
+Version: 6.1.7600.16385
+
+Image Version: 6.1.7600.16385
+
+An error occurred trying to open - c:\\temp\\test6.1-KB2664825-v3-x64.cab Error: 0x80070003
+Error: 3
+The system cannot find the path specified.
+The DISM log file can be found at C:\\Windows\\Logs\\DISM\\dism.log.
+      EOF
+    end
+
+    before do
+      new_resource.source = "C:\\Temp\\Test6.1-KB2664825-v3-x64.cab"
+      installed_package_list_obj = double(stdout: installed_package_list_stdout)
+      allow(provider).to receive(:dism_command).with("/Get-Packages").and_return(installed_package_list_obj)
+    end
+
+    it "raises error for invalid source path or file" do
+      expect { provider.load_current_resource }.to raise_error(Chef::Exceptions::Package, "DISM: The system cannot find the path or file specified.")
+    end
+  end
 end