From 429bd73495173a22d5fb09a008043d37e32e350a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 09:26:36 -0700 Subject: [PATCH 1/9] core: provider has default priority of 5 --- lib/vagrant/plugin/v2/plugin.rb | 6 ++++-- test/unit/vagrant/plugin/v2/plugin_test.rb | 9 +++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/vagrant/plugin/v2/plugin.rb b/lib/vagrant/plugin/v2/plugin.rb index 51a9f458b..e9559da6d 100644 --- a/lib/vagrant/plugin/v2/plugin.rb +++ b/lib/vagrant/plugin/v2/plugin.rb @@ -185,9 +185,11 @@ module Vagrant # Registers additional providers to be available. # # @param [Symbol] name Name of the provider. - def self.provider(name=UNSET_VALUE, options=nil, &block) + def self.provider(name=UNSET_VALUE, **options, &block) + options[:priority] ||= 5 + components.providers.register(name.to_sym) do - [block.call, options || {}] + [block.call, options] end nil diff --git a/test/unit/vagrant/plugin/v2/plugin_test.rb b/test/unit/vagrant/plugin/v2/plugin_test.rb index a8b3159e6..944d38f9f 100644 --- a/test/unit/vagrant/plugin/v2/plugin_test.rb +++ b/test/unit/vagrant/plugin/v2/plugin_test.rb @@ -249,7 +249,9 @@ describe Vagrant::Plugin::V2::Plugin do provider("foo") { "bar" } end - expect(plugin.components.providers[:foo]).to eq(["bar", {}]) + result = plugin.components.providers[:foo] + expect(result[0]).to eq("bar") + expect(result[1][:priority]).to eq(5) end it "should register provider classes with options" do @@ -257,7 +259,10 @@ describe Vagrant::Plugin::V2::Plugin do provider("foo", foo: "yep") { "bar" } end - expect(plugin.components.providers[:foo]).to eq(["bar", { foo: "yep" }]) + result = plugin.components.providers[:foo] + expect(result[0]).to eq("bar") + expect(result[1][:priority]).to eq(5) + expect(result[1][:foo]).to eq("yep") end it "should lazily register provider classes" do From a9dfb6b3bdd7e25b38c7ba9dfdd48d8eac910599 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 09:40:52 -0700 Subject: [PATCH 2/9] core: default provider chosen by usability and prority --- .../action/builtin/mixin_synced_folders.rb | 2 +- lib/vagrant/environment.rb | 31 +++++++++++++++++-- .../shared/capability_helpers_context.rb | 8 +++++ test/unit/vagrant/environment_test.rb | 27 +++++++++++++--- 4 files changed, 60 insertions(+), 8 deletions(-) diff --git a/lib/vagrant/action/builtin/mixin_synced_folders.rb b/lib/vagrant/action/builtin/mixin_synced_folders.rb index ab8058fd0..42e3a290f 100644 --- a/lib/vagrant/action/builtin/mixin_synced_folders.rb +++ b/lib/vagrant/action/builtin/mixin_synced_folders.rb @@ -22,7 +22,7 @@ module Vagrant ordered << [priority, key, impl] end - # Order the plugins by priority. Higher is tries before lower. + # Order the plugins by priority. Higher is tried before lower. ordered = ordered.sort { |a, b| b[0] <=> a[0] } # Find the proper implementation diff --git a/lib/vagrant/environment.rb b/lib/vagrant/environment.rb index cf37ffeb4..19975b4ec 100644 --- a/lib/vagrant/environment.rb +++ b/lib/vagrant/environment.rb @@ -254,8 +254,35 @@ module Vagrant # environment. # # @return [Symbol] Name of the default provider. - def default_provider - (ENV['VAGRANT_DEFAULT_PROVIDER'] || :virtualbox).to_sym + def default_provider(**opts) + default = ENV["VAGRANT_DEFAULT_PROVIDER"] + default = nil if default == "" + default = default.to_sym if default + + ordered = [] + Vagrant.plugin("2").manager.providers.each do |key, data| + impl = data[0] + opts = data[1] + + ordered << [opts[:priority], key, impl, opts] + end + + # Order the providers by priority. Higher values are tried first. + ordered = ordered.sort do |a, b| + # If we see the default, then that one always wins + next -1 if a[1] == default + next 1 if b[1] == default + + b[0] <=> a[0] + end + + # Find the matching implementation + ordered.each do |_, key, impl, _| + return key if impl.usable?(false) + end + + # If all else fails, return VirtualBox + return :virtualbox end # Returns the collection of boxes for the environment. diff --git a/test/unit/support/shared/capability_helpers_context.rb b/test/unit/support/shared/capability_helpers_context.rb index 7db62d161..4976aaf8d 100644 --- a/test/unit/support/shared/capability_helpers_context.rb +++ b/test/unit/support/shared/capability_helpers_context.rb @@ -7,6 +7,14 @@ shared_context "capability_helpers" do end end + def provider_usable_class(result) + Class.new do + define_singleton_method(:usable?) do |*args| + result + end + end + end + def cap_instance(name, options=nil) options ||= {} diff --git a/test/unit/vagrant/environment_test.rb b/test/unit/vagrant/environment_test.rb index 070df3ecf..ca16d4187 100644 --- a/test/unit/vagrant/environment_test.rb +++ b/test/unit/vagrant/environment_test.rb @@ -699,15 +699,32 @@ VF end describe "default provider" do - it "is virtualbox without any environmental variable" do + let(:plugin_providers) { {} } + + before do + m = Vagrant.plugin("2").manager + m.stub(providers: plugin_providers) + end + + it "is the highest matching usable provider" do + plugin_providers[:foo] = [provider_usable_class(true), { priority: 5 }] + plugin_providers[:bar] = [provider_usable_class(true), { priority: 7 }] + plugin_providers[:baz] = [provider_usable_class(true), { priority: 2 }] + plugin_providers[:boom] = [provider_usable_class(true), { priority: 3 }] + with_temp_env("VAGRANT_DEFAULT_PROVIDER" => nil) do - expect(subject.default_provider).to eq(:virtualbox) + expect(subject.default_provider).to eq(:bar) end end - it "is whatever the environmental variable is if set" do - with_temp_env("VAGRANT_DEFAULT_PROVIDER" => "foo") do - expect(subject.default_provider).to eq(:foo) + it "is the default provider if it is usable" do + plugin_providers[:foo] = [provider_usable_class(true), { priority: 5 }] + plugin_providers[:bar] = [provider_usable_class(true), { priority: 7 }] + plugin_providers[:baz] = [provider_usable_class(true), { priority: 2 }] + plugin_providers[:boom] = [provider_usable_class(true), { priority: 3 }] + + with_temp_env("VAGRANT_DEFAULT_PROVIDER" => "baz") do + expect(subject.default_provider).to eq(:baz) end end end From ba6272cc48188f3b93c0b31f006cdc14f16b9205 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 09:41:59 -0700 Subject: [PATCH 3/9] Update tests --- test/unit/vagrant/environment_test.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/unit/vagrant/environment_test.rb b/test/unit/vagrant/environment_test.rb index ca16d4187..20255e5b7 100644 --- a/test/unit/vagrant/environment_test.rb +++ b/test/unit/vagrant/environment_test.rb @@ -727,6 +727,16 @@ VF expect(subject.default_provider).to eq(:baz) end end + + it "is VirtualBox if nothing else is usable" do + plugin_providers[:foo] = [provider_usable_class(false), { priority: 5 }] + plugin_providers[:bar] = [provider_usable_class(false), { priority: 5 }] + plugin_providers[:baz] = [provider_usable_class(false), { priority: 5 }] + + with_temp_env("VAGRANT_DEFAULT_PROVIDER" => nil) do + expect(subject.default_provider).to eq(:virtualbox) + end + end end describe "local data path" do From ab9f91568ee3538019e8a4d7d862b97d4b9acb03 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 09:46:40 -0700 Subject: [PATCH 4/9] core: more intuitive logic around default providers --- lib/vagrant/environment.rb | 8 ++++++-- test/unit/vagrant/environment_test.rb | 22 +++++++++++++++++++++- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/lib/vagrant/environment.rb b/lib/vagrant/environment.rb index 19975b4ec..ea5d77b15 100644 --- a/lib/vagrant/environment.rb +++ b/lib/vagrant/environment.rb @@ -249,16 +249,20 @@ module Vagrant end # This returns the provider name for the default provider for this - # environment. The provider returned is currently hardcoded to "virtualbox" - # but one day should be a detected valid, best-case provider for this # environment. # # @return [Symbol] Name of the default provider. def default_provider(**opts) + opts[:force_default] = true if !opts.has_key?(:force_default) + default = ENV["VAGRANT_DEFAULT_PROVIDER"] default = nil if default == "" default = default.to_sym if default + # If we're forcing the default, just short-circuit and return + # that (the default behavior) + return default if default && opts[:force_default] + ordered = [] Vagrant.plugin("2").manager.providers.each do |key, data| impl = data[0] diff --git a/test/unit/vagrant/environment_test.rb b/test/unit/vagrant/environment_test.rb index 20255e5b7..ae3199bdf 100644 --- a/test/unit/vagrant/environment_test.rb +++ b/test/unit/vagrant/environment_test.rb @@ -717,7 +717,7 @@ VF end end - it "is the default provider if it is usable" do + it "is the default provider set if usable" do plugin_providers[:foo] = [provider_usable_class(true), { priority: 5 }] plugin_providers[:bar] = [provider_usable_class(true), { priority: 7 }] plugin_providers[:baz] = [provider_usable_class(true), { priority: 2 }] @@ -728,6 +728,26 @@ VF end end + it "is the default provider set even if unusable" do + plugin_providers[:baz] = [provider_usable_class(false), { priority: 5 }] + plugin_providers[:foo] = [provider_usable_class(true), { priority: 5 }] + plugin_providers[:bar] = [provider_usable_class(true), { priority: 7 }] + + with_temp_env("VAGRANT_DEFAULT_PROVIDER" => "baz") do + expect(subject.default_provider).to eq(:baz) + end + end + + it "is the usable despite default if not forced" do + plugin_providers[:baz] = [provider_usable_class(false), { priority: 5 }] + plugin_providers[:foo] = [provider_usable_class(true), { priority: 5 }] + plugin_providers[:bar] = [provider_usable_class(true), { priority: 7 }] + + with_temp_env("VAGRANT_DEFAULT_PROVIDER" => "baz") do + expect(subject.default_provider(force_default: false)).to eq(:bar) + end + end + it "is VirtualBox if nothing else is usable" do plugin_providers[:foo] = [provider_usable_class(false), { priority: 5 }] plugin_providers[:bar] = [provider_usable_class(false), { priority: 5 }] From cca9bffa90822227ddc0f8cda65dafa6349bb85a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 09:50:35 -0700 Subject: [PATCH 5/9] core: Can exclude providers --- lib/vagrant/environment.rb | 10 +++++++--- test/unit/vagrant/environment_test.rb | 11 +++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/vagrant/environment.rb b/lib/vagrant/environment.rb index ea5d77b15..34deaf058 100644 --- a/lib/vagrant/environment.rb +++ b/lib/vagrant/environment.rb @@ -253,6 +253,7 @@ module Vagrant # # @return [Symbol] Name of the default provider. def default_provider(**opts) + opts[:exclude] = Set.new(opts[:exclude]) if opts[:exclude] opts[:force_default] = true if !opts.has_key?(:force_default) default = ENV["VAGRANT_DEFAULT_PROVIDER"] @@ -265,10 +266,13 @@ module Vagrant ordered = [] Vagrant.plugin("2").manager.providers.each do |key, data| - impl = data[0] - opts = data[1] + impl = data[0] + popts = data[1] - ordered << [opts[:priority], key, impl, opts] + # Skip excluded providers + next if opts[:exclude] && opts[:exclude].include?(key) + + ordered << [popts[:priority], key, impl, popts] end # Order the providers by priority. Higher values are tried first. diff --git a/test/unit/vagrant/environment_test.rb b/test/unit/vagrant/environment_test.rb index ae3199bdf..7fc0bcc3f 100644 --- a/test/unit/vagrant/environment_test.rb +++ b/test/unit/vagrant/environment_test.rb @@ -717,6 +717,17 @@ VF end end + it "is the highest matching usable provider that isn't excluded" do + plugin_providers[:foo] = [provider_usable_class(true), { priority: 5 }] + plugin_providers[:bar] = [provider_usable_class(true), { priority: 7 }] + plugin_providers[:baz] = [provider_usable_class(true), { priority: 2 }] + plugin_providers[:boom] = [provider_usable_class(true), { priority: 3 }] + + with_temp_env("VAGRANT_DEFAULT_PROVIDER" => nil) do + expect(subject.default_provider(exclude: [:bar, :foo])).to eq(:boom) + end + end + it "is the default provider set if usable" do plugin_providers[:foo] = [provider_usable_class(true), { priority: 5 }] plugin_providers[:bar] = [provider_usable_class(true), { priority: 7 }] From 76e29b912fffb82832289201190b4a8f1e77fcae Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 09:51:41 -0700 Subject: [PATCH 6/9] providers/docker: lower priority We do this mostly because Docker is the only provider that using it requires some amount of Docker-specific knowledge. VirtualBox, VMware, etc. kind of "just work". It is not the likely case that someone does a `vagrant up` and expects Docker, at this point. --- plugins/providers/docker/plugin.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/providers/docker/plugin.rb b/plugins/providers/docker/plugin.rb index b6988661b..5b024519a 100644 --- a/plugins/providers/docker/plugin.rb +++ b/plugins/providers/docker/plugin.rb @@ -16,7 +16,7 @@ module VagrantPlugins Docker containers. EOF - provider(:docker, box_optional: true, parallel: true) do + provider(:docker, box_optional: true, parallel: true, priority: 3) do require_relative 'provider' init! Provider From 52bb68ba731e167e98917e23e4a2ce32e2bd9500 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 10:03:49 -0700 Subject: [PATCH 7/9] core: Fix some tests, revert a change --- lib/vagrant/plugin/v2/plugin.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/vagrant/plugin/v2/plugin.rb b/lib/vagrant/plugin/v2/plugin.rb index e9559da6d..9a7f6177d 100644 --- a/lib/vagrant/plugin/v2/plugin.rb +++ b/lib/vagrant/plugin/v2/plugin.rb @@ -185,7 +185,8 @@ module Vagrant # Registers additional providers to be available. # # @param [Symbol] name Name of the provider. - def self.provider(name=UNSET_VALUE, **options, &block) + def self.provider(name=UNSET_VALUE, options=nil, &block) + options ||= {} options[:priority] ||= 5 components.providers.register(name.to_sym) do From 169a829cd8940a9dde1c5883c05e742ab69da2b9 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 10:06:03 -0700 Subject: [PATCH 8/9] core: dup the provider options so we can't overwrite them --- lib/vagrant/vagrantfile.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vagrant/vagrantfile.rb b/lib/vagrant/vagrantfile.rb index da85d2360..b6b2c15ec 100644 --- a/lib/vagrant/vagrantfile.rb +++ b/lib/vagrant/vagrantfile.rb @@ -192,7 +192,7 @@ module Vagrant return { box: box, provider_cls: provider_cls, - provider_options: provider_options, + provider_options: provider_options.dup, config: config, config_warnings: config_warnings, config_errors: config_errors, From c5b3dbbf752ac4e7d781da50c4d6c514e25dbc96 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 1 May 2014 10:12:36 -0700 Subject: [PATCH 9/9] core: fix final test --- test/unit/vagrant/plugin/v2/manager_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/vagrant/plugin/v2/manager_test.rb b/test/unit/vagrant/plugin/v2/manager_test.rb index 9d9903e6e..45ebd5a0c 100644 --- a/test/unit/vagrant/plugin/v2/manager_test.rb +++ b/test/unit/vagrant/plugin/v2/manager_test.rb @@ -167,8 +167,8 @@ describe Vagrant::Plugin::V2::Manager do instance.register(pB) expect(instance.providers.to_hash.length).to eq(2) - expect(instance.providers[:foo]).to eq(["bar", {}]) - expect(instance.providers[:bar]).to eq(["baz", { foo: "bar" }]) + expect(instance.providers[:foo]).to eq(["bar", { priority: 5 }]) + expect(instance.providers[:bar]).to eq(["baz", { foo: "bar", priority: 5 }]) end it "provides the collection of registered provider configs" do