From 2d57afbbda68b6a0755073a0f3544336efa07607 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 20 Jan 2013 22:04:50 -0500 Subject: [PATCH] Support warnings/errors when upgrading Vagrantfiles internally --- lib/vagrant/config/loader.rb | 36 +++++++++++++++++----- lib/vagrant/config/v2/loader.rb | 14 +++++++-- lib/vagrant/environment.rb | 25 +++++++++++++-- lib/vagrant/errors.rb | 4 +++ plugins/kernel_v1/config/vm.rb | 9 +++++- templates/config/messages.erb | 14 +++++++++ templates/locales/en.yml | 12 ++++++++ test/unit/vagrant/config/loader_test.rb | 33 +++++++++++++++++--- test/unit/vagrant/config/v2/loader_test.rb | 4 +++ 9 files changed, 133 insertions(+), 18 deletions(-) create mode 100644 templates/config/messages.erb diff --git a/lib/vagrant/config/loader.rb b/lib/vagrant/config/loader.rb index 097108fa9..6dfa6cdc5 100644 --- a/lib/vagrant/config/loader.rb +++ b/lib/vagrant/config/loader.rb @@ -87,6 +87,11 @@ module Vagrant # This will hold our result result = current_config_klass.init + # Keep track of the warnings and errors that may come from + # upgrading the Vagrantfiles + warnings = [] + errors = [] + order.each do |key| next if !@sources.has_key?(key) @@ -98,6 +103,11 @@ module Vagrant version_loader = @versions.get(version) version_config = version_loader.load(proc) + # Store the errors/warnings associated with loading this + # configuration. We'll store these for later. + version_warnings = [] + version_errors = [] + # If this version is not the current version, then we need # to upgrade to the latest version. if version != current_version @@ -114,30 +124,40 @@ module Vagrant upgrade_result = loader.upgrade(version_config) # XXX: Do something with the warning/error messages - warnings = upgrade_result[1] - errors = upgrade_result[2] + this_warnings = upgrade_result[1] + this_errors = upgrade_result[2] @logger.debug("Upgraded to version #{next_version} with " + - "#{warnings.length} warnings and " + - "#{errors.length} errors") + "#{this_warnings.length} warnings and " + + "#{this_errors.length} errors") + + # Append loading this to the version warnings and errors + version_warnings += this_warnings + version_errors += this_errors # Store the new upgraded version version_config = upgrade_result[0] end end - # Cache the results for this proc - @config_cache[proc] = version_config + # Cache the loaded configuration along with any warnings + # or errors so that they can be retrieved later. + @config_cache[proc] = [version_config, version_warnings, version_errors] else @logger.debug("Loading from: #{key} (cache)") end # Merge the configurations - result = current_config_klass.merge(result, @config_cache[proc]) + cache_data = @config_cache[proc] + result = current_config_klass.merge(result, cache_data[0]) + + # Append the total warnings/errors + warnings += cache_data[1] + errors += cache_data[2] end end @logger.debug("Configuration loaded successfully, finalizing and returning") - current_config_klass.finalize(result) + [current_config_klass.finalize(result), warnings, errors] end protected diff --git a/lib/vagrant/config/v2/loader.rb b/lib/vagrant/config/v2/loader.rb index 143e76448..11f66caf7 100644 --- a/lib/vagrant/config/v2/loader.rb +++ b/lib/vagrant/config/v2/loader.rb @@ -92,14 +92,24 @@ module Vagrant # Get a new root root = new_root_object + # Store the warnings/errors + warnings = [] + errors = [] + # Go through the old keys and upgrade them if they can be old.__internal_state["keys"].each do |_, old_value| if old_value.respond_to?(:upgrade) - old_value.upgrade(root) + result = old_value.upgrade(root) + + # Sanity check to guard against random return values + if result.is_a?(Array) + warnings += result[0] + errors += result[1] + end end end - [root, [], []] + [root, warnings, errors] end protected diff --git a/lib/vagrant/environment.rb b/lib/vagrant/environment.rb index edc7c35b7..0362a51d2 100644 --- a/lib/vagrant/environment.rb +++ b/lib/vagrant/environment.rb @@ -247,7 +247,8 @@ module Vagrant # with the box Vagrantfile (if it has one). vm_config_key = "vm_#{name}".to_sym @config_loader.set(vm_config_key, sub_vm.config_procs) - config = @config_loader.load([:default, :home, :root, vm_config_key]) + config, config_warnings, config_errors = \ + @config_loader.load([:default, :home, :root, vm_config_key]) box = nil begin @@ -270,7 +271,8 @@ module Vagrant @logger.info("Box exists with Vagrantfile. Reloading machine config.") box_config_key = "box_#{box.name}_#{box.provider}".to_sym @config_loader.set(box_config_key, box_vagrantfile) - config = @config_loader.load([:default, box_config_key, :home, :root, vm_config_key]) + config, config_warnings, config_errors = \ + @config_loader.load([:default, box_config_key, :home, :root, vm_config_key]) end end @@ -282,6 +284,22 @@ module Vagrant machine_data_path = @local_data_path.join("machines/#{name}/#{provider}") FileUtils.mkdir_p(machine_data_path) + # If there were warnings or errors we want to output them + if !config_warnings.empty? || !config_errors.empty? + # The color of the output depends on whether we have warnings + # or errors... + level = config_errors.empty? ? :warn : :error + output = Util::TemplateRenderer.render( + "config/messages", + :warnings => config_warnings, + :errors => config_errors).chomp + @ui.send(level, I18n.t("vagrant.general.config_upgrade_messages", + :output => output)) + + # If we had errors, then we bail + raise Errors::ConfigUpgradeErrors if !config_errors.empty? + end + # Create the machine and cache it for future calls. This will also # return the machine from this method. @machines[cache_key] = Machine.new(name, provider_cls, provider_config, @@ -448,7 +466,8 @@ module Vagrant # Make the initial call to get the "global" config. This is mostly # only useful to get the list of machines that we are managing. - @config_global = @config_loader.load([:default, :home, :root]) + # Because of this, we ignore any warnings or errors. + @config_global, _ = @config_loader.load([:default, :home, :root]) # Old order: default, box, home, root, vm end diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 4fdc5ce2c..f081000bd 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -154,6 +154,10 @@ module Vagrant error_key(:config_invalid) end + class ConfigUpgradeErrors < VagrantError + error_key(:config_upgrade_errors) + end + class DestroyRequiresForce < VagrantError status_code(74) error_key(:destroy_requires_force) diff --git a/plugins/kernel_v1/config/vm.rb b/plugins/kernel_v1/config/vm.rb index 03f928c8b..0618b56d0 100644 --- a/plugins/kernel_v1/config/vm.rb +++ b/plugins/kernel_v1/config/vm.rb @@ -131,7 +131,14 @@ module VagrantPlugins new.vm.define(name, options, &block) end - # XXX: Warning: `vm.name` is useless now + # If name is used, warn that it has no effect anymore + warnings = [] + if @name + warnings << "`config.vm.name` has no effect anymore. Names are derived\n" + + "directly from any `config.vm.define` calls." + end + + [warnings, []] end end end diff --git a/templates/config/messages.erb b/templates/config/messages.erb new file mode 100644 index 000000000..6a2cedf33 --- /dev/null +++ b/templates/config/messages.erb @@ -0,0 +1,14 @@ +<% if !warnings.empty? -%> +Warnings: +<% warnings.each do |warning| -%> +* <%= warning %> +<% end -%> + +<% end -%> +<% if !errors.empty? -%> +Errors: +<% errors.each do |error| -%> +* <%= error %> +<% end -%> + +<% end -%> diff --git a/templates/locales/en.yml b/templates/locales/en.yml index f76cebe55..3c00b3a9a 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1,6 +1,15 @@ en: vagrant: general: + config_upgrade_messages: |- + There were warnings and/or errors while loading your Vagrantfile. + Your Vagrantfile was written for an earlier version of Vagrant, + and while Vagrant does the best it can to remain backwards + compatible, there are some cases where things have changed + significantly enough to warrant a message. These messages are + shown below. + + %{output} moving_home_dir: "Moving old Vagrant home directory to new location: %{directory}" home_dir_migration_failed: |- Both an old and new Vagrant home directory exist. Only the new one will @@ -61,6 +70,9 @@ en: the following errors and try again: %{errors} + config_upgrade_errors: |- + Because there were errors upgrading your Vagrantfiles, Vagrant + can no longer continue. Please fix the errors above and try again. destroy_requires_force: |- Destroy doesn't have a TTY to ask for confirmation. Please pass the `--force` flag to force a destroy, otherwise attach a TTY so that diff --git a/test/unit/vagrant/config/loader_test.rb b/test/unit/vagrant/config/loader_test.rb index 83272b961..9109eb863 100644 --- a/test/unit/vagrant/config/loader_test.rb +++ b/test/unit/vagrant/config/loader_test.rb @@ -49,9 +49,11 @@ describe Vagrant::Config::Loader do end instance.set(:proc, [[current_version, proc]]) - config = instance.load([:proc]) + config, warnings, errors = instance.load([:proc]) config[:foo].should == "yep" + warnings.should == [] + errors.should == [] end end @@ -70,7 +72,7 @@ describe Vagrant::Config::Loader do # Run the actual configuration and assert that we get the proper result instance.set(:proc, [[current_version, proc]]) - config = instance.load([:proc]) + config, _ = instance.load([:proc]) config[:foo].should == "yep" config[:finalized].should == true end @@ -93,10 +95,33 @@ describe Vagrant::Config::Loader do # Load a version 1 proc, and verify it is upgraded to version 2 proc = lambda { |config| config[:foo] = "yep" } instance.set(:proc, [["1", proc]]) - config = instance.load([:proc]) + config, _ = instance.load([:proc]) config[:foo].should == "yep" config[:v2].should == true end + + it "should keep track of warnings and errors" do + test_loader_v2 = Class.new(test_loader) do + def self.upgrade(old) + new = old.dup + new[:v2] = true + + [new, ["foo!"], ["bar!"]] + end + end + + versions.register("2") { test_loader_v2 } + version_order << "2" + + # Load a version 1 proc, and verify it is upgraded to version 2 + proc = lambda { |config| config[:foo] = "yep" } + instance.set(:proc, [["1", proc]]) + config, warnings, errors = instance.load([:proc]) + config[:foo].should == "yep" + config[:v2].should == true + warnings.should == ["foo!"] + errors.should == ["bar!"] + end end describe "loading edge cases" do @@ -110,7 +135,7 @@ describe Vagrant::Config::Loader do instance.set(:proc, [[current_version, proc]]) 5.times do - result = instance.load([:proc]) + result, _ = instance.load([:proc]) # Verify the config result result[:foo].should == "yep" diff --git a/test/unit/vagrant/config/v2/loader_test.rb b/test/unit/vagrant/config/v2/loader_test.rb index a0bdabbe7..d88c6724e 100644 --- a/test/unit/vagrant/config/v2/loader_test.rb +++ b/test/unit/vagrant/config/v2/loader_test.rb @@ -128,6 +128,8 @@ describe Vagrant::Config::V2::Loader do def upgrade(new) new.foo.value = value * 2 + + [["foo"], ["bar"]] end end @@ -142,6 +144,8 @@ describe Vagrant::Config::V2::Loader do data = described_class.upgrade(old) data[0].foo.value.should == 10 + data[1].should == ["foo"] + data[2].should == ["bar"] end end end