Please review pull request #593: Bug/master/13284 missing env vars during provider command execution opened by (zaphod42)
Description:
Provide a mechanism for better controlling the execution of commands in providers. This is mainly to fix the macports provider losing HOME.
- Opened: Fri Mar 23 22:10:08 UTC 2012
- Based on: puppetlabs:master (0b900c9548f6c652f0d7271156b69c35aa232ead)
- Requested merge: zaphod42:bug/master/13284-missing-env-vars-during-provider-command-execution (52c393c954601868ac411ef3ad7247dc7bab317d)
Diff follows:
diff --git a/lib/puppet/provider.rb b/lib/puppet/provider.rb
index d902206..0e8d9f1 100644
--- a/lib/puppet/provider.rb
+++ b/lib/puppet/provider.rb
@@ -7,6 +7,7 @@ class Puppet::Provider
extend Puppet::Util::Warnings
require 'puppet/provider/confiner'
+ require 'puppet/provider/command'
extend Puppet::Provider::Confiner
@@ -48,8 +49,12 @@ def self.command(name)
end
# Define commands that are not optional.
- def self.commands(hash)
- optional_commands(hash) do |name, path|
+ #
+ # @param [Hash{String => String, Puppet::Provider::Command}] command_specs Named commands that the provider will
+ # be executing on the system. Each command is specified with a name and the path of the executable or a
+ # Puppet::Provider::Command object.
+ def self.commands(command_specs)
+ optional_commands(command_specs) do |name, path|
confine :exists => path, :for_binary => true
end
end
@@ -107,19 +112,18 @@ def self.instances
end
# Create the methods for a given command.
+ #
+ # @deprecated Use {#commands} or {#optional_commands} instead. This was not meant to be part of a public API
def self.make_command_methods(name)
+ Puppet.deprecation_warning "Provider.make_command_methods is deprecated; use Provider.commands or Provider.optional_commands instead for creating command methods"
+
# Now define a method for that command
unless singleton_class.method_defined?(name)
meta_def(name) do |*args|
raise Puppet::Error, "Command #{name} is missing" unless command(name)
- if args.empty?
- cmd = [command(name)]
- else
- cmd = [command(name)] + args
- end
# This might throw an ExecutionFailure, but the system above
# will catch it, if so.
- return execute(cmd)
+ return Puppet::Provider::Command.new(command(name)).execute(Puppet::Util::Execution, *args)
end
# And then define an instance method that just calls the class method.
@@ -161,17 +165,36 @@ def self.mk_resource_methods
# Define one or more binaries we'll be using. If a block is passed, yield the name
# and path to the block (really only used by 'commands').
+ #
+ # (see #commands)
def self.optional_commands(hash)
- hash.each do |name, path|
+ hash.each do |name, target|
name = symbolize(name)
- @commands[name] = path
+ command = target.is_a?(String) ? Puppet::Provider::Command.new(target) : target
- yield(name, path) if block_given?
+ @commands[name] = command.executable
+
+ yield(name, command.executable) if block_given?
# Now define the class and instance methods.
- make_command_methods(name)
+ create_class_and_instance_method(name) do |*args|
+ return command.execute(name, Puppet::Util, Puppet::Util::Execution, *args)
+ end
+ end
+ end
+
+ def self.create_class_and_instance_method(name, &block)
+ unless singleton_class.method_defined?(name)
+ meta_def(name, &block)
+ end
+
+ unless method_defined?(name)
+ define_method(name) do |*args|
+ self.class.send(name, *args)
+ end
end
end
+ private_class_method :create_class_and_instance_method
# Retrieve the data source. Defaults to the provider name.
def self.source
diff --git a/lib/puppet/provider/command.rb b/lib/puppet/provider/command.rb
new file mode 100644
index 0000000..74f8345
--- /dev/null
+++ b/lib/puppet/provider/command.rb
@@ -0,0 +1,21 @@
+# A command that can be executed on the system
+class Puppet::Provider::Command
+ attr_reader :executable
+
+ # @param [String] executable The path to the executable file
+ # @param [Hash] options Extra options to be used when executing (see Puppet::Util::Execution#execute)
+ def initialize(executable, options = {})
+ @executable = executable
+ @options = options
+ end
+
+ # @param [String] name A way of referencing the name
+ # @param resolver An object for resolving the executable to an absolute path (usually Puppet::Util)
+ # @param executor An object for performing the actual execution of the command (usually Puppet::Util::Execution)
+ # @param [Array<String>] Any command line arguments to pass to the executable
+ # @returns The output from the command
+ def execute(name, resolver, executor, *args)
+ resolved_executable = resolver.which(@executable) or raise Puppet::Error, "Command #{name} is missing"
+ executor.execute([resolved_executable] + args, @options)
+ end
+end
diff --git a/lib/puppet/provider/package/macports.rb b/lib/puppet/provider/package/macports.rb
index 3df29af..43e26d1 100755
--- a/lib/puppet/provider/package/macports.rb
+++ b/lib/puppet/provider/package/macports.rb
@@ -1,4 +1,5 @@
require 'puppet/provider/package'
+require 'puppet/provider/command'
Puppet::Type.type(:package).provide :macports, :parent => Puppet::Provider::Package do
desc "Package management using MacPorts on OS X.
@@ -12,7 +13,7 @@
"
confine :operatingsystem => :darwin
- commands :port => "/opt/local/bin/port"
+ commands :port => Puppet::Provider::Command.new("/opt/local/bin/port", { :custom_environment => { :HOME => ENV['HOME'] } })
has_feature :installable
has_feature :uninstallable
diff --git a/lib/puppet/util/execution.rb b/lib/puppet/util/execution.rb
index 17247ba..7313b1a 100644
--- a/lib/puppet/util/execution.rb
+++ b/lib/puppet/util/execution.rb
@@ -54,8 +54,11 @@ def self.execfail(command, exception)
# Execute the desired command, and return the status and output.
- # def execute(command, arguments)
- # [arguments] a Hash optionally containing any of the following keys:
+ # def execute(command, options)
+ # [command] an Array or String representing the command to execute. If it is
+ # an Array the first element should be the executable and the rest of the
+ # elements should be the individual arguments to that executable.
+ # [options] a Hash optionally containing any of the following keys:
# :failonfail (default true) -- if this value is set to true, then this method will raise an error if the
# command is not executed successfully.
# :uid (default nil) -- the user id of the user that the process should be run as
@@ -70,11 +73,11 @@ def self.execfail(command, exception)
# Passing in a value of false for this option will allow the command to be executed using the user/system locale.
# :custom_environment (default {}) -- a hash of key/value pairs to set as environment variables for the duration
# of the command
- def self.execute(command, arguments = {})
+ def self.execute(command, options = {})
# specifying these here rather than in the method signature to allow callers to pass in a partial
# set of overrides without affecting the default values for options that they don't pass in
- default_arguments = {
+ default_options = {
:failonfail => true,
:uid => nil,
:gid => nil,
@@ -85,7 +88,7 @@ def self.execute(command, arguments = {})
:custom_environment => {},
}
- arguments = default_arguments.merge(arguments)
+ options = default_options.merge(options)
if command.is_a?(Array)
command = command.flatten.map(&:to_s)
@@ -102,11 +105,11 @@ def self.execute(command, arguments = {})
null_file = Puppet.features.microsoft_windows? ? 'NUL' : '/dev/null'
- stdin = File.open(arguments[:stdinfile] || null_file, 'r')
- stdout = arguments[:squelch] ? File.open(null_file, 'w') : Tempfile.new('puppet')
- stderr = arguments[:combine] ? stdout : File.open(null_file, 'w')
+ stdin = File.open(options[:stdinfile] || null_file, 'r')
+ stdout = options[:squelch] ? File.open(null_file, 'w') : Tempfile.new('puppet')
+ stderr = options[:combine] ? stdout : File.open(null_file, 'w')
- exec_args = [command, arguments, stdin, stdout, stderr]
+ exec_args = [command, options, stdin, stdout, stderr]
if execution_stub = Puppet::Util::ExecutionStub.current_value
return execution_stub.call(*exec_args)
@@ -126,12 +129,12 @@ def self.execute(command, arguments = {})
[stdin, stdout, stderr].each {|io| io.close rescue nil}
# read output in if required
- unless arguments[:squelch]
+ unless options[:squelch]
output = wait_for_output(stdout)
Puppet.warning "Could not get output" unless output
end
- if arguments[:failonfail] and exit_status != 0
+ if options[:failonfail] and exit_status != 0
raise ExecutionFailure, "Execution of '#{str}' returned #{exit_status}: #{output}"
end
@@ -152,7 +155,7 @@ class << self
# this is private method, see call to private_class_method after method definition
- def self.execute_posix(command, arguments, stdin, stdout, stderr)
+ def self.execute_posix(command, options, stdin, stdout, stderr)
child_pid = Kernel.fork do
# We can't just call Array(command), and rely on it returning
# things like ['foo'], when passed ['foo'], because
@@ -172,10 +175,10 @@ def self.execute_posix(command, arguments, stdin, stdout, stderr)
# (assumes that there are only 256 file descriptors used)
3.upto(256){|fd| IO::new(fd).close rescue nil}
- Puppet::Util::SUIDManager.change_privileges(arguments[:uid], arguments[:gid], true)
+ Puppet::Util::SUIDManager.change_privileges(options[:uid], options[:gid], true)
# if the caller has requested that we override locale environment variables,
- if (arguments[:override_locale]) then
+ if (options[:override_locale]) then
# loop over them and clear them
Puppet::Util::POSIX::LOCALE_ENV_VARS.each { |name| ENV.delete(name) }
# set LANG and LC_ALL to 'C' so that the command will have consistent, predictable output
@@ -192,8 +195,8 @@ def self.execute_posix(command, arguments, stdin, stdout, stderr)
# a forked process.
Puppet::Util::POSIX::USER_ENV_VARS.each { |name| ENV.delete(name) }
- arguments[:custom_environment] ||= {}
- Puppet::Util.withenv(arguments[:custom_environment]) do
+ options[:custom_environment] ||= {}
+ Puppet::Util.withenv(options[:custom_environment]) do
Kernel.exec(*command)
end
rescue => detail
@@ -207,14 +210,14 @@ def self.execute_posix(command, arguments, stdin, stdout, stderr)
# this is private method, see call to private_class_method after method definition
- def self.execute_windows(command, arguments, stdin, stdout, stderr)
+ def self.execute_windows(command, options, stdin, stdout, stderr)
command = command.map do |part|
part.include?(' ') ? %Q["#{part.gsub(/"/, '\"')}"] : part
end.join(" ") if command.is_a?(Array)
- arguments[:custom_environment] ||= {}
- Puppet::Util.withenv(arguments[:custom_environment]) do
- Puppet::Util::Windows::Process.execute(command, arguments, stdin, stdout, stderr)
+ options[:custom_environment] ||= {}
+ Puppet::Util.withenv(options[:custom_environment]) do
+ Puppet::Util::Windows::Process.execute(command, options, stdin, stdout, stderr)
end
end
private_class_method :execute_windows
diff --git a/spec/unit/property/command_spec.rb b/spec/unit/property/command_spec.rb
new file mode 100755
index 0000000..68c6691
--- /dev/null
+++ b/spec/unit/property/command_spec.rb
@@ -0,0 +1,62 @@
+require 'spec_helper'
+
+require 'puppet/provider/command'
+
+describe Puppet::Provider::Command do
+ let(:name) { "the name" }
+ let(:the_options) { { :option => 1 } }
+ let(:no_options) { {} }
+ let(:executable) { "foo" }
+ let(:executable_absolute_path) { "/foo/bar" }
+
+ let(:executor) { mock('executor') }
+ let(:resolver) { mock('resolver') }
+
+ let(:path_resolves_to_itself) do
+ resolves = Object.new
+ class << resolves
+ def which(path)
+ path
+ end
+ end
+ resolves
+ end
+
+ it "executes a simple command" do
+ executor.expects(:execute).with([executable], no_options)
+
+ command = Puppet::Provider::Command.new(executable)
+ command.execute(name, path_resolves_to_itself, executor)
+ end
+
+ it "executes a command with extra options" do
+ executor.expects(:execute).with([executable], the_options)
+
+ command = Puppet::Provider::Command.new(executable, the_options)
+ command.execute(name, path_resolves_to_itself, executor)
+ end
+
+ it "executes a command with arguments" do
+ executor.expects(:execute).with([executable, "arg1", "arg2"], no_options)
+
+ command = Puppet::Provider::Command.new(executable)
+ command.execute(name, path_resolves_to_itself, executor, "arg1", "arg2")
+ end
+
+ it "resolves to an absolute path for better execution" do
+ resolver.expects(:which).with(executable).returns(executable_absolute_path)
+ executor.expects(:execute).with([executable_absolute_path], no_options)
+
+ command = Puppet::Provider::Command.new(executable)
+ command.execute(name, resolver, executor)
+ end
+
+ it "errors when the executable resolves to nothing" do
+ resolver.expects(:which).with(executable).returns(nil)
+ executor.expects(:execute).never
+
+ command = Puppet::Provider::Command.new(executable)
+
+ lambda { command.execute(name, resolver, executor) }.should raise_error(Puppet::Error, "Command #{name} is missing")
+ end
+end
diff --git a/spec/unit/provider/package/pacman_spec.rb b/spec/unit/provider/package/pacman_spec.rb
index e1e5f14..42458ff 100755
--- a/spec/unit/provider/package/pacman_spec.rb
+++ b/spec/unit/provider/package/pacman_spec.rb
@@ -4,8 +4,13 @@
provider = Puppet::Type.type(:package).provider(:pacman)
describe provider do
+ let(:no_extra_options) { {} }
+ let(:executor) { Puppet::Util::Execution }
+ let(:resolver) { Puppet::Util }
+
before do
- provider.stubs(:command).with(:pacman).returns('/usr/bin/pacman')
+ resolver.stubs(:which).with('/usr/bin/pacman').returns('/usr/bin/pacman')
+ provider.stubs(:which).with('/usr/bin/pacman').returns('/usr/bin/pacman')
@resource = Puppet::Type.type(:package).new(:name => 'package')
@provider = provider.new(@resource)
end
@@ -17,42 +22,18 @@
})
end
- it "should call pacman" do
- provider.
+ it "should call pacman to install the right package quietly" do
+ executor.
expects(:execute).
at_least_once.
- with { |args|
- args[0] == "/usr/bin/pacman"
- }.
+ with(["/usr/bin/pacman", "--noconfirm", "--noprogressbar", "-Sy", @resource[:name]], no_extra_options).
returns ""
@provider.install
end
- it "should be quiet" do
- provider.
- expects(:execute).
- with { |args|
- args[1,2] == ["--noconfirm", "--noprogressbar"]
- }.
- returns("")
-
- @provider.install
- end
-
- it "should install the right package" do
- provider.
- expects(:execute).
- with { |args|
- args[3,4] == ["-Sy", @resource[:name]]
- }.
- returns("")
-
- @provider.install
- end
-
it "should raise an ExecutionFailure if the installation failed" do
- provider.stubs(:execute).returns("")
+ executor.stubs(:execute).returns("")
@provider.expects(:query).returns(nil)
lambda { @provider.install }.should raise_exception(Puppet::ExecutionFailure)
@@ -72,13 +53,13 @@
it "should install #{source} directly" do
@resource[:source] = source
- provider.expects(:execute).
- with(all_of(includes("-Sy"), includes("--noprogressbar"))).
+ executor.expects(:execute).
+ with(all_of(includes("-Sy"), includes("--noprogressbar")), no_extra_options).
in_sequence(@install).
returns("")
- provider.expects(:execute).
- with(all_of(includes("-U"), includes(source))).
+ executor.expects(:execute).
+ with(all_of(includes("-U"), includes(source)), no_extra_options).
in_sequence(@install).
returns("")
@@ -95,14 +76,16 @@
end
it "should install from the path segment of the URL" do
- provider.expects(:execute).
- with(all_of(includes("-Sy"), includes("--noprogressbar"),
- includes("--noconfirm"))).
+ executor.expects(:execute).
+ with(all_of(includes("-Sy"),
+ includes("--noprogressbar"),
+ includes("--noconfirm")),
+ no_extra_options).
in_sequence(@install).
returns("")
- provider.expects(:execute).
- with(all_of(includes("-U"), includes(@actual_file_path))).
+ executor.expects(:execute).
+ with(all_of(includes("-U"), includes(@actual_file_path)), no_extra_options).
in_sequence(@install).
returns("")
@@ -140,45 +123,21 @@
end
describe "when uninstalling" do
- it "should call pacman" do
- provider.
+ it "should call pacman to remove the right package quietly" do
+ executor.
expects(:execute).
- with { |args|
- args[0] == "/usr/bin/pacman"
- }.
+ with(["/usr/bin/pacman", "--noconfirm", "--noprogressbar", "-R", @resource[:name]], no_extra_options).
returns ""
@provider.uninstall
end
-
- it "should be quiet" do
- provider.
- expects(:execute).
- with { |args|
- args[1,2] == ["--noconfirm", "--noprogressbar"]
- }.
- returns("")
-
- @provider.uninstall
- end
-
- it "should remove the right package" do
- provider.
- expects(:execute).
- with { |args|
- args[3,4] == ["-R", @resource[:name]]
- }.
- returns("")
-
- @provider.uninstall
- end
end
describe "when querying" do
it "should query pacman" do
- provider.
+ executor.
expects(:execute).
- with(["/usr/bin/pacman", "-Qi", @resource[:name]])
+ with(["/usr/bin/pacman", "-Qi", @resource[:name]], no_extra_options)
@provider.query
end
@@ -206,17 +165,17 @@
Description : A library-based package manager with dependency support
EOF
- provider.expects(:execute).returns(query_output)
+ executor.expects(:execute).returns(query_output)
@provider.query.should == {:ensure => "1.01.3-2"}
end
it "should return a nil if the package isn't found" do
- provider.expects(:execute).returns("")
+ executor.expects(:execute).returns("")
@provider.query.should be_nil
end
it "should return a hash indicating that the package is missing on error" do
- provider.expects(:execute).raises(Puppet::ExecutionFailure.new("ERROR!"))
+ executor.expects(:execute).raises(Puppet::ExecutionFailure.new("ERROR!"))
@provider.query.should == {
:ensure => :purged,
:status => 'missing',
@@ -266,12 +225,12 @@
describe "when determining the latest version" do
it "should refresh package list" do
get_latest_version = sequence("get_latest_version")
- provider.
+ executor.
expects(:execute).
in_sequence(get_latest_version).
- with(['/usr/bin/pacman', '-Sy'])
+ with(['/usr/bin/pacman', '-Sy'], no_extra_options)
- provider.
+ executor.
stubs(:execute).
in_sequence(get_latest_version).
returns("")
@@ -281,21 +240,21 @@
it "should get query pacman for the latest version" do
get_latest_version = sequence("get_latest_version")
- provider.
+ executor.
stubs(:execute).
in_sequence(get_latest_version)
- provider.
+ executor.
expects(:execute).
in_sequence(get_latest_version).
- with(['/usr/bin/pacman', '-Sp', '--print-format', '%v', @resource[:name]]).
+ with(['/usr/bin/pacman', '-Sp', '--print-format', '%v', @resource[:name]], no_extra_options).
returns("")
@provider.latest
end
it "should return the version number from pacman" do
- provider.
+ executor.
expects(:execute).
at_least_once().
returns("1.00.2-3\n")
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
