From 98bce8f836b9ff51f39b8baab43bf06364b031fc Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 28 Aug 2010 13:54:59 -0700 Subject: [PATCH] General package action raises exceptions instead of using env.error --- lib/vagrant/action/general/package.rb | 14 +++------ lib/vagrant/errors.rb | 25 +++++++++++++-- templates/locales/en.yml | 8 +++++ templates/strings.yml | 9 ------ test/vagrant/action/general/package_test.rb | 35 +++++++-------------- test/vagrant/errors_test.rb | 11 +++++-- 6 files changed, 57 insertions(+), 45 deletions(-) diff --git a/lib/vagrant/action/general/package.rb b/lib/vagrant/action/general/package.rb index d65412243..61531ced9 100644 --- a/lib/vagrant/action/general/package.rb +++ b/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) diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 88f08b7d8..36e285600 100644 --- a/lib/vagrant/errors.rb +++ b/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 diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 487dae2d5..e3b34b883 100644 --- a/templates/locales/en.yml +++ b/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: diff --git a/templates/strings.yml b/templates/strings.yml index 91ce2df51..649e720d9 100644 --- a/templates/strings.yml +++ b/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: |- diff --git a/test/vagrant/action/general/package_test.rb b/test/vagrant/action/general/package_test.rb index eba87c37b..fe8424aab 100644 --- a/test/vagrant/action/general/package_test.rb +++ b/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 diff --git a/test/vagrant/errors_test.rb b/test/vagrant/errors_test.rb index f06c72ba8..84919ebd7 100644 --- a/test/vagrant/errors_test.rb +++ b/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