Fix issue with install_switches, changed to use config file instead due to how ruby writes path to Windows exec

Travis Fields · Travis Fields · commit b9ae791a8591 · 2015-03-05T13:08:04.000-08:00
diff --git a/lib/puppet/provider/sqlserver_features/mssql.rb b/lib/puppet/provider/sqlserver_features/mssql.rb
@@ -58,18 +58,43 @@ def modify_features(action, features)
         if not_nil_and_not_empty?(@resource[:pid])
           cmd_args << "/PID=#{@resource[:pid]}"
         end
-        if not_nil_and_not_empty?(@resource[:install_switches])
-          @resource[:install_switches].each_pair do |k, v|
-            if v.is_a?(Numeric) || (v.is_a?(String) && v =~ /\d/)
-              cmd_args << "/#{k}=#{v}"
-            else
-              cmd_args << "/#{k}='#{v}'"
-            end
-          end
+      end
+      begin
+        config_file = create_temp_for_install_switch unless action == 'uninstall'
+        cmd_args << "/ConfigurationFile=\"#{config_file.path}\"" unless config_file.nil?
+        try_execute(cmd_args, "Unable to #{action} features (#{features.join(', ')})")
+      ensure
+        if config_file
+          config_file.close
+          config_file.unlink
+        end
+      end
+    end
+  end
+
+  def create_temp_for_install_switch
+    if not_nil_and_not_empty? @resource[:install_switches]
+      config_file = ["[OPTIONS]"]
+      @resource[:install_switches].each_pair do |k, v|
+        if RESERVED_SWITCHES.include? k
+          warn("Reserved switch [#{k}] found for `install_switches`, please know the provided value
+may be overridden by some command line arguments")
+        end
+        if v.is_a?(Numeric) || (v.is_a?(String) && v =~ /\d/)
+          config_file << "#{k}=#{v}"
+        elsif v.nil?
+          config_file << k
+        else
+          config_file << "#{k}=\"#{v}\""
         end
       end
-      try_execute(cmd_args, "Unable to #{action} features (#{features.join(', ')})")
+      config_temp = Tempfile.new(['sqlconfig', '.ini'])
+      config_temp.write(config_file.join("\n"))
+      config_temp.flush
+      config_temp.close
+      return config_temp
     end
+    return nil
   end
 
   def installNet35
diff --git a/lib/puppet/provider/sqlserver_instance/mssql.rb b/lib/puppet/provider/sqlserver_instance/mssql.rb
@@ -2,6 +2,9 @@
 require File.expand_path(File.join(File.dirname(__FILE__), '..', 'sqlserver'))
 
 Puppet::Type::type(:sqlserver_instance).provide(:mssql, :parent => Puppet::Provider::Sqlserver) do
+  RESERVED_SWITCHES =
+    %w(AGTSVCACCOUNT AGTSVCPASSWORD ASSVCACCOUNT AGTSVCPASSWORD PID
+       RSSVCACCOUNT RSSVCPASSWORD SAPWD SECURITYMODE SQLSYSADMINACCOUNTS FEATURES)
 
   def self.instances
     instances = []
@@ -42,7 +45,17 @@ def add_features(features)
   def modify_features(features, action)
     if not_nil_and_not_empty? features
       debug "#{action.capitalize}ing features '#{features.join(',')}'"
-      try_execute(build_cmd_args(features, action), "Error trying to #{action} features (#{features.join(', ')}")
+      cmd_args = build_cmd_args(features, action)
+      begin
+        config_file = create_temp_for_install_switch unless action == 'uninstall'
+        cmd_args << "/ConfigurationFile=\"#{config_file.path}\"" unless config_file.nil?
+        try_execute(cmd_args, "Error trying to #{action} features (#{features.join(', ')}")
+      ensure
+        if config_file
+          config_file.close
+          config_file.unlink
+        end
+      end
     end
   end
 
@@ -56,11 +69,46 @@ def create
       destroy
     else
       installNet35
-      cmd_args = build_cmd_args(@resource[:features])
-      try_execute(cmd_args)
+      add_features(@resource[:features])
+      # cmd_args = build_cmd_args(@resource[:features])
+      # begin
+      #   config_file = create_temp_for_install_switch
+      #   cmd_args << "/ConfigurationFile=\"#{config_file.path}\"" unless config_file.nil?
+      #   try_execute(cmd_args)
+      # ensure
+      #   if config_file
+      #     config_file.close
+      #     config_file.unlink
+      #   end
+      # end
     end
   end
 
+  def create_temp_for_install_switch
+    if not_nil_and_not_empty? @resource[:install_switches]
+      config_file = ["[OPTIONS]"]
+      @resource[:install_switches].each_pair do |k, v|
+        if RESERVED_SWITCHES.include? k
+          warn("Reserved switch [#{k}] found for `install_switches`, please know the provided value
+may be overridden by some command line arguments")
+        end
+        if v.is_a?(Numeric) || (v.is_a?(String) && v =~ /\d/)
+          config_file << "#{k}=#{v}"
+        elsif v.nil?
+          config_file << k
+        else
+          config_file << "#{k}=\"#{v}\""
+        end
+      end
+      config_temp = Tempfile.new(['sqlconfig', '.ini'])
+      config_temp.write(config_file.join("\n"))
+      config_temp.flush
+      config_temp.close
+      return config_temp
+    end
+    return nil
+  end
+
   def basic_cmd_args(features, action)
     cmd_args = ["#{@resource[:source]}/setup.exe",
                 "/ACTION=#{action}",
@@ -83,15 +131,6 @@ def build_cmd_args(features, action="install")
           cmd_args << "/SQLSYSADMINACCOUNTS=\"#{@resource[:sql_sysadmin_accounts]}\""
         end
       end
-      if not_nil_and_not_empty? @resource[:install_switches]
-        @resource[:install_switches].each_pair do |k, v|
-          if v.is_a?(Numeric) || (v.is_a?(String) && v =~ /\d/)
-            cmd_args << "/#{k}=#{v}"
-          else
-            cmd_args << "/#{k}='#{v}'"
-          end
-        end
-      end
     end
     cmd_args
   end
diff --git a/spec/unit/puppet/provider/sqlserver__instance_spec.rb b/spec/unit/puppet/provider/sqlserver__instance_spec.rb
@@ -1,4 +1,5 @@
 require 'spec_helper'
+require 'mocha'
 
 require File.expand_path(File.join(File.dirname(__FILE__), '..', 'sqlserver_install_context.rb'))
 
@@ -144,10 +145,18 @@ def stub_uninstall(args, installed_features)
   end
 
   describe 'install_switches' do
+    before :each do
+      @file_double = Tempfile.new(['sqlconfig', '.ini'])
+      @file_double.stubs(:write)
+      @file_double.stubs(:flush)
+      @file_double.stubs(:close)
+      Tempfile.stubs(:new).with(['sqlconfig', '.ini']).returns(@file_double)
+    end
+
     it_behaves_like 'create' do
       args = get_basic_args
       args[:install_switches] = {'ERRORREPORTING' => 1}
-      let(:additional_install_switches) { ['/ERRORREPORTING=1'] }
+      let(:additional_install_switches) { ["/ConfigurationFile=\"#{@file_double.path}\""] }
       let(:args) { args }
       munged = {:features => Array.new(args[:features])}
       munged[:features].delete('SQL')
@@ -158,7 +167,7 @@ def stub_uninstall(args, installed_features)
     it_behaves_like 'create' do
       args = get_basic_args
       args[:install_switches] = {'ERRORREPORTING' => 1, 'SQLBACKUPDIR' => 'I:\DBbackup'}
-      let(:additional_install_switches) { ['/ERRORREPORTING=1', '/SQLBACKUPDIR=\'I:\DBbackup\''] }
+      let(:additional_install_switches) { ["/ConfigurationFile=\"#{@file_double.path}\""] }
       let(:args) { args }
       munged = {:features => Array.new(args[:features])}
       munged[:features].delete('SQL')
diff --git a/spec/unit/puppet/provider/sqlserver_features_spec.rb b/spec/unit/puppet/provider/sqlserver_features_spec.rb
@@ -1,20 +1,29 @@
 require 'spec_helper'
 require 'rspec'
 require File.expand_path(File.join(File.dirname(__FILE__), '..', 'sqlserver_spec_helper.rb'))
+require 'mocha'
 
 provider_class = Puppet::Type.type(:sqlserver_features).provider(:mssql)
 
 RSpec.describe provider_class do
   subject { provider_class }
-
-  shared_examples 'create' do |args, munged_args = {}, additional_switches = []|
+  let(:params) { {
+    :name => 'Base features',
+    :source => 'C:\myinstallexecs',
+    :features => %w(BC SSMS)
+  } }
+  let(:additional_params) { {} }
+  let(:munged_args) { {} }
+  let(:additional_switches) { [] }
+  shared_examples 'create' do
     it {
-      @resource = Puppet::Type::Sqlserver_features.new(args)
+      params.merge!(additional_params)
+      @resource = Puppet::Type::Sqlserver_features.new(params)
       @provider = provider_class.new(@resource)
 
       stub_powershell_call(subject)
 
-      executed_args = args.merge(munged_args)
+      executed_args = params.merge(munged_args)
       stub_add_features(executed_args, executed_args[:features], additional_switches)
       @provider.create
     }
@@ -32,21 +41,28 @@
 
   context 'it should provide the correct command default command' do
     include_context 'features'
-    it_should_behave_like 'create', @feature_params
+    it_should_behave_like 'create'
   end
 
   context 'it should provide the correct command default command' do
-    include_context 'features'
-    @feature_params[:install_switches] ={'ERRORREPORTING' => 1, 'SQLBACKUPDIR' => 'I:\DBbackup'}
-    additional_switches = ['/ERRORREPORTING=1', '/SQLBACKUPDIR=\'I:\DBbackup\'']
-    it_should_behave_like 'create', @feature_params, {}, additional_switches
+    before :each do
+      @file_double = Tempfile.new(['sqlconfig', '.ini'])
+      @file_double.stubs(:write)
+      @file_double.stubs(:flush)
+      @file_double.stubs(:close)
+      Tempfile.stubs(:new).with(['sqlconfig', '.ini']).returns(@file_double)
+    end
+    it_should_behave_like 'create' do
+      let(:additional_params) { {:install_switches => {'ERRORREPORTING' => 1, 'SQLBACKUPDIR' => 'I:\DBbackup'}} }
+      let(:additional_switches) { ["/ConfigurationFile=\"#{@file_double.path}\""] }
+    end
   end
 
   context 'it should expand the superset for features' do
     include_context 'features'
-    @feature_params[:features] = %w(Tools)
-    munged = {:features => %w(ADV_SSMS Conn SSMS)}
-    it_should_behave_like 'create', @feature_params, munged
+    let(:additional_params) { {:features => %w(Tools)} }
+    let(:munged_args) { {:features => %w(ADV_SSMS Conn SSMS)} }
+    it_should_behave_like 'create'
   end
 
   shared_examples 'features=' do |args|
