(FM-2577) - Change from sqlcmd.exe to win32ole connector #99

cyberious · 2015-05-04T23:17:36Z

remove dependency to sqlcmd.exe and temp files, gives us more control and ability to get error messages back
Login contains GO in sql query which breaks with new method
Removes GO and adds the TRY and CATCH used throughout
Move config to $::vardir/cache/sqlserver instead of in C:/Program Files/Microsoft SQL Server/.puppet

ferventcoder · 2015-05-11T16:55:17Z

lib/puppet/provider/sqlserver_tsql/mssql.rb

@@ -1,17 +1,61 @@
 require File.expand_path(File.join(File.dirname(__FILE__), '..', 'sqlserver'))
+require File.expand_path(File.join(File.dirname(__FILE__), '..', '..', '..', 'puppet_x/sqlserver/sql_connection'))


Wheeeeee. I've never understood when and why things go in puppet_x

This is a convention I believe that came out of moving non puppet related code out. @hunner maybe you could give a better history of this.

That sounds great! I would like to understand the history of this.

Iristyle · 2015-05-11T23:22:43Z

A couple of minor requests:

Can we move the temp directory change out to a separate commit? i.e. the $::vardir/cache/sqlserver instead of in C:/Program Files/Microsoft SQL Server/.puppet bit
The commit message has Login contains GO in sql query which breaks with new method twice

Iristyle · 2015-05-11T23:45:37Z

lib/puppet_x/sqlserver/sql_connection.rb

+      end
+
+      def has_errors
+        return @exception_caught != nil


return is not necessary here

Any reason not to have it? I prefer the explicit for readability but I have not strong opinion either way.

I used to prefer the explicit return as well, but it's not idiomatic Ruby.... and we tend to not use it in any of the newer Puppet core code unless it's for a short-circuit return. It's also nice to be consistent given the file has a mix of both styles.

See some of the newer Windows code in Puppet core - you'll see we only have short-circuit returns.

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/file.rb
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/com.rb
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/api_types.rb
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/security.rb
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/sid.rb
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/taskscheduler.rb

cyberious · 2015-05-12T04:41:32Z

@Iristyle Moved into separate commit for config change

cyberious · 2015-05-12T15:52:13Z

@Iristyle previously we were parsing the JSON in PowerShell which doesn't care about the quotes, however Ruby wants the quotes to always be double quotes instead.

ferventcoder · 2015-05-12T16:17:25Z

.fixtures.yml

@@ -2,6 +2,5 @@ fixtures:
  forge_modules:
    acl: "puppetlabs/acl"
    stdlib: "puppetlabs/stdlib"
-    acl: "puppetlabs/acl"


interesting... duplicate eh?

Yeah I think this came from a merge conflict.

ferventcoder · 2015-05-12T16:32:14Z

lib/puppet_x/sqlserver/sql_connection.rb

+          connection_string << "Data Source=localhost\\#{config['instance']};"
+        else
+          connection_string << "Data Source=localhost;"
+        end


What is this? As in why am I seeing localhost here on either side of the config?

Data source should not always be localhost unless I'm missing something.

So for example we have a named instance on the localhost you have to have \Instance name, if it is however the default instance of MSSQLSERVER and you add localhost\MSSQLSERVER it will fail the connection.

ferventcoder · 2015-05-13T15:22:53Z

bundle exec rake validate was fine until spec/functions/sqlserver_upcase_spec.rb:

ruby -c spec/functions/sqlserver_upcase_spec.rb
spec/functions/sqlserver_upcase_spec.rb:1:in `require': cannot load such file -- spec (LoadError)
rake aborted!
Command failed with status (1): [ruby -c spec/functions/sqlserver_upcase_sp...]
/Users/rob/code/puppetlabs/MODULES/puppetlabs-sqlserver/Rakefile:13:in `block (2 levels) in <top (required)>'
/Users/rob/code/puppetlabs/MODULES/puppetlabs-sqlserver/Rakefile:12:in `each'
/Users/rob/code/puppetlabs/MODULES/puppetlabs-sqlserver/Rakefile:12:in `block in <top (required)>'

I'm guessing it is this line:

#! /usr/bin/env ruby -S rspec

ferventcoder · 2015-05-13T15:42:19Z

I also got a duplicate constant warning when running specs:

rob@skylight:~/code/puppetlabs/modules/puppetlabs-sqlserver [(fd38dfd...)] 10:40:03 $ bundle exec rake spec
/opt/boxen/rbenv/versions/2.1.5/bin/ruby -I/opt/boxen/rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rspec-core-3.2.3/lib:/opt/boxen/rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rspec-support-3.2.2/lib /opt/boxen/rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rspec-core-3.2.3/exe/rspec --pattern spec/\{classes,defines,unit,functions,hosts,integration\}/\*\*/\*_spec.rb --color
/Users/rob/code/puppetlabs/MODULES/puppetlabs-sqlserver/spec/fixtures/modules/sqlserver/lib/puppet_x/sqlserver/sql_connection.rb:6: warning: already initialized constant PuppetX::Sqlserver::SqlConnection::CONNECTION_CLOSED
/Users/rob/code/puppetlabs/MODULES/puppetlabs-sqlserver/lib/puppet_x/sqlserver/sql_connection.rb:6: warning: previous definition of CONNECTION_CLOSED was here
Uninstalling all features for instance MYSQLSERVER because an empty array was passed, please use ensure absent instead.==========>                                |  ETA: 00:00:01
Uninstalling all sql server features not tied into an instance because an empty array was passed, please use ensure absent instead.====>                          |  ETA: 00:00:01
 263/263 |========================================================================= 100 =========================================================================>| Time: 00:00:05


Finished in 5.03 seconds (files took 0.98677 seconds to load)
263 examples, 0 failures

PuppetX::Sqlserver::SqlConnection::CONNECTION_CLOSED

All the specs that could run on OSX passed though.

I think it's likely it just loaded the file twice for specs. As long as it isn't there for normal runs, I have no issues with this.

ferventcoder · 2015-05-13T15:46:12Z

Clean up the validate issue though, please.

- remove dependency to sqlcmd.exe and temp files, gives us more control and ability to get error messages back - Login contains GO in sql query which breaks with new method - Removes GO and adds the TRY and CATCH used throughout - Change config to double quotes, Ruby does not parse JSON with single quotes

cyberious · 2015-05-13T16:51:38Z

@ferventcoder resolved

ferventcoder · 2015-05-13T17:08:34Z

spec/functions/sqlserver_upcase_spec.rb

@@ -1,4 +1,3 @@
-#! /usr/bin/env ruby -S rspec


(FM-2577) - Change from sqlcmd.exe to win32ole connector

Iristyle · 2015-05-13T17:25:52Z

So what happened with attribution?

ferventcoder · 2015-05-13T17:32:22Z

@Iristyle yeah, that still needs to be added. @cyberious would you follow up with that?

ferventcoder · 2015-05-13T17:32:58Z

@Iristyle not a blocker to merge but I agree it should be added.

Iristyle · 2015-05-13T22:48:56Z

Any code remnants have been completely rewritten, so attribution is no longer necessary.

ferventcoder reviewed May 11, 2015
View reviewed changes

Iristyle reviewed May 11, 2015
View reviewed changes

Travis Fields added 2 commits May 11, 2015 17:57

(fix) - Remove duplicate acl dependency in .fixtures.yml

7e5fc9d

(maint) Move config file to vardir instead of C:\Program Files\

d1a29d9

ferventcoder reviewed May 12, 2015
View reviewed changes

cyberious closed this May 13, 2015

cyberious reopened this May 13, 2015

ferventcoder reviewed May 13, 2015
View reviewed changes

spec/functions/sqlserver_upcase_spec.rb

@@ -1,4 +1,3 @@

#! /usr/bin/env ruby -S rspec

Copy link

Contributor

ferventcoder May 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

ferventcoder added a commit that referenced this pull request May 13, 2015

Merge pull request #99 from cyberious/TSqlRefacter

28cf097

(FM-2577) - Change from sqlcmd.exe to win32ole connector

ferventcoder merged commit 28cf097 into puppetlabs:master May 13, 2015

ferventcoder deleted the TSqlRefacter branch May 13, 2015 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(FM-2577) - Change from sqlcmd.exe to win32ole connector #99

(FM-2577) - Change from sqlcmd.exe to win32ole connector #99

cyberious commented May 4, 2015

ferventcoder May 11, 2015

cyberious May 11, 2015

ferventcoder May 11, 2015

Iristyle commented May 11, 2015

Iristyle May 11, 2015

cyberious May 12, 2015

Iristyle May 12, 2015

cyberious commented May 12, 2015

cyberious commented May 12, 2015

ferventcoder May 12, 2015

cyberious May 12, 2015

ferventcoder May 12, 2015

ferventcoder May 12, 2015

ferventcoder May 12, 2015

cyberious May 12, 2015

ferventcoder commented May 13, 2015

ferventcoder commented May 13, 2015

ferventcoder commented May 13, 2015

cyberious commented May 13, 2015

ferventcoder May 13, 2015

Iristyle commented May 13, 2015

ferventcoder commented May 13, 2015

ferventcoder commented May 13, 2015

Iristyle commented May 13, 2015

		@@ -1,17 +1,61 @@
		require File.expand_path(File.join(File.dirname(__FILE__), '..', 'sqlserver'))
		require File.expand_path(File.join(File.dirname(__FILE__), '..', '..', '..', 'puppet_x/sqlserver/sql_connection'))

(FM-2577) - Change from sqlcmd.exe to win32ole connector #99

(FM-2577) - Change from sqlcmd.exe to win32ole connector #99

Conversation

cyberious commented May 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Iristyle commented May 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyberious commented May 12, 2015

cyberious commented May 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferventcoder commented May 13, 2015

ferventcoder commented May 13, 2015

ferventcoder commented May 13, 2015

cyberious commented May 13, 2015

Choose a reason for hiding this comment

Iristyle commented May 13, 2015

ferventcoder commented May 13, 2015

ferventcoder commented May 13, 2015

Iristyle commented May 13, 2015