Please review pull request #168: (#12311) use 'ensure' to restore env vars in Resolution.with_env opened by (cprice-puppet)
Description:
This will make sure that the environment variables get restored to their original values, even if the caller's yield block throws an exception or uses a "return" statement.
- Opened: Wed Feb 08 01:30:06 UTC 2012
- Based on: puppetlabs:master (61936e4965ebe3e2cecc16833b1350041dc803e2)
- Requested merge: cprice-puppet:bug/master/12311-with-env-needs-ensure (f77584f4fedf5d85a91573fd4c91ff7686bf9a07)
Diff follows:
diff --git a/lib/facter/util/resolution.rb b/lib/facter/util/resolution.rb
index 690a8cb..7e266c7 100644
--- a/lib/facter/util/resolution.rb
+++ b/lib/facter/util/resolution.rb
@@ -45,6 +45,8 @@ def self.with_env(values)
end
# execute the caller's block, capture the return value
rv = yield
+ # use an ensure block to make absolutely sure we restore the variables
+ ensure
# restore the old values
values.each do |var, value|
if old.include?(var)
@@ -89,14 +91,10 @@ def self.exec(code, interpreter = nil)
else
path = %x{which #{binary} 2>/dev/null}.chomp
# we don't have the binary necessary
- # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
- # without performing the necessary cleanup of environment variables
- next if path == "" or path.match(/Command not found\./)
+ return if path == "" or path.match(/Command not found\./)
end
- # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
- # without performing the necessary cleanup of environment variables
- next unless FileTest.exists?(path)
+ return unless FileTest.exists?(path)
end
out = nil
@@ -105,22 +103,16 @@ def self.exec(code, interpreter = nil)
out = %x{#{code}}.chomp
rescue Errno::ENOENT => detail
# command not found on Windows
- # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
- # without performing the necessary cleanup of environment variables
- next
+ return
rescue => detail
$stderr.puts detail
- # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
- # without performing the necessary cleanup of environment variables
- next
+ return
end
- # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
- # without performing the necessary cleanup of environment variables
if out == ""
- next
+ return
else
- next out
+ return out
end
end
diff --git a/spec/unit/util/resolution_spec.rb b/spec/unit/util/resolution_spec.rb
index 4788d45..e48e81a 100755
--- a/spec/unit/util/resolution_spec.rb
+++ b/spec/unit/util/resolution_spec.rb
@@ -86,6 +86,27 @@
ENV[key].should == orig_env[key]
end
end
+
+ it "should not be affected by a 'return' statement in the yield block" do
+ @sentinel_var = :resolution_test_foo.to_s
+
+ # the intent of this test case is to test a yield block that contains a return statement. However, it's illegal
+ # to use a return statement outside of a method, so we need to create one here to give scope to the 'return'
+ def handy_method()
+ ENV[@sentinel_var] = "foo"
+ new_env = { @sentinel_var => "bar" }
+
+ Facter::Util::Resolution.with_env new_env do
+ ENV[@sentinel_var].should == "bar"
+ return
+ end
+ end
+
+ handy_method()
+
+ ENV[@sentinel_var].should == "foo"
+
+ end
end
describe "when setting the code" do
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
