From 344aef91a3589ce3fcc4dfb00df16762ceef81d5 Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Sat, 6 Aug 2011 17:46:20 +0100 Subject: (#8808) Fail Augeas resource when unable to save changes Raise a failure when Augeas changes cannot be saved (due to invalid layout of the tree, permissions etc). Fixes a regression. --- lib/puppet/provider/augeas/augeas.rb | 8 +++++--- spec/unit/provider/augeas/augeas_spec.rb | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/puppet/provider/augeas/augeas.rb b/lib/puppet/provider/augeas/augeas.rb index 95142568e..eb9c69ac8 100644 --- a/lib/puppet/provider/augeas/augeas.rb +++ b/lib/puppet/provider/augeas/augeas.rb @@ -290,8 +290,10 @@ Puppet::Type.type(:augeas).provide(:augeas) do set_augeas_save_mode(SAVE_NEWFILE) do_execute_changes save_result = @aug.save + fail("Save failed with return code #{save_result}") unless save_result + saved_files = @aug.match("/augeas/events/saved") - if save_result and saved_files.size > 0 + if saved_files.size > 0 root = resource[:root].sub(/^\/$/, "") saved_files.each do |key| saved_file = @aug.get(key).to_s.sub(/^\/files/, root) @@ -305,13 +307,13 @@ Puppet::Type.type(:augeas).provide(:augeas) do debug("Files changed, should execute") return_value = true else - debug("Skipping because no files were changed or save failed") + debug("Skipping because no files were changed") return_value = false end end end ensure - if not return_value or resource.noop? + if not return_value or resource.noop? or not save_result close_augeas end end diff --git a/spec/unit/provider/augeas/augeas_spec.rb b/spec/unit/provider/augeas/augeas_spec.rb index 434a99d70..874f70a8d 100755 --- a/spec/unit/provider/augeas/augeas_spec.rb +++ b/spec/unit/provider/augeas/augeas_spec.rb @@ -442,6 +442,20 @@ describe provider_class do @provider.expects(:diff).with("#{file}", "#{file}.augnew").returns("") @provider.should be_need_to_run end + + it "should fail with an error if saving fails" do + file = "/etc/hosts" + + @resource[:context] = "/files" + @resource[:changes] = ["set #{file}/foo bar"] + + @augeas_stub.stubs(:save).returns(false) + @augeas_stub.stubs(:match).with("/augeas/events/saved").returns([]) + @augeas_stub.expects(:close) + + @provider.expects(:diff).never() + lambda { @provider.need_to_run? }.should raise_error + end end end -- cgit From b52fbf4e3c8cac2956c8a0d301f91d8eb081f994 Mon Sep 17 00:00:00 2001 From: nfagerlund Date: Mon, 15 Aug 2011 17:11:00 -0700 Subject: (#8612) Clarify the function of the example for exec's "creates" parameter It was not clear to all readers that /var/tmp/myfile was being extracted from the tarball. This commit adds a sentence to make the conditions when the exec will run more explicit and fixes an error in the tar command. --- lib/puppet/type/exec.rb | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/puppet/type/exec.rb b/lib/puppet/type/exec.rb index 3ba488f19..35e0c96d7 100755 --- a/lib/puppet/type/exec.rb +++ b/lib/puppet/type/exec.rb @@ -311,17 +311,20 @@ module Puppet end newcheck(:creates, :parent => Puppet::Parameter::Path) do - desc "A file that this command creates. If this + desc <<-EOT + A file that this command creates. If this parameter is provided, then the command will only be run - if the specified file does not exist: + if the specified file does not exist. - exec { \"tar xf /my/tar/file.tar\": - cwd => \"/var/tmp\", - creates => \"/var/tmp/myfile\", - path => [\"/usr/bin\", \"/usr/sbin\"] + exec { "tar -xf /Volumes/nfs02/important.tar": + cwd => "/var/tmp", + creates => "/var/tmp/myfile", + path => ["/usr/bin", "/usr/sbin"] } - " + In this example, if `/var/tmp/myfile` is ever deleted, the exec + will bring it back by re-extracting the tarball. + EOT accept_arrays -- cgit From 2297899e76dd3b65787768f2e4bf6b74b95a3d66 Mon Sep 17 00:00:00 2001 From: Jacob Helwig Date: Mon, 15 Aug 2011 17:07:51 -0700 Subject: Do not leak indirector state from apply tests Since the indirector state persists across tests, we were seeing order dependent test failures with tests that assumed the default indirector settings. Specifically, if the following tests were run in order, the first would cause failures in the second two: spec/unit/application/apply_spec.rb spec/unit/node_spec.rb spec/integration/node_spec.rb To protect against this state leakage, we now: - reset the Puppet::Node terminus before each test in spec/integration/node_spec.rb to ensure we are testing a clean environment. - reset the Puppet::Node, and Puppet::Node::Facts terminus, and cache class after each test in spec/unit/application/apply_spec.rb to prevent leakage into other tests. Since the cache class has the same state leakage problem as the terminus class, but does not have the same ability to lazily populate the default when set to nil, we remove the test. Testing the default for the cache class would require running the test before all other tests to ensure there is no state pollution.n --- spec/integration/node_spec.rb | 3 +++ spec/unit/application/apply_spec.rb | 10 ++++++++-- spec/unit/node_spec.rb | 7 +------ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/spec/integration/node_spec.rb b/spec/integration/node_spec.rb index 4ea6142e2..b81a1fdc3 100755 --- a/spec/integration/node_spec.rb +++ b/spec/integration/node_spec.rb @@ -10,6 +10,9 @@ require 'puppet/node' describe Puppet::Node do describe "when delegating indirection calls" do before do + Puppet::Node.indirection.reset_terminus_class + Puppet::Node.indirection.cache_class = nil + @name = "me" @node = Puppet::Node.new(@name) end diff --git a/spec/unit/application/apply_spec.rb b/spec/unit/application/apply_spec.rb index 489f4db36..c1f85d501 100755 --- a/spec/unit/application/apply_spec.rb +++ b/spec/unit/application/apply_spec.rb @@ -12,6 +12,14 @@ describe Puppet::Application::Apply do Puppet::Util::Log.stubs(:newdestination) end + after :each do + Puppet::Node::Facts.indirection.reset_terminus_class + Puppet::Node::Facts.indirection.cache_class = nil + + Puppet::Node.indirection.reset_terminus_class + Puppet::Node.indirection.cache_class = nil + end + [:debug,:loadclasses,:verbose,:use_nodes,:detailed_exitcodes].each do |option| it "should declare handle_#{option} method" do @apply.should respond_to("handle_#{option}".to_sym) @@ -48,7 +56,6 @@ describe Puppet::Application::Apply do end describe "during setup" do - before :each do Puppet::Log.stubs(:newdestination) Puppet.stubs(:parse_config) @@ -111,7 +118,6 @@ describe Puppet::Application::Apply do end describe "when executing" do - it "should dispatch to 'apply' if it was called with 'apply'" do @apply.options[:catalog] = "foo" diff --git a/spec/unit/node_spec.rb b/spec/unit/node_spec.rb index c15093d90..339054d55 100755 --- a/spec/unit/node_spec.rb +++ b/spec/unit/node_spec.rb @@ -130,14 +130,9 @@ end describe Puppet::Node, "when indirecting" do it "should default to the 'plain' node terminus" do Puppet::Node.indirection.reset_terminus_class - Puppet::Node.indirection.terminus_class.should == :plain - end - it "should not have a cache class defined" do - Puppet::Node.indirection.cache_class.should be_nil - end + Puppet::Node.indirection.terminus_class.should == :plain - after do Puppet::Util::Cacher.expire end end -- cgit From b28bcb031346cfd2026361ec5ffb420c1dcf60d7 Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Mon, 15 Aug 2011 15:51:37 -0700 Subject: (#5495) Remove dead Windows-specific code from posix exec provider Because this provider only applies when the posix feature is present (and thus not the windows feature), it can never be used on Windows. Thus, the Windows-specific command handling is unnecessary and unused. Also added more specific error messages for the cases where a command doesn't exist, isn't a file, and isn't executable. These only apply when the command path is absolute (otherwise the message is simply command not found). Reviewed-By: Matt Robinson --- lib/puppet/provider/exec/posix.rb | 32 +++--- spec/unit/provider/exec/posix_spec.rb | 209 +++++++++++++++++----------------- 2 files changed, 120 insertions(+), 121 deletions(-) diff --git a/lib/puppet/provider/exec/posix.rb b/lib/puppet/provider/exec/posix.rb index 157d0f28d..782f1eac6 100644 --- a/lib/puppet/provider/exec/posix.rb +++ b/lib/puppet/provider/exec/posix.rb @@ -76,26 +76,26 @@ Puppet::Type.type(:exec).provide :posix do def checkexe(command) exe = extractexe(command) - if resource[:path] - if Puppet.features.posix? and !File.exists?(exe) - withenv :PATH => resource[:path].join(File::PATH_SEPARATOR) do - exe = which(exe) || raise(ArgumentError,"Could not find command '#{exe}'") - end - elsif Puppet.features.microsoft_windows? and !File.exists?(exe) - resource[:path].each do |path| - [".exe", ".ps1", ".bat", ".com", ""].each do |extension| - file = File.join(path, exe+extension) - return if File.exists?(file) - end - end + if File.expand_path(exe) == exe + if !File.exists?(exe) + raise ArgumentError, "Could not find command '#{exe}'" + elsif !File.file?(exe) + raise ArgumentError, "'#{exe}' is a #{File.ftype(exe)}, not a file" + elsif !File.executable?(exe) + raise ArgumentError, "'#{exe}' is not executable" end + return end - raise ArgumentError, "Could not find command '#{exe}'" unless File.exists?(exe) - unless File.executable?(exe) - raise ArgumentError, - "'#{exe}' is not executable" + if resource[:path] + withenv :PATH => resource[:path].join(File::PATH_SEPARATOR) do + return if which(exe) + end end + + # 'which' will only return the command if it's executable, so we can't + # distinguish not found from not executable + raise ArgumentError, "Could not find command '#{exe}'" end def extractexe(command) diff --git a/spec/unit/provider/exec/posix_spec.rb b/spec/unit/provider/exec/posix_spec.rb index 50697d826..876b9724d 100755 --- a/spec/unit/provider/exec/posix_spec.rb +++ b/spec/unit/provider/exec/posix_spec.rb @@ -1,120 +1,119 @@ #!/usr/bin/env rspec require 'spec_helper' -provider_class = Puppet::Type.type(:exec).provider(:posix) +describe Puppet::Type.type(:exec).provider(:posix) do + include PuppetSpec::Files + + def make_exe + command = tmpfile('my_command') + FileUtils.touch(command) + File.chmod(0755, command) + command + end + + let(:resource) { Puppet::Type.type(:exec).new(:title => '/foo') } + let(:provider) { described_class.new(resource) } -describe provider_class do before :each do - @resource = Puppet::Resource.new(:exec, 'foo') - @provider = provider_class.new(@resource) + Puppet.features.stubs(:posix?).returns(true) + Puppet.features.stubs(:microsoft_windows?).returns(false) + end + + describe "#validatecmd" do + it "should fail if no path is specified and the command is not fully qualified" do + expect { provider.validatecmd("foo") }.to raise_error( + Puppet::Error, + "'foo' is not qualified and no path was specified. Please qualify the command or specify a path." + ) + end + + it "should pass if a path is given" do + provider.resource[:path] = ['/bogus/bin'] + provider.validatecmd("../foo") + end + + it "should pass if command is fully qualifed" do + provider.resource[:path] = ['/bogus/bin'] + provider.validatecmd("/bin/blah/foo") + end end - ["posix", "microsoft_windows"].each do |feature| - describe "when in #{feature} environment" do - before :each do - if feature == "microsoft_windows" - Puppet.features.stubs(:microsoft_windows?).returns(true) - Puppet.features.stubs(:posix?).returns(false) - else - Puppet.features.stubs(:posix?).returns(true) - Puppet.features.stubs(:microsoft_windows?).returns(false) - end + describe "#run" do + describe "when the command is an absolute path" do + let(:command) { tmpfile('foo') } + + it "should fail if the command doesn't exist" do + expect { provider.run(command) }.to raise_error(ArgumentError, "Could not find command '#{command}'") + end + + it "should fail if the command isn't a file" do + FileUtils.mkdir(command) + FileUtils.chmod(0755, command) + + expect { provider.run(command) }.to raise_error(ArgumentError, "'#{command}' is a directory, not a file") + end + + it "should fail if the command isn't executable" do + FileUtils.touch(command) + + expect { provider.run(command) }.to raise_error(ArgumentError, "'#{command}' is not executable") + end + end + + describe "when the command is a relative path" do + it "should execute the command if it finds it in the path and is executable" do + command = make_exe + provider.resource[:path] = [File.dirname(command)] + filename = File.basename(command) + + Puppet::Util.expects(:execute).with { |cmdline, arguments| (cmdline == [filename]) && (arguments.is_a? Hash) } + + provider.run(filename) end - describe "#validatecmd" do - it "should fail if no path is specified and the command is not fully qualified" do - lambda { @provider.validatecmd("foo") }.should raise_error( - Puppet::Error, - "'foo' is not qualified and no path was specified. Please qualify the command or specify a path." - ) - end - - it "should pass if a path is given" do - @provider.resource[:path] = ['/bogus/bin'] - @provider.validatecmd("../foo") - end - - it "should pass if command is fully qualifed" do - @provider.resource[:path] = ['/bogus/bin'] - @provider.validatecmd("/bin/blah/foo") - end + it "should fail if the command isn't in the path" do + resource[:path] = ["/fake/path"] + + expect { provider.run('foo') }.to raise_error(ArgumentError, "Could not find command 'foo'") end - describe "#run" do - it "should fail if no path is specified and command does not exist" do - lambda { @provider.run("foo") }.should raise_error(ArgumentError, "Could not find command 'foo'") - end - - it "should fail if the command isn't in the path" do - @provider.resource[:path] = ['/bogus/bin'] - lambda { @provider.run("foo") }.should raise_error(ArgumentError, "Could not find command 'foo'") - end - - it "should fail if the command isn't executable" do - @provider.resource[:path] = ['/bogus/bin'] - File.stubs(:exists?).with("foo").returns(true) - - lambda { @provider.run("foo") }.should raise_error(ArgumentError, "'foo' is not executable") - end - - it "should not be able to execute shell builtins" do - @provider.resource[:path] = ['/bin'] - lambda { @provider.run("cd ..") }.should raise_error(ArgumentError, "Could not find command 'cd'") - end - - it "should execute the command if the command given includes arguments or subcommands" do - @provider.resource[:path] = ['/bogus/bin'] - File.stubs(:exists?).returns(false) - File.stubs(:exists?).with("foo").returns(true) - File.stubs(:executable?).with("foo").returns(true) - - Puppet::Util.expects(:execute).with() { |command, arguments| (command == ['foo bar --sillyarg=true --blah']) && (arguments.is_a? Hash) } - @provider.run("foo bar --sillyarg=true --blah") - end - - it "should fail if quoted command doesn't exist" do - @provider.resource[:path] = ['/bogus/bin'] - File.stubs(:exists?).returns(false) - File.stubs(:exists?).with("foo").returns(true) - File.stubs(:executable?).with("foo").returns(true) - - lambda { @provider.run('"foo bar --sillyarg=true --blah"') }.should raise_error(ArgumentError, "Could not find command 'foo bar --sillyarg=true --blah'") - end - - it "should execute the command if it finds it in the path and is executable" do - @provider.resource[:path] = ['/bogus/bin'] - File.stubs(:exists?).with("foo").returns(true) - File.stubs(:executable?).with("foo").returns(true) - Puppet::Util.expects(:execute).with() { |command, arguments| (command == ['foo']) && (arguments.is_a? Hash) } - - @provider.run("foo") - end - - if feature == "microsoft_windows" - [".exe", ".ps1", ".bat", ".com", ""].each do |extension| - it "should check file extension #{extension} when it can't find the executable" do - @provider.resource[:path] = ['/bogus/bin'] - File.stubs(:exists?).returns(false) - File.stubs(:exists?).with("/bogus/bin/foo#{extension}").returns(true) - File.stubs(:executable?).with("foo").returns(true) - Puppet::Util.expects(:execute).with() { |command, arguments| (command == ['foo']) && (arguments.is_a? Hash) } - - @provider.run("foo") - end - end - end - - it "should warn if you're overriding something in environment" do - @provider.resource[:environment] = ['WHATEVER=/something/else', 'WHATEVER=/foo'] - File.stubs(:exists?).returns(false) - File.stubs(:exists?).with("foo").returns(true) - File.stubs(:executable?).with("foo").returns(true) - - Puppet::Util.expects(:execute).with() { |command, arguments| (command == ['foo']) && (arguments.is_a? Hash) } - @provider.run("foo") - @logs.map {|l| "#{l.level}: #{l.message}" }.should == ["warning: Overriding environment setting 'WHATEVER' with '/foo'"] - end + it "should fail if the command is in the path but not executable" do + command = tmpfile('foo') + FileUtils.touch(command) + resource[:path] = [File.dirname(command)] + filename = File.basename(command) + + expect { provider.run(filename) }.to raise_error(ArgumentError, "Could not find command '#{filename}'") end end + + it "should not be able to execute shell builtins" do + provider.resource[:path] = ['/bin'] + expect { provider.run("cd ..") }.to raise_error(ArgumentError, "Could not find command 'cd'") + end + + it "should execute the command if the command given includes arguments or subcommands" do + provider.resource[:path] = ['/bogus/bin'] + command = make_exe + + Puppet::Util.expects(:execute).with { |cmdline, arguments| (cmdline == ["#{command} bar --sillyarg=true --blah"]) && (arguments.is_a? Hash) } + provider.run("#{command} bar --sillyarg=true --blah") + end + + it "should fail if quoted command doesn't exist" do + provider.resource[:path] = ['/bogus/bin'] + command = "/foo bar --sillyarg=true --blah" + + expect { provider.run(%Q["#{command}"]) }.to raise_error(ArgumentError, "Could not find command '#{command}'") + end + + it "should warn if you're overriding something in environment" do + provider.resource[:environment] = ['WHATEVER=/something/else', 'WHATEVER=/foo'] + command = make_exe + + Puppet::Util.expects(:execute).with { |cmdline, arguments| (cmdline== [command]) && (arguments.is_a? Hash) } + provider.run(command) + @logs.map {|l| "#{l.level}: #{l.message}" }.should == ["warning: Overriding environment setting 'WHATEVER' with '/foo'"] + end end end -- cgit