Support warnings/errors when upgrading Vagrantfiles internally

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]
+                  this_warnings = upgrade_result[1]
-                  errors   = upgrade_result[2]
+                  this_errors   = upgrade_result[2]
                   @logger.debug("Upgraded to version #{next_version} with " +
-                                "#{warnings.length} warnings and " +
+                                "#{this_warnings.length} warnings and " +
-                                "#{errors.length} errors")
+                                "#{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
+              # Cache the loaded configuration along with any warnings
-              @config_cache[proc] = version_config
+              # 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

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

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

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)

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

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 -%>

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

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"

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