diff --git a/Gemfile b/Gemfile index 3e457e4..6569931 100644 --- a/Gemfile +++ b/Gemfile @@ -2,12 +2,12 @@ source ENV['GEM_SOURCE'] || "https://rubygems.org" group :development, :test do gem 'rake', '10.1.1' - gem 'puppetlabs_spec_helper', :require => false + gem 'puppetlabs_spec_helper', :require => false gem 'rspec-puppet', '~> 1.0' gem 'puppet-lint', '~> 0.3.2' - gem 'beaker', :require => false - gem 'beaker-rspec', :require => false - gem 'serverspec', :require => false + gem 'beaker', :require => false + gem 'beaker-rspec', '> 2.0.0', :require => false + gem 'serverspec', :require => false end if puppetversion = ENV['PUPPET_GEM_VERSION'] diff --git a/lib/puppet/provider/postgresql_psql/ruby.rb b/lib/puppet/provider/postgresql_psql/ruby.rb index d79f8f6..7dd69ac 100644 --- a/lib/puppet/provider/postgresql_psql/ruby.rb +++ b/lib/puppet/provider/postgresql_psql/ruby.rb @@ -1,55 +1,5 @@ Puppet::Type.type(:postgresql_psql).provide(:ruby) do - def command() - if ((! resource[:unless]) or (resource[:unless].empty?)) - if (resource.refreshonly?) - # So, if there's no 'unless', and we're in "refreshonly" mode, - # we need to return the target command here. If we don't, - # then Puppet will generate an event indicating that this - # property has changed. - return resource[:command] - end - - # if we're not in refreshonly mode, then we return nil, - # which will cause Puppet to sync this property. This - # is what we want if there is no 'unless' value specified. - return nil - end - - if Puppet::PUPPETVERSION.to_f < 4 - output, status = run_unless_sql_command(resource[:unless]) - else - output = run_unless_sql_command(resource[:unless]) - status = output.exitcode - end - - if status != 0 - puts status - self.fail("Error evaluating 'unless' clause: '#{output}'") - end - result_count = output.strip.to_i - if result_count > 0 - # If the 'unless' query returned rows, then we don't want to execute - # the 'command'. Returning the target 'command' here will cause - # Puppet to treat this property as already being 'insync?', so it - # won't call the setter to run the 'command' later. - return resource[:command] - end - - # Returning 'nil' here will cause Puppet to see this property - # as out-of-sync, so it will call the setter later. - nil - end - - def command=(val) - output, status = run_sql_command(val) - - if status != 0 - self.fail("Error executing SQL; psql returned #{status}: '#{output}'") - end - end - - def run_unless_sql_command(sql) # for the 'unless' queries, we wrap the user's query in a 'SELECT COUNT', # which makes it easier to parse and process the output. @@ -75,6 +25,8 @@ Puppet::Type.type(:postgresql_psql).provide(:ruby) do end end + private + def run_command(command, user, group) if Puppet::PUPPETVERSION.to_f < 3.4 Puppet::Util::SUIDManager.run_and_capture(command, user, group) diff --git a/lib/puppet/type/postgresql_psql.rb b/lib/puppet/type/postgresql_psql.rb index 70a056f..d724cf8 100644 --- a/lib/puppet/type/postgresql_psql.rb +++ b/lib/puppet/type/postgresql_psql.rb @@ -10,32 +10,20 @@ Puppet::Type.newtype(:postgresql_psql) do defaultto { @resource[:name] } - def sync(refreshing = false) - # We're overriding 'sync' here in order to do some magic - # in support of providing a 'refreshonly' parameter. This - # is kind of hacky because the logic for 'refreshonly' is - # spread between the type and the provider, but this is - # the least horrible way that I could determine to accomplish - # it. - # - # Note that our overridden version of 'sync' takes a parameter, - # 'refreshing', which the parent version doesn't take. This - # allows us to call the sync method directly from the 'refresh' - # method, and then inside of the body of 'sync' we can tell - # whether or not we're refreshing. - - if (!@resource.refreshonly? || refreshing) - # If we're not in 'refreshonly' mode, or we're not currently - # refreshing, then we just call the parent method. - super() + # If needing to run the SQL command, return a fake value that will trigger + # a sync, else return the expected SQL command so no sync takes place + def retrieve + if @resource.should_run_sql + return :notrun else - # If we get here, it means we're in 'refreshonly' mode and - # we're not being called by the 'refresh' method, so we - # just no-op. We'll be called again by the 'refresh' - # method momentarily. - nil + return self.should end end + + def sync + output, status = provider.run_sql_command(value) + self.fail("Error executing SQL; psql returned #{status}: '#{output}'") unless status == 0 + end end newparam(:unless) do @@ -43,6 +31,21 @@ Puppet::Type.newtype(:postgresql_psql) do "this is generally intended to be used for idempotency, to check " + "for the existence of an object in the database to determine whether " + "or not the main SQL command needs to be executed at all." + + # Return true if a matching row is found + def matches(value) + if Puppet::PUPPETVERSION.to_f < 4 + output, status = provider.run_unless_sql_command(value) + else + output = provider.run_unless_sql_command(value) + status = output.exitcode + end + self.fail("Error evaluating 'unless' clause, returned #{status}: '#{output}'") unless status == 0 + + result_count = output.strip.to_i + self.debug("Found #{result_count} row(s) executing 'unless' clause") + result_count > 0 + end end newparam(:db) do @@ -84,10 +87,15 @@ Puppet::Type.newtype(:postgresql_psql) do newvalues(:true, :false) end - def refresh() - # All of the magic for this type is attached to the ':command' property, so - # we just need to sync it to accomplish a 'refresh'. - self.property(:command).sync(true) + def should_run_sql(refreshing = false) + unless_param = @parameters[:unless] + return false if !unless_param.nil? && !unless_param.value.nil? && unless_param.matches(unless_param.value) + return false if !refreshing && @parameters[:refreshonly].value == :true + true + end + + def refresh + self.property(:command).sync if self.should_run_sql(true) end end diff --git a/spec/acceptance/postgresql_psql_spec.rb b/spec/acceptance/postgresql_psql_spec.rb index 5c03a7a..aab46fd 100644 --- a/spec/acceptance/postgresql_psql_spec.rb +++ b/spec/acceptance/postgresql_psql_spec.rb @@ -6,6 +6,22 @@ describe 'postgresql_psql:', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osf apply_manifest("class { 'postgresql::server': ensure => absent }", :catch_failures => true) end + it 'should always run SQL' do + pp = <<-EOS.unindent + class { 'postgresql::server': } + + postgresql_psql { 'foobar': + db => 'postgres', + psql_user => 'postgres', + command => 'select 1', + require => Class['postgresql::server'], + } + EOS + + apply_manifest(pp, :catch_failures => true) + apply_manifest(pp, :catch_failures => true) + end + it 'should run some SQL when the unless query returns no rows' do pp = <<-EOS.unindent class { 'postgresql::server': } @@ -40,4 +56,79 @@ describe 'postgresql_psql:', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osf apply_manifest(pp, :catch_changes => true) end + it 'should not run SQL when refreshed and the unless query returns rows' do + pp = <<-EOS.unindent + class { 'postgresql::server': } + + notify { 'trigger': } ~> + postgresql_psql { 'foobar': + db => 'postgres', + psql_user => 'postgres', + command => 'invalid sql statement', + unless => 'select 1 where 1=1', + require => Class['postgresql::server'], + } + EOS + + apply_manifest(pp, :catch_failures => true) + apply_manifest(pp, :catch_failures => true) + end + + context 'with refreshonly' do + it 'should not run SQL when the unless query returns no rows' do + pp = <<-EOS.unindent + class { 'postgresql::server': } + + postgresql_psql { 'foobar': + db => 'postgres', + psql_user => 'postgres', + command => 'select 1', + unless => 'select 1 where 1=2', + refreshonly => true, + require => Class['postgresql::server'], + } + EOS + + apply_manifest(pp, :catch_failures => true) + apply_manifest(pp, :catch_changes => true) + end + + it 'should run SQL when refreshed and the unless query returns no rows' do + pp = <<-EOS.unindent + class { 'postgresql::server': } + + notify { 'trigger': } ~> + postgresql_psql { 'foobar': + db => 'postgres', + psql_user => 'postgres', + command => 'select 1', + unless => 'select 1 where 1=2', + refreshonly => true, + require => Class['postgresql::server'], + } + EOS + + apply_manifest(pp, :catch_failures => true) + apply_manifest(pp, :catch_failures => true) + end + + it 'should not run SQL when refreshed and the unless query returns rows' do + pp = <<-EOS.unindent + class { 'postgresql::server': } + + notify { 'trigger': } ~> + postgresql_psql { 'foobar': + db => 'postgres', + psql_user => 'postgres', + command => 'invalid sql query', + unless => 'select 1 where 1=1', + refreshonly => true, + require => Class['postgresql::server'], + } + EOS + + apply_manifest(pp, :catch_failures => true) + apply_manifest(pp, :catch_failures => true) + end + end end diff --git a/spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb b/spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb index 325e3db..fb640c1 100644 --- a/spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb +++ b/spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb @@ -66,75 +66,14 @@ describe Puppet::Type.type(:postgresql_psql).provider(:ruby) do provider.run_sql_command("SELECT something") end end - end - context("#command") do - context "when unless is specified" do - [:true, :false, true, false].each do |refresh| - context "and refreshonly is #{refresh}" do - let(:attributes) { { - :command => 'SELECT something', - :db => 'spec_db', - :unless => 'SELECT something', - :refreshonly => refresh - } } + context("#run_unless_sql_command") do + let(:attributes) do { } end - it "does not fail when the status is successful" do - expect(provider).to receive(:run_unless_sql_command).and_return ["1 row returned", 0] - provider.command - end - - it "returns the given command when rows are returned" do - expect(provider).to receive(:run_unless_sql_command).and_return ["1 row returned", 0] - expect(provider.command).to eq("SELECT something") - end - - it "does not return the given command when no rows are returned" do - expect(provider).to receive(:run_unless_sql_command).and_return ["0 rows returned", 0] - expect(provider.command).to_not eq("SELECT something") - end - - it "raises an error when the sql command fails" do - allow(provider).to receive(:run_unless_sql_command).and_return ["Something went wrong", 1] - expect { provider.command }.to raise_error(Puppet::Error, /Something went wrong/) - end - end - end - end - - context "when unless is not specified" do - context "and refreshonly is true" do - let(:attributes) do { - :command => 'SELECT something', - :db => 'spec_db', - :refreshonly => :true - } end - it "does not run unless sql command" do - expect(provider).to_not receive(:run_unless_sql_command) - provider.command - end - - it "returns the given command do disable sync" do - expect(provider.command).to eq("SELECT something") - end - end - - context "and refreshonly is false" do - let(:attributes) do { - :command => 'SELECT something', - :db => 'spec_db', - :refreshonly => :false - } end - it "does not run unless sql command" do - expect(provider).to_not receive(:run_unless_sql_command) - provider.command - end - - it "does not return the command so as to enable sync" do - expect(provider.command).to_not eq("SELECT something") - end - end + it "calls #run_sql_command with SQL" do + expect(provider).to receive(:run_sql_command).with('SELECT COUNT(*) FROM (SELECT 1) count') + provider.run_unless_sql_command('SELECT 1') end end end diff --git a/spec/unit/puppet/type/postgresql_psql_spec.rb b/spec/unit/puppet/type/postgresql_psql_spec.rb index e89c05f..6abdb91 100644 --- a/spec/unit/puppet/type/postgresql_psql_spec.rb +++ b/spec/unit/puppet/type/postgresql_psql_spec.rb @@ -44,49 +44,182 @@ describe Puppet::Type.type(:postgresql_psql), :unless => Puppet.features.microso its([:psql_user]) { should eq("postgres") } its([:psql_group]) { should eq("postgres") } its([:cwd]) { should eq("/tmp") } - its(:refreshonly?) { should be_false } + its(:refreshonly?) { should be_falsey } end end - describe "#refreshonly" do - [true, :true].each do |refreshonly| - context "=> #{refreshonly.inspect}" do - let(:attributes) do { :refreshonly => refreshonly } end - it "has a value of true" do - expect(subject.refreshonly?).to be_true + describe "#command" do + let(:attributes) do {:command => 'SELECT stuff'} end + + it "will have the value :notrun if the command should execute" do + expect(subject).to receive(:should_run_sql).and_return(true) + expect(subject.property(:command).retrieve).to eq(:notrun) + end + + it "will be the 'should' value if the command should not execute" do + expect(subject).to receive(:should_run_sql).and_return(false) + expect(subject.property(:command).retrieve).to eq('SELECT stuff') + end + + it "will call provider#run_sql_command on sync" do + expect(subject.provider).to receive(:run_sql_command).with('SELECT stuff').and_return(["done", 0]) + subject.property(:command).sync + end + end + + describe "#unless" do + let(:attributes) do {:unless => 'SELECT something'} end + + describe "#matches" do + it "does not fail when the status is successful" do + expect(subject.provider).to receive(:run_unless_sql_command).and_return ["1 row returned", 0] + subject.parameter(:unless).matches('SELECT something') + end + + it "returns true when rows are returned" do + expect(subject.provider).to receive(:run_unless_sql_command).and_return ["1 row returned", 0] + expect(subject.parameter(:unless).matches('SELECT something')).to be_truthy + end + + it "returns false when no rows are returned" do + expect(subject.provider).to receive(:run_unless_sql_command).and_return ["0 rows returned", 0] + expect(subject.parameter(:unless).matches('SELECT something')).to be_falsey + end + + it "raises an error when the sql command fails" do + allow(subject.provider).to receive(:run_unless_sql_command).and_return ["Something went wrong", 1] + expect { + subject.parameter(:unless).matches('SELECT something') + }.to raise_error(Puppet::Error, /Something went wrong/) + end + end + end + + describe "#should_run_sql" do + context "without 'unless'" do + [true, :true].each do |refreshonly| + context "refreshonly => #{refreshonly.inspect}" do + let(:attributes) do { + :refreshonly => refreshonly, + } end + + context "not refreshing" do + it { expect(subject.should_run_sql).to be_falsey } + end + + context "refreshing" do + it { expect(subject.should_run_sql(true)).to be_truthy } + end end - it "will not enforce command on sync because refresh() will be called" do - expect(subject.provider).to_not receive(:command=) - subject.property(:command).sync + end + + [false, :false].each do |refreshonly| + context "refreshonly => #{refreshonly.inspect}" do + let(:attributes) do { + :refreshonly => refreshonly, + } end + + context "not refreshing" do + it { expect(subject.should_run_sql).to be_truthy } + end + + context "refreshing" do + it { expect(subject.should_run_sql(true)).to be_truthy } + end end end end - [false, :false].each do |refreshonly| - context "=> #{refreshonly.inspect}" do - let(:attributes) do { :refreshonly => refreshonly } end - it "has a value of false" do - expect(subject.refreshonly?).to be_false + context "with matching 'unless'" do + before { expect(subject.parameter(:unless)).to receive(:matches).with('SELECT something').and_return(true) } + + [true, :true].each do |refreshonly| + context "refreshonly => #{refreshonly.inspect}" do + let(:attributes) do { + :refreshonly => refreshonly, + :unless => 'SELECT something', + } end + + context "not refreshing" do + it { expect(subject.should_run_sql).to be_falsey } + end + + context "refreshing" do + it { expect(subject.should_run_sql(true)).to be_falsey } + end end - it "will enforce command on sync because refresh() will not be called" do - expect(subject.provider).to receive(:command=) - subject.property(:command).sync + end + + [false, :false].each do |refreshonly| + context "refreshonly => #{refreshonly.inspect}" do + let(:attributes) do { + :refreshonly => refreshonly, + :unless => 'SELECT something', + } end + + context "not refreshing" do + it { expect(subject.should_run_sql).to be_falsey } + end + + context "refreshing" do + it { expect(subject.should_run_sql(true)).to be_falsey } + end + end + end + end + + context "when not matching 'unless'" do + before { expect(subject.parameter(:unless)).to receive(:matches).with('SELECT something').and_return(false) } + + [true, :true].each do |refreshonly| + context "refreshonly => #{refreshonly.inspect}" do + let(:attributes) do { + :refreshonly => refreshonly, + :unless => 'SELECT something', + } end + + context "not refreshing" do + it { expect(subject.should_run_sql).to be_falsey } + end + + context "refreshing" do + it { expect(subject.should_run_sql(true)).to be_truthy } + end + end + end + + [false, :false].each do |refreshonly| + context "refreshonly => #{refreshonly.inspect}" do + let(:attributes) do { + :refreshonly => refreshonly, + :unless => 'SELECT something', + } end + + context "not refreshing" do + it { expect(subject.should_run_sql).to be_truthy } + end + + context "refreshing" do + it { expect(subject.should_run_sql(true)).to be_truthy } + end end end end end - ## If we refresh the resource, the command should always be run regardless of - ## refreshonly - describe "when responding to refresh" do - [true, :true, false, :false].each do |refreshonly| - context "with refreshonly => #{refreshonly.inspect}" do - let(:attributes) do { :refreshonly => refreshonly } end - it "will enforce command on sync" do - expect(subject.provider).to receive(:command=) - subject.refresh - end - end + describe "#refresh" do + let(:attributes) do {} end + + it "syncs command property when command should run" do + expect(subject).to receive(:should_run_sql).with(true).and_return(true) + expect(subject.property(:command)).to receive(:sync) + subject.refresh + end + + it "does not sync command property when command should not run" do + expect(subject).to receive(:should_run_sql).with(true).and_return(false) + expect(subject.property(:command)).not_to receive(:sync) + subject.refresh end end end