windows_workgroup: Coerce the provided reboot property and add more tests

Instead of converting the passed in reboot property when we use it coerce it when it's passed in. This allows us to easily test the conversion and it makes it so the docs don't include the legacy values.

I also added moved command generation into a method and added tests for that.

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

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