From 97b5882262fcc5c652123c88515b8f925c42f574 Mon Sep 17 00:00:00 2001 From: Virgil Dupras Date: Wed, 13 Dec 2017 16:35:16 -0500 Subject: [PATCH] Refactoring: make SudoWrapper a bit more self-contained By looking at the code, it seems that it was a goal to make the sudo wrapper path configurable through the Vagrantfile, but it wasn't effective and didn't make much sense (that kind of config is a per-host config, not a per-guest one). This caused the cause to be needlessly complex by giving the Provider the responsibility of instanciating the wrapper. This commit gets rid of that. I didn't get rid of `sudo_wrapper` injection in `Driver` and `Driver::CLI` constructors because they're needed for tests. I'm not ready to tackle this yet. --- lib/vagrant-lxc.rb | 12 ------------ lib/vagrant-lxc/command/sudoers.rb | 5 +++-- lib/vagrant-lxc/config.rb | 2 -- lib/vagrant-lxc/driver.rb | 7 ++++--- lib/vagrant-lxc/provider.rb | 9 +++------ lib/vagrant-lxc/sudo_wrapper.rb | 8 ++++++-- spec/Vagrantfile | 4 ---- 7 files changed, 16 insertions(+), 31 deletions(-) diff --git a/lib/vagrant-lxc.rb b/lib/vagrant-lxc.rb index a1e3bf1..d1ad0f7 100644 --- a/lib/vagrant-lxc.rb +++ b/lib/vagrant-lxc.rb @@ -1,22 +1,10 @@ require "vagrant-lxc/version" require "vagrant-lxc/plugin" -require "vagrant-lxc/sudo_wrapper" module Vagrant module LXC def self.source_root @source_root ||= Pathname.new(File.dirname(__FILE__)).join('..').expand_path end - - def self.sudo_wrapper_path - "/usr/local/bin/vagrant-lxc-wrapper" - end - - def self.sudo_wrapper - wrapper = Pathname.new(sudo_wrapper_path).exist? && - sudo_wrapper_path || nil - SudoWrapper.new(wrapper) - end - end end diff --git a/lib/vagrant-lxc/command/sudoers.rb b/lib/vagrant-lxc/command/sudoers.rb index 765d15e..1a5ff91 100644 --- a/lib/vagrant-lxc/command/sudoers.rb +++ b/lib/vagrant-lxc/command/sudoers.rb @@ -1,6 +1,7 @@ require 'tempfile' require "vagrant-lxc/driver" +require "vagrant-lxc/sudo_wrapper" module Vagrant module LXC @@ -27,7 +28,7 @@ module Vagrant argv = parse_options(opts) return unless argv - wrapper_path = Vagrant::LXC.sudo_wrapper_path + wrapper_path = SudoWrapper.bin_path wrapper = create_wrapper! sudoers = create_sudoers!(options[:user], wrapper_path) @@ -45,7 +46,7 @@ module Vagrant # This requires vagrant 1.5.2+ https://github.com/mitchellh/vagrant/commit/3371c3716278071680af9b526ba19235c79c64cb def create_wrapper! - lxc_base_path = Driver.new(0, LXC.sudo_wrapper).containers_path + lxc_base_path = Driver.new("").containers_path wrapper = Tempfile.new('lxc-wrapper').tap do |file| template = Vagrant::Util::TemplateRenderer.new( 'sudoers.rb', diff --git a/lib/vagrant-lxc/config.rb b/lib/vagrant-lxc/config.rb index 6593dca..7a387a3 100644 --- a/lib/vagrant-lxc/config.rb +++ b/lib/vagrant-lxc/config.rb @@ -24,7 +24,6 @@ module Vagrant @customizations = [] @backingstore = UNSET_VALUE @backingstore_options = [] - @sudo_wrapper = UNSET_VALUE @container_name = UNSET_VALUE @fetch_ip_tries = UNSET_VALUE end @@ -50,7 +49,6 @@ module Vagrant end def finalize! - @sudo_wrapper = nil if @sudo_wrapper == UNSET_VALUE @container_name = nil if @container_name == UNSET_VALUE @backingstore = "best" if @backingstore == UNSET_VALUE @existing_container_name = nil if @existing_container_name == UNSET_VALUE diff --git a/lib/vagrant-lxc/driver.rb b/lib/vagrant-lxc/driver.rb index e2304bf..b374493 100644 --- a/lib/vagrant-lxc/driver.rb +++ b/lib/vagrant-lxc/driver.rb @@ -3,6 +3,7 @@ require "vagrant/util/subprocess" require "vagrant-lxc/errors" require "vagrant-lxc/driver/cli" +require "vagrant-lxc/sudo_wrapper" require "etc" @@ -21,10 +22,10 @@ module Vagrant attr_reader :container_name, :customizations - def initialize(container_name, sudo_wrapper, cli = nil) + def initialize(container_name, sudo_wrapper = nil, cli = nil) @container_name = container_name - @sudo_wrapper = sudo_wrapper - @cli = cli || CLI.new(sudo_wrapper, container_name) + @sudo_wrapper = sudo_wrapper || SudoWrapper.new() + @cli = cli || CLI.new(@sudo_wrapper, container_name) @logger = Log4r::Logger.new("vagrant::provider::lxc::driver") @customizations = [] end diff --git a/lib/vagrant-lxc/provider.rb b/lib/vagrant-lxc/provider.rb index 806733d..52c82ff 100644 --- a/lib/vagrant-lxc/provider.rb +++ b/lib/vagrant-lxc/provider.rb @@ -2,6 +2,7 @@ require "log4r" require "vagrant-lxc/action" require "vagrant-lxc/driver" +require "vagrant-lxc/sudo_wrapper" module Vagrant module LXC @@ -24,13 +25,9 @@ module Vagrant machine_id_changed end - def sudo_wrapper - @shell ||= LXC.sudo_wrapper - end - def ensure_lxc_installed! begin - sudo_wrapper.run("which", "lxc-create") + SudoWrapper.new().run("which", "lxc-create") rescue Vagrant::LXC::Errors::ExecuteError raise Errors::LxcNotInstalled end @@ -43,7 +40,7 @@ module Vagrant begin @logger.debug("Instantiating the container for: #{id.inspect}") - @driver = Driver.new(id, self.sudo_wrapper) + @driver = Driver.new(id) @driver.validate! rescue Driver::ContainerNotFound # The container doesn't exist, so we probably have a stale diff --git a/lib/vagrant-lxc/sudo_wrapper.rb b/lib/vagrant-lxc/sudo_wrapper.rb index 600f060..67b9409 100644 --- a/lib/vagrant-lxc/sudo_wrapper.rb +++ b/lib/vagrant-lxc/sudo_wrapper.rb @@ -6,8 +6,12 @@ module Vagrant attr_reader :wrapper_path - def initialize(wrapper_path = nil) - @wrapper_path = wrapper_path + def self.dest_path + "/usr/local/bin/vagrant-lxc-wrapper" + end + + def initialize() + @wrapper_path = Pathname.new(SudoWrapper.dest_path).exist? && SudoWrapper.dest_path || nil @logger = Log4r::Logger.new("vagrant::lxc::sudo_wrapper") end diff --git a/spec/Vagrantfile b/spec/Vagrantfile index 9b35550..6a42ac4 100644 --- a/spec/Vagrantfile +++ b/spec/Vagrantfile @@ -15,10 +15,6 @@ Vagrant.configure("2") do |config| config.cache.auto_detect = true - config.vm.provider :lxc do |lxc| - # lxc.sudo_wrapper = '/usr/bin/lxc-vagrant-wrapper' - end - config.vm.provision :shell, inline: 'mkdir -p /vagrant/tmp && echo -n "Provisioned" > /vagrant/tmp/provisioning'