Fix array parsing in windows_feature_dism / windows_feature_powershellfix_dism

This fixes how we parse out the arrays and adds testing to make sure it's doing what we want.

Plus it properly continues on when the user has removed all local feature but specified an external source of those via the registry. That was a reported issue that came in on the cookbook after the rewrite.

Lastly this also fixes a bad method call to the dism method that came over when diffing from the cookbook.

Signed-off-by: Tim Smith <tsmith@chef.io>

4 files changed, 30 insertions, 7 deletions

diff --git a/lib/chef/resource/windows_feature_dism.rb b/lib/chef/resource/windows_feature_dism.rb
index 1ac906790a..03cb5019ad 100644
--- a/lib/chef/resource/windows_feature_dism.rb
+++ b/lib/chef/resource/windows_feature_dism.rb
@@ -30,7 +30,7 @@ class Chef
 
       property :feature_name, [Array, String],
                description: "The name of the feature/role(s) to install if it differs from the resource name.",
-               coerce: proc { |x| Array(x) },
+               coerce: proc { |x| x.is_a?(String) ? x.split(/\s*,\s*/) : x },
                name_property: true
 
       property :source, String,
@@ -55,7 +55,7 @@ class Chef
         unless features_to_install.empty?
           message = "install Windows feature#{'s' if features_to_install.count > 1} #{features_to_install.join(',')}"
           converge_by(message) do
-            install_command = "#{dism} /online /enable-feature #{features_to_install.map { |f| "/featurename:#{f}" }.join(' ')} /norestart"
+            install_command = "dism.exe /online /enable-feature #{features_to_install.map { |f| "/featurename:#{f}" }.join(' ')} /norestart"
             install_command << " /LimitAccess /Source:\"#{new_resource.source}\"" if new_resource.source
             install_command << " /All" if new_resource.all
 
@@ -195,6 +195,9 @@ class Chef
         # @return [void]
         def fail_if_removed
           return if new_resource.source # if someone provides a source then all is well
+          if node["os_version"].to_f > 6.2
+            return if registry_key_exists?('HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Policies\Servicing') && registry_value_exists?('HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Policies\Servicing', name: "LocalSourcePath") # if source is defined in the registry, still fine
+          end
           removed = new_resource.feature_name & node["dism_features_cache"]["removed"]
           raise "The Windows feature#{'s' if removed.count > 1} #{removed.join(',')} #{removed.count > 1 ? 'are' : 'is'} have been removed from the host and cannot be installed." unless removed.empty?
         end
diff --git a/lib/chef/resource/windows_feature_powershell.rb b/lib/chef/resource/windows_feature_powershell.rb
index 7e7fb6c160..b25574f0f5 100644
--- a/lib/chef/resource/windows_feature_powershell.rb
+++ b/lib/chef/resource/windows_feature_powershell.rb
@@ -33,7 +33,7 @@ class Chef
 
       property :feature_name, [Array, String],
                description: "The name of the feature/role(s) to install if it differs from the resource name.",
-               coerce: proc { |x| Array(x) },
+               coerce: proc { |x| x.is_a?(String) ? x.split(/\s*,\s*/) : x },
                name_property: true
 
       property :source, String,
diff --git a/spec/unit/resource/windows_feature_dism.rb b/spec/unit/resource/windows_feature_dism.rb
index 3885f4813e..1004b75ba9 100644
--- a/spec/unit/resource/windows_feature_dism.rb
+++ b/spec/unit/resource/windows_feature_dism.rb
@@ -18,7 +18,7 @@
 require "spec_helper"
 
 describe Chef::Resource::WindowsFeatureDism do
-  let(:resource) { Chef::Resource::WindowsFeatureDism.new("SNMP") }
+  let(:resource) { Chef::Resource::WindowsFeatureDism.new(%w{SNMP DHCP}) }
 
   it "sets resource name as :windows_feature_dism" do
     expect(resource.resource_name).to eql(:windows_feature_dism)
@@ -28,7 +28,17 @@ describe Chef::Resource::WindowsFeatureDism do
     expect(resource.action).to eql([:install])
   end
 
-  it "sets the feature_name property as its name and coerces it to an array" do
+  it "sets the feature_name property as its name property" do
+    expect(resource.feature_name).to eql(%w{SNMP DHCP})
+  end
+
+  it "coerces comma separated lists of features to arrays" do
+    resource.feature_name "SNMP, DHCP"
+    expect(resource.feature_name).to eql(%w{SNMP DHCP})
+  end
+
+  it "coerces a single feature as a String into an array" do
+    resource.feature_name "SNMP"
     expect(resource.feature_name).to eql(["SNMP"])
   end
 
diff --git a/spec/unit/resource/windows_feature_powershell.rb b/spec/unit/resource/windows_feature_powershell.rb
index 02f308ca73..5ea226575c 100644
--- a/spec/unit/resource/windows_feature_powershell.rb
+++ b/spec/unit/resource/windows_feature_powershell.rb
@@ -18,7 +18,7 @@
 require "spec_helper"
 
 describe Chef::Resource::WindowsFeaturePowershell do
-  let(:resource) { Chef::Resource::WindowsFeaturePowershell.new("SNMP") }
+  let(:resource) { Chef::Resource::WindowsFeaturePowershell.new(%w{SNMP DHCP}) }
 
   it "sets resource name as :windows_feature_powershell" do
     expect(resource.resource_name).to eql(:windows_feature_powershell)
@@ -28,7 +28,17 @@ describe Chef::Resource::WindowsFeaturePowershell do
     expect(resource.action).to eql([:install])
   end
 
-  it "sets the feature_name property as its name and coerces it to an array" do
+  it "sets the feature_name property as its name property" do
+    expect(resource.feature_name).to eql(%w{SNMP DHCP})
+  end
+
+  it "coerces comma separated lists of features to arrays" do
+    resource.feature_name "SNMP, DHCP"
+    expect(resource.feature_name).to eql(%w{SNMP DHCP})
+  end
+
+  it "coerces a single feature as a String into an array" do
+    resource.feature_name "SNMP"
     expect(resource.feature_name).to eql(["SNMP"])
   end