Merge pull request #7916 from chef/testing

windows_workgroup: Coerce the provided reboot property and add more tests

2 files changed, 59 insertions, 23 deletions

diff --git a/lib/chef/resource/windows_workgroup.rb b/lib/chef/resource/windows_workgroup.rb
index f8391a88cd..2aeb534074 100644
--- a/lib/chef/resource/windows_workgroup.rb
+++ b/lib/chef/resource/windows_workgroup.rb
@@ -36,19 +36,36 @@ class Chef
                name_property: true
 
       property :user, String,
-               description: "The local administrator user to use to change the workgroup.",
+               description: "The local administrator user to use to change the workgroup. Required if using the password property.",
                desired_state: false
 
       property :password, String,
-               description: "The password for the local administrator user.",
+               description: "The password for the local administrator user. Required if using the user property.",
                desired_state: false
 
       property :reboot, Symbol,
-               equal_to: [:immediate, :delayed, :never, :request_reboot, :reboot_now],
+               equal_to: [:never, :request_reboot, :reboot_now],
                validation_message: "The reboot property accepts :immediate (reboot as soon as the resource completes), :delayed (reboot once the Chef run completes), and :never (Don't reboot)",
                description: "Controls the system reboot behavior post workgroup joining. Reboot immediately, after the Chef run completes, or never. Note that a reboot is necessary for changes to take effect.",
+               coerce: proc { |x| clarify_reboot(x) },
                default: :immediate, desired_state: false
 
+      # This resource historically took `:immediate` and `:delayed` as arguments to the reboot property but then
+      # tried to shove that straight to the `reboot` resource which objected strenuously. We need to convert these
+      # legacy actions into actual reboot actions
+      #
+      # @return [Symbol] chef reboot resource action
+      def clarify_reboot(reboot_action)
+        case reboot_action
+        when :immediate
+          :reboot_now
+        when :delayed
+          :request_reboot
+        else
+          reboot_action
+        end
+      end
+
       # define this again so we can default it to true. Otherwise failures print the password
       property :sensitive, [TrueClass, FalseClass],
                default: true, desired_state: false
@@ -57,20 +74,13 @@ class Chef
         description "Update the workgroup."
 
         unless workgroup_member?
-          cmd = ""
-          cmd << "$pswd = ConvertTo-SecureString \'#{new_resource.password}\' -AsPlainText -Force;" if new_resource.password
-          cmd << "$credential = New-Object System.Management.Automation.PSCredential (\"#{new_resource.user}\",$pswd);" if new_resource.password
-          cmd << "Add-Computer -WorkgroupName #{new_resource.workgroup_name}"
-          cmd << " -Credential $credential" if new_resource.password
-          cmd << " -Force"
-
           converge_by("join workstation workgroup #{new_resource.workgroup_name}") do
-            ps_run = powershell_out(cmd)
+            ps_run = powershell_out(join_command)
             raise "Failed to join the workgroup #{new_resource.workgroup_name}: #{ps_run.stderr}}" if ps_run.error?
 
             unless new_resource.reboot == :never
               reboot "Reboot to join workgroup #{new_resource.workgroup_name}" do
-                action clarify_reboot(new_resource.reboot)
+                action new_resource.reboot
                 reason "Reboot to join workgroup #{new_resource.workgroup_name}"
               end
             end
@@ -79,19 +89,18 @@ class Chef
       end
 
       action_class do
-        # This resource historically took `:immediate` and `:delayed` as arguments to the reboot property but then
-        # tried to shove that straight to the `reboot` resource which objected strenuously
-        def clarify_reboot(reboot_action)
-          case reboot_action
-          when :immediate
-            :reboot_now
-          when :delayed
-            :request_reboot
-          else
-            reboot_action
-          end
+        # return [String] the appropriate PS command to joint the workgroup
+        def join_command
+          cmd = ""
+          cmd << "$pswd = ConvertTo-SecureString \'#{new_resource.password}\' -AsPlainText -Force;" if new_resource.password
+          cmd << "$credential = New-Object System.Management.Automation.PSCredential (\"#{new_resource.user}\",$pswd);" if new_resource.password
+          cmd << "Add-Computer -WorkgroupName #{new_resource.workgroup_name}"
+          cmd << " -Credential $credential" if new_resource.password
+          cmd << " -Force"
+          cmd
         end
 
+        # @return [Boolean] is the node a member of the workgroup specified in the resource
         def workgroup_member?
           node_workgroup = powershell_out!("(Get-WmiObject -Class Win32_ComputerSystem).Workgroup")
           raise "Failed to determine if system already a member of workgroup #{new_resource.workgroup_name}" if node_workgroup.error?
diff --git a/spec/unit/resource/windows_workgroup_spec.rb b/spec/unit/resource/windows_workgroup_spec.rb
index 3d7acec2fb..0ea0d8b291 100644
--- a/spec/unit/resource/windows_workgroup_spec.rb
+++ b/spec/unit/resource/windows_workgroup_spec.rb
@@ -19,6 +19,7 @@ require "spec_helper"
 
 describe Chef::Resource::WindowsWorkgroup do
   let(:resource) { Chef::Resource::WindowsWorkgroup.new("example") }
+  let(:provider) { resource.provider_for_action(:join) }
 
   it "sets resource name as :windows_workgroup" do
     expect(resource.resource_name).to eql(:windows_workgroup)
@@ -28,6 +29,16 @@ describe Chef::Resource::WindowsWorkgroup do
     expect(resource.workgroup_name).to eql("example")
   end
 
+  it "converts the legacy :immediate reboot property to :reboot_now" do
+    resource.reboot(:immediate)
+    expect(resource.reboot).to eql(:reboot_now)
+  end
+
+  it "converts the legacy :delayed reboot property to :request_reboot" do
+    resource.reboot(:delayed)
+    expect(resource.reboot).to eql(:request_reboot)
+  end
+
   it "sets the default action as :join" do
     expect(resource.action).to eql([:join])
   end
@@ -44,4 +55,20 @@ describe Chef::Resource::WindowsWorkgroup do
     expect { resource.reboot :never }.not_to raise_error
     expect { resource.reboot :nopenope }.to raise_error(ArgumentError)
   end
+
+  describe "#join_command" do
+    context "if password property is not specified" do
+      it "contructs a command without credentials" do
+        expect(provider.join_command).to eql("Add-Computer -WorkgroupName example -Force")
+      end
+    end
+
+    context "if password property is specified" do
+      it "contructs a command without credentials" do
+        resource.password("1234")
+        resource.user("admin")
+        expect(provider.join_command).to eql("$pswd = ConvertTo-SecureString '1234' -AsPlainText -Force;$credential = New-Object System.Management.Automation.PSCredential (\"admin\",$pswd);Add-Computer -WorkgroupName example -Credential $credential -Force")
+      end
+    end
+  end
 end