-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Escape bundler environment when shelling out to fix #190 #191
Changes from 4 commits
d3a13e0
62227d0
5272a6f
7a82d83
202d5fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
--color | ||
-fd | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -291,6 +291,15 @@ def render_template | |
end | ||
end | ||
|
||
# Gets the character that separates elements of the PATH | ||
# environment variable for this platform. | ||
# | ||
# @return [String] ":" on POSIX and ";" on Windows | ||
# @api private | ||
def path_elem_separator | ||
RUBY_PLATFORM =~ /mswin|mingw|windows/ ? ";" : ":" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does File::PATH_SEPARATOR not work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the constant I was looking for. I knew I'd seen it somewhere. |
||
end | ||
|
||
# Convenience method to run a command locally. | ||
# | ||
# @param cmd [String] command to run locally | ||
|
@@ -310,11 +319,36 @@ def run(cmd, options = {}) | |
# * `:log_subject` defaults to a String representation of the Driver's | ||
# class name | ||
# | ||
# Since vagrant does not support being run through bundler, we escape | ||
# any bundler environment should we detect one. Otherwise, subcommands | ||
# will inherit our bundled environment. | ||
# @see https://github.com/test-kitchen/kitchen-vagrant/issues/190 | ||
# @see Kitchen::ShellOut#run_command | ||
def run_command(cmd, options = {}) | ||
merged = { | ||
:use_sudo => config[:use_sudo], :log_subject => name | ||
:use_sudo => config[:use_sudo], | ||
:log_subject => name, | ||
:environment => {} | ||
}.merge(options) | ||
|
||
# Attempt to extract bundler and associated GEM related values. | ||
# TODO: To this accurately, we'd need to create a test-kitchen | ||
# launch wrapper that serializes the existing environment before | ||
# bundler touches it so that we can go back to it. Since that is | ||
# "A Hard Problem"(TM), we simply blow away all known bundler | ||
# related changes. | ||
env = merged[:environment] | ||
%w[BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH GEM_ROOT RUBY_LIB | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope :(. That's the approach I initially took. Turns out with_clean_env doesn't unset GEM_* and also doesn't remove previous additions to PATH. Bundler.with_clean_env was only designed to work within the bundler process itself (or within Gemfiles, etc.). By the time bundler has exec-ed an underlying process, the damage is already done. See my conversation with @sethvargo on the previous diff of this file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a pretty sad, unsolved problem. The easy way to solve it is to use a binstub that grabs the current env and stores it before execing into the Ruby process, but that's not an easy transition to make. |
||
RUBY_OPT _ORIGINAL_GEM_PATH].each do |var| | ||
env[var] = nil | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might add a test to ensure these get stripped |
||
gem_home = ENV["GEM_HOME"] | ||
if gem_home && (env["PATH"] || ENV["PATH"]) | ||
env["PATH"] ||= ENV["PATH"].dup if ENV["PATH"] | ||
gem_bin = File.join(gem_home, "bin") + path_elem_separator | ||
env["PATH"][gem_bin] = "" if env["PATH"].include?(gem_bin) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same for |
||
|
||
super(cmd, merged) | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,17 @@ | |
) | ||
end | ||
|
||
before { stub_const("ENV", env) } | ||
module RunCommandStub | ||
def run_command(_cmd, options = {}) | ||
options | ||
end | ||
end | ||
|
||
before(:all) do | ||
Kitchen::Driver::Vagrant.instance_eval { include RunCommandStub } | ||
end | ||
|
||
before(:each) { stub_const("ENV", env) } | ||
|
||
after do | ||
driver_object.class.send(:winrm_plugin_passed=, nil) | ||
|
@@ -77,6 +87,66 @@ | |
Kitchen::Driver::VAGRANT_VERSION) | ||
end | ||
|
||
describe "#run_command" do | ||
before(:each) do | ||
allow(driver_object).to receive(:path_elem_separator).and_return(":") | ||
end | ||
|
||
context "when invoked from a clean environment" do | ||
|
||
it "passes through environment variables" do | ||
options = driver.send( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's protected. |
||
:run_command, | ||
"cmd", | ||
:environment => { "EV1" => "Val1", "EV2" => "Val2" }) | ||
expect(options[:environment]["EV1"]).to eq("Val1") | ||
expect(options[:environment]["EV2"]).to eq("Val2") | ||
end | ||
|
||
it "leaves path alone" do | ||
options = driver.send( | ||
:run_command, | ||
"cmd", | ||
:environment => { "PATH" => "/foo/:/bar" }) | ||
expect(options[:environment]["PATH"]).to eq("/foo/:/bar") | ||
end | ||
|
||
end | ||
|
||
context "when invoked from a bundler[:environment]" do | ||
|
||
let(:bundler_env) do | ||
{ | ||
"BUNDLE_BIN_PATH" => "bundle_bin_path", | ||
"BUNDLE_GEMFILE" => "bundle_gem_file", | ||
"GEM_HOME" => "gem_home", | ||
"GEM_PATH" => "gem_path", | ||
"GEM_ROOT" => "gem_root", | ||
"RUBY_LIB" => "ruby_lib", | ||
"RUBY_OPT" => "ruby_opt", | ||
"_ORIGINAL_GEM_PATH" => "original_gem_path" | ||
} | ||
end | ||
|
||
it "removes all bundler related variables" do | ||
env.merge!(bundler_env) | ||
options = driver.send(:run_command, "cmd") | ||
bundler_env.each do |k, _v| | ||
expect(options[:environment]).to include(k) | ||
expect(options[:environment][k]).to eq(nil) | ||
end | ||
end | ||
|
||
it "fixes path if it notices gem_home in it" do | ||
env.merge!(bundler_env) | ||
env["PATH"] = "gem_home/bin:/something/else" | ||
options = driver.send(:run_command, "cmd") | ||
puts(options) | ||
expect(options[:environment]["PATH"]).to eq("/something/else") | ||
end | ||
end | ||
end | ||
|
||
describe "configuration" do | ||
|
||
%W[centos debian fedora opensuse ubuntu].each do |name| | ||
|
@@ -252,7 +322,7 @@ | |
] | ||
|
||
expect(driver[:synced_folders]).to eq([ | ||
["/host_path", "/vm_path", "create: true, type: :nfs"] | ||
[File.expand_path("/host_path"), "/vm_path", "create: true, type: :nfs"] | ||
]) | ||
end | ||
|
||
|
@@ -262,7 +332,7 @@ | |
] | ||
|
||
expect(driver[:synced_folders]).to eq([ | ||
["/root/suitey-fooos-99", "/vm_path", "stuff"] | ||
[File.expand_path("/root/suitey-fooos-99"), "/vm_path", "stuff"] | ||
]) | ||
end | ||
|
||
|
@@ -272,7 +342,7 @@ | |
] | ||
|
||
expect(driver[:synced_folders]).to eq([ | ||
["/kroot/a", "/vm_path", "stuff"] | ||
[File.expand_path("/kroot/a"), "/vm_path", "stuff"] | ||
]) | ||
end | ||
|
||
|
@@ -282,7 +352,7 @@ | |
] | ||
|
||
expect(driver[:synced_folders]).to eq([ | ||
["/host_path", "/vm_path", "nil"] | ||
[File.expand_path("/host_path"), "/vm_path", "nil"] | ||
]) | ||
end | ||
|
||
|
@@ -295,13 +365,15 @@ | |
it "sets :vagrantfile_erb to a default value" do | ||
config[:vagrantfile_erb] = "/a/Vagrantfile.erb" | ||
|
||
expect(driver[:vagrantfile_erb]).to eq("/a/Vagrantfile.erb") | ||
expect(driver[:vagrantfile_erb]).to eq( | ||
File.expand_path("/a/Vagrantfile.erb")) | ||
end | ||
|
||
it "expands path for :vagrantfile_erb" do | ||
config[:vagrantfile_erb] = "Yep.erb" | ||
|
||
expect(driver[:vagrantfile_erb]).to eq("/kroot/Yep.erb") | ||
expect(driver[:vagrantfile_erb]).to eq( | ||
File.expand_path("/kroot/Yep.erb")) | ||
end | ||
|
||
it "sets :vagrantfiles to an empty array by default" do | ||
|
@@ -312,7 +384,7 @@ | |
config[:vagrantfiles] = %W[one two three] | ||
|
||
expect(driver[:vagrantfiles]).to eq( | ||
%W[/kroot/one /kroot/two /kroot/three] | ||
%W[/kroot/one /kroot/two /kroot/three].map { |f| File.expand_path(f) } | ||
) | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference, I usually also add
--require spec_helper.rb
so that you don't have to have that require at the top of every spec file, but that may just be my preference. Don't need to change the PR