General package action raises exceptions instead of using env.error

lib/vagrant/action/general/package.rb

@@ -27,9 +27,10 @@ module Vagrant
         def call(env)
           @env = env
 
-          return env.error!(:package_output_exists) if File.exist?(tar_path)
-          return env.error!(:package_requires_directory) if !@env["package.directory"] || !File.directory?(@env["package.directory"])
-          return if !verify_included_files
+          raise Errors::PackageOutputExists.new if File.exist?(tar_path)
+          raise Errors::PackageRequiresDirectory.new if !@env["package.directory"] || !File.directory?(@env["package.directory"])
+
+          verify_included_files
           compress
 
           @app.call(env)
@@ -42,13 +43,8 @@ module Vagrant
 
         def verify_included_files
           @env["package.include"].each do |file|
-            if !File.exist?(file)
-              @env.error!(:package_include_file_doesnt_exist, :filename => file)
-              return false
-            end
+            raise Errors::PackageIncludeMissing.new(:file => file) if !File.exist?(file)
           end
-
-          true
         end
 
         # This method copies the include files (passed in via command line)

lib/vagrant/errors.rb

@@ -10,8 +10,14 @@ module Vagrant
     # I18n.
     class VagrantError < StandardError
       DEFAULT_NAMESPACE = "vagrant.errors"
+      @@used_codes = []
 
       def self.status_code(code = nil)
+        if code
+          raise "Status code already in use: #{code}"  if @@used_codes.include?(code)
+          @@used_codes << code
+        end
+
         define_method(:status_code) { code }
       end
 
@@ -34,7 +40,7 @@ module Vagrant
     end
 
     class BaseVMNotFound < VagrantError
-      status_code(6)
+      status_code(18)
       error_key(:base_vm_not_found)
     end
 
@@ -78,6 +84,21 @@ module Vagrant
       error_key(:no_env)
     end
 
+    class PackageIncludeMissing < VagrantError
+      status_code(20)
+      error_key(:include_file_missing, "vagrant.actions.general.package")
+    end
+
+    class PackageOutputExists < VagrantError
+      status_code(16)
+      error_key(:output_exists, "vagrant.actions.general.package")
+    end
+
+    class PackageRequiresDirectory < VagrantError
+      status_code(19)
+      error_key(:requires_directory, "vagrant.actions.general.package")
+    end
+
     class SSHAuthenticationFailed < VagrantError
       status_code(11)
       error_key(:ssh_authentication_failed)
@@ -99,7 +120,7 @@ module Vagrant
     end
 
     class VirtualBoxInvalidVersion < VagrantError
-      status_code(9)
+      status_code(17)
       error_key(:virtualbox_invalid_version)
     end
 

templates/locales/en.yml

@@ -197,6 +197,14 @@ en:
         package:
           packaging: "Packaging additional file: %{file}"
           compressing: "Compressing package to: %w{tar_path}"
+          output_exists: |-
+            The specified file to save the package as already exists. Please
+            remove this file or specify a different filename for outputting.
+          requires_directory: |-
+            A directory was not specified to package. This should never happen
+            and is a result of an internal inconsistency.
+          include_file_missing: |-
+            Package include file doesn't exist: %{file}
 
     hosts:
       bsd:

templates/strings.yml

@@ -98,15 +98,6 @@
   This will cause your specified IP to be inaccessible. Please change
   the IP or name of your host only network to not match that of
   a bridged or non-hostonly network.
-:package_include_file_doesnt_exist: |-
-  File specified to include: '<%= filename %>' does not exist!
-:package_output_exists: |-
-  The specified file to save the package as already exists. Please
-  remove this file or specify a different filename for outputting.
-:package_requires_directory: |-
-  A directory was not specified to package. This is an internal
-  issue. Please send the relevant stack trace (if any) and information
-  about this issue to the Vagrant team.
 :package_requires_export: |-
   Package must be used in conjunction with export.
 :provisioner_invalid_class: |-

test/vagrant/action/general/package_test.rb

@@ -62,39 +62,28 @@ class PackageGeneralActionTest < Test::Unit::TestCase
         @instance.call(@env)
       end
 
-      should "halt the chain if verify failed" do
-        @instance.expects(:verify_included_files).returns(false)
-        @instance.expects(:compress).never
-        @app.expects(:call).never
-
-        @instance.call(@env)
-      end
-
       should "halt the chain if the output file already exists" do
         File.expects(:exist?).returns(true)
         @app.expects(:call).never
-        @instance.call(@env)
-
-        assert @env.error?
-        assert_equal :package_output_exists, @env.error.first
+        assert_raises(Vagrant::Errors::PackageOutputExists) {
+          @instance.call(@env)
+        }
       end
 
       should "halt the chain if directory isn't set" do
         @env["package.directory"] = nil
         @app.expects(:call).never
-        @instance.call(@env)
-
-        assert @env.error?
-        assert_equal :package_requires_directory, @env.error.first
+        assert_raises(Vagrant::Errors::PackageRequiresDirectory) {
+          @instance.call(@env)
+        }
       end
 
       should "halt the chain if directory doesn't exist" do
         File.expects(:directory?).with(@env["package.directory"]).returns(false)
         @app.expects(:call).never
-        @instance.call(@env)
-
-        assert @env.error?
-        assert_equal :package_requires_directory, @env.error.first
+        assert_raises(Vagrant::Errors::PackageRequiresDirectory) {
+          @instance.call(@env)
+        }
       end
     end
 
@@ -129,9 +118,9 @@ class PackageGeneralActionTest < Test::Unit::TestCase
 
       should "error if included file is not found" do
         File.expects(:exist?).with("foo").returns(false)
-        assert !@instance.verify_included_files
-        assert @env.error?
-        assert_equal :package_include_file_doesnt_exist, @env.error.first
+        assert_raises(Vagrant::Errors::PackageIncludeMissing) {
+          @instance.verify_included_files
+        }
       end
 
       should "return true if all exist" do

test/vagrant/errors_test.rb

@@ -6,8 +6,15 @@ class ErrorsTest < Test::Unit::TestCase
   end
 
   should "set the given status code" do
-    klass = Class.new(@super) { status_code(4) }
-    assert_equal 4, klass.new.status_code
+    klass = Class.new(@super) { status_code(4444) }
+    assert_equal 4444, klass.new.status_code
+  end
+
+  should "raise an error if attempting to set the same status code twice" do
+    klass = Class.new(@super) { status_code(4445) }
+    assert_raises(RuntimeError) {
+      Class.new(@super) { status_code(4445) }
+    }
   end
 
   should "use the given message if no set error key" do