From e6f9586d83737fa089e2b6479d793f0410a3e136 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 12:14:40 -0800 Subject: [PATCH 01/18] New validation method on the root that returns errors --- lib/vagrant/config/v2/root.rb | 38 ++++++++++++--------- test/unit/vagrant/config/v2/root_test.rb | 43 ++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 16 deletions(-) diff --git a/lib/vagrant/config/v2/root.rb b/lib/vagrant/config/v2/root.rb index 32b804108..9b8cbf1f4 100644 --- a/lib/vagrant/config/v2/root.rb +++ b/lib/vagrant/config/v2/root.rb @@ -39,24 +39,30 @@ module Vagrant end end - # Validates the configuration classes of this instance and raises an - # exception if they are invalid. If you are implementing a custom configuration - # class, the method you want to implement is {Base#validate}. This is - # the method that checks all the validation, not one which defines - # validation rules. - def validate!(env) - # Validate each of the configured classes and store the results into - # a hash. - errors = @keys.inject({}) do |container, data| - key, instance = data - recorder = ErrorRecorder.new - instance.validate(env, recorder) - container[key.to_sym] = recorder if !recorder.errors.empty? - container + # This validates the configuration and returns a hash of error + # messages by section. If there are no errors, an empty hash + # is returned. + # + # @param [Environment] env + # @return [Hash] + def validate(env) + # Go through each of the configuration keys and validate + errors = {} + @keys.each do |_key, instance| + if instance.respond_to?(:validate) + # Validate this single item, and if we have errors then + # we merge them into our total errors list. + result = instance.validate(env) + if result && !result.empty? + result.each do |key, value| + errors[key] ||= [] + errors[key] += value + end + end + end end - return if errors.empty? - raise Errors::ConfigValidationFailed, :messages => Util::TemplateRenderer.render("config/validation_failed", :errors => errors) + errors end # Returns the internal state of the root object. This is used diff --git a/test/unit/vagrant/config/v2/root_test.rb b/test/unit/vagrant/config/v2/root_test.rb index 59140af77..10b5228a0 100644 --- a/test/unit/vagrant/config/v2/root_test.rb +++ b/test/unit/vagrant/config/v2/root_test.rb @@ -31,4 +31,47 @@ describe Vagrant::Config::V2::Root do "keys" => {} } end + + describe "validation" do + let(:instance) do + map = { :foo => Object, :bar => Object } + described_class.new(map) + end + + it "should return nil if valid" do + instance.validate({}).should == {} + end + + it "should return errors if invalid" do + errors = { "foo" => ["errors!"] } + env = { "errors" => errors } + foo = instance.foo + def foo.validate(env) + env["errors"] + end + + instance.validate(env).should == errors + end + + it "should merge errors via array concat if matching keys" do + errors = { "foo" => ["errors!"] } + env = { "errors" => errors } + foo = instance.foo + bar = instance.bar + def foo.validate(env) + env["errors"] + end + + def bar.validate(env) + env["errors"].merge({ "bar" => ["bar"] }) + end + + expected_errors = { + "foo" => ["errors!", "errors!"], + "bar" => ["bar"] + } + + instance.validate(env).should == expected_errors + end + end end From 42a1ce6e9d29bcff614bb32f7ae86b6322244503 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 12:15:36 -0800 Subject: [PATCH 02/18] Remove the old config validation error --- lib/vagrant/errors.rb | 5 ----- templates/locales/en.yml | 5 ----- 2 files changed, 10 deletions(-) diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 3822786e0..b01a13c3b 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -150,11 +150,6 @@ module Vagrant error_key(:cli_invalid_options) end - class ConfigValidationFailed < VagrantError - status_code(42) - error_key(:config_validation) - end - class DeprecationError < VagrantError status_code(60) error_key(:deprecation) diff --git a/templates/locales/en.yml b/templates/locales/en.yml index f9a4603eb..fb7dd4cd3 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -56,11 +56,6 @@ en: available below. %{help} - config_validation: |- - There was a problem with the configuration of Vagrant. The error message(s) - are printed below: - - %{messages} deprecation: |- You are using a feature that has been removed in this version. Explanation: From e0c8fadae43b28702becd1349c52f1d3cf2e3288 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 12:18:30 -0800 Subject: [PATCH 03/18] I can remove the `validate!` method from the v1 root --- lib/vagrant/config/v1/root.rb | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/lib/vagrant/config/v1/root.rb b/lib/vagrant/config/v1/root.rb index 7f561c431..f501a1fb3 100644 --- a/lib/vagrant/config/v1/root.rb +++ b/lib/vagrant/config/v1/root.rb @@ -39,26 +39,6 @@ module Vagrant end end - # Validates the configuration classes of this instance and raises an - # exception if they are invalid. If you are implementing a custom configuration - # class, the method you want to implement is {Base#validate}. This is - # the method that checks all the validation, not one which defines - # validation rules. - def validate!(env) - # Validate each of the configured classes and store the results into - # a hash. - errors = @keys.inject({}) do |container, data| - key, instance = data - recorder = ErrorRecorder.new - instance.validate(env, recorder) - container[key.to_sym] = recorder if !recorder.errors.empty? - container - end - - return if errors.empty? - raise Errors::ConfigValidationFailed, :messages => Util::TemplateRenderer.render("config/validation_failed", :errors => errors) - end - # Returns the internal state of the root object. This is used # by outside classes when merging, and shouldn't be called directly. # Note the strange method name is to attempt to avoid any name From bae65558361890eec2c3692114f47a730c7f9851 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 12:23:29 -0800 Subject: [PATCH 04/18] Remove DeprecationError --- lib/vagrant/errors.rb | 5 ----- templates/locales/en.yml | 5 ----- 2 files changed, 10 deletions(-) diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index b01a13c3b..ed62e8617 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -150,11 +150,6 @@ module Vagrant error_key(:cli_invalid_options) end - class DeprecationError < VagrantError - status_code(60) - error_key(:deprecation) - end - class DestroyRequiresForce < VagrantError status_code(74) error_key(:destroy_requires_force) diff --git a/templates/locales/en.yml b/templates/locales/en.yml index fb7dd4cd3..76bcfc7b0 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -56,11 +56,6 @@ en: available below. %{help} - deprecation: |- - You are using a feature that has been removed in this version. Explanation: - - %{message} - Note that this error message will not appear in the next version of Vagrant. 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 From a8b57ba13fc8ba4579e10f708887c57a071b160f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 12:27:29 -0800 Subject: [PATCH 05/18] Ignore empty error groups --- lib/vagrant/config/v2/root.rb | 6 ++++-- test/unit/vagrant/config/v2/root_test.rb | 11 +++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/vagrant/config/v2/root.rb b/lib/vagrant/config/v2/root.rb index 9b8cbf1f4..b2a6bf4f0 100644 --- a/lib/vagrant/config/v2/root.rb +++ b/lib/vagrant/config/v2/root.rb @@ -55,8 +55,10 @@ module Vagrant result = instance.validate(env) if result && !result.empty? result.each do |key, value| - errors[key] ||= [] - errors[key] += value + if !value.empty? + errors[key] ||= [] + errors[key] += value + end end end end diff --git a/test/unit/vagrant/config/v2/root_test.rb b/test/unit/vagrant/config/v2/root_test.rb index 10b5228a0..a705153ba 100644 --- a/test/unit/vagrant/config/v2/root_test.rb +++ b/test/unit/vagrant/config/v2/root_test.rb @@ -73,5 +73,16 @@ describe Vagrant::Config::V2::Root do instance.validate(env).should == expected_errors end + + it "shouldn't count empty keys" do + errors = { "foo" => [] } + env = { "errors" => errors } + foo = instance.foo + def foo.validate(env) + env["errors"] + end + + instance.validate(env).should == {} + end end end From 3e9e422ce0656875d05a0934a57358e67edf22b4 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 12:33:37 -0800 Subject: [PATCH 06/18] Convert existing validate methods to new API for kernel --- plugins/kernel_v2/config/ssh.rb | 11 ++++++++--- plugins/kernel_v2/config/vm.rb | 21 +++++++++++++-------- templates/locales/en.yml | 2 +- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/plugins/kernel_v2/config/ssh.rb b/plugins/kernel_v2/config/ssh.rb index 582f1361d..a296c83fd 100644 --- a/plugins/kernel_v2/config/ssh.rb +++ b/plugins/kernel_v2/config/ssh.rb @@ -15,15 +15,20 @@ module VagrantPlugins attr_accessor :forward_x11 attr_accessor :shell - def validate(env, errors) + def validate(env) + errors = [] + [:username, :max_tries, :timeout].each do |field| value = instance_variable_get("@#{field}".to_sym) - errors.add(I18n.t("vagrant.config.common.error_empty", :field => field)) if !value + errors << I18n.t("vagrant.config.common.error_empty", :field => field) if !value end if private_key_path && !File.file?(File.expand_path(private_key_path, env.root_path)) - errors.add(I18n.t("vagrant.config.ssh.private_key_missing", :path => private_key_path)) + errors << I18n.t("vagrant.config.ssh.private_key_missing", :path => private_key_path) end + + # Return the errors + { "ssh" => errors } end end end diff --git a/plugins/kernel_v2/config/vm.rb b/plugins/kernel_v2/config/vm.rb index 803a892d5..3c340c014 100644 --- a/plugins/kernel_v2/config/vm.rb +++ b/plugins/kernel_v2/config/vm.rb @@ -122,26 +122,31 @@ module VagrantPlugins define(DEFAULT_VM_NAME) if defined_vm_keys.empty? end - def validate(env, errors) - errors.add(I18n.t("vagrant.config.vm.box_missing")) if !box - errors.add(I18n.t("vagrant.config.vm.box_not_found", :name => box)) if box && !box_url && !env.boxes.find(box, :virtualbox) - errors.add(I18n.t("vagrant.config.vm.base_mac_invalid")) if env.boxes.find(box, :virtualbox) && !base_mac + def validate(env) + errors = [] + errors << I18n.t("vagrant.config.vm.box_missing") if !box + errors << I18n.t("vagrant.config.vm.box_not_found", :name => box) if \ + box && !box_url && !env.boxes.find(box, :virtualbox) + errors << I18n.t("vagrant.config.vm.base_mac_invalid") if \ + env.boxes.find(box, :virtualbox) && !base_mac shared_folders.each do |name, options| hostpath = Pathname.new(options[:hostpath]).expand_path(env.root_path) if !hostpath.directory? && !options[:create] - errors.add(I18n.t("vagrant.config.vm.shared_folder_hostpath_missing", + errors << I18n.t("vagrant.config.vm.shared_folder_hostpath_missing", :name => name, - :path => options[:hostpath])) + :path => options[:hostpath]) end if options[:nfs] && (options[:owner] || options[:group]) # Owner/group don't work with NFS - errors.add(I18n.t("vagrant.config.vm.shared_folder_nfs_owner_group", - :name => name)) + errors << I18n.t("vagrant.config.vm.shared_folder_nfs_owner_group", + :name => name) end end + + { "vm" => errors } end end end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 76bcfc7b0..c42e6492e 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -306,7 +306,7 @@ en: #------------------------------------------------------------------------------- config: common: - error_empty: "`%{field}` must be filled in." + error_empty: "`%{field}` must be not be empty." chef: cookbooks_path_empty: "Must specify a cookbooks path for chef solo." run_list_empty: "Run list must not be empty." From 3f3c7027aa92ca4866acc052ad7a0ef7ee46097e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 12:42:49 -0800 Subject: [PATCH 07/18] Machine objects are passed into validate instead of env --- lib/vagrant/config/v2/root.rb | 4 ++-- plugins/kernel_v2/config/ssh.rb | 5 +++-- plugins/kernel_v2/config/vm.rb | 8 +++----- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/vagrant/config/v2/root.rb b/lib/vagrant/config/v2/root.rb index b2a6bf4f0..d35b5f2f4 100644 --- a/lib/vagrant/config/v2/root.rb +++ b/lib/vagrant/config/v2/root.rb @@ -45,14 +45,14 @@ module Vagrant # # @param [Environment] env # @return [Hash] - def validate(env) + def validate(machine) # Go through each of the configuration keys and validate errors = {} @keys.each do |_key, instance| if instance.respond_to?(:validate) # Validate this single item, and if we have errors then # we merge them into our total errors list. - result = instance.validate(env) + result = instance.validate(machine) if result && !result.empty? result.each do |key, value| if !value.empty? diff --git a/plugins/kernel_v2/config/ssh.rb b/plugins/kernel_v2/config/ssh.rb index a296c83fd..eff0f8481 100644 --- a/plugins/kernel_v2/config/ssh.rb +++ b/plugins/kernel_v2/config/ssh.rb @@ -15,7 +15,7 @@ module VagrantPlugins attr_accessor :forward_x11 attr_accessor :shell - def validate(env) + def validate(machine) errors = [] [:username, :max_tries, :timeout].each do |field| @@ -23,7 +23,8 @@ module VagrantPlugins errors << I18n.t("vagrant.config.common.error_empty", :field => field) if !value end - if private_key_path && !File.file?(File.expand_path(private_key_path, env.root_path)) + if private_key_path && \ + !File.file?(File.expand_path(private_key_path, machine.env.root_path)) errors << I18n.t("vagrant.config.ssh.private_key_missing", :path => private_key_path) end diff --git a/plugins/kernel_v2/config/vm.rb b/plugins/kernel_v2/config/vm.rb index 3c340c014..7a33c4e80 100644 --- a/plugins/kernel_v2/config/vm.rb +++ b/plugins/kernel_v2/config/vm.rb @@ -122,16 +122,14 @@ module VagrantPlugins define(DEFAULT_VM_NAME) if defined_vm_keys.empty? end - def validate(env) + def validate(machine) errors = [] errors << I18n.t("vagrant.config.vm.box_missing") if !box errors << I18n.t("vagrant.config.vm.box_not_found", :name => box) if \ - box && !box_url && !env.boxes.find(box, :virtualbox) - errors << I18n.t("vagrant.config.vm.base_mac_invalid") if \ - env.boxes.find(box, :virtualbox) && !base_mac + box && !box_url && !machine.box shared_folders.each do |name, options| - hostpath = Pathname.new(options[:hostpath]).expand_path(env.root_path) + hostpath = Pathname.new(options[:hostpath]).expand_path(machine.env.root_path) if !hostpath.directory? && !options[:create] errors << I18n.t("vagrant.config.vm.shared_folder_hostpath_missing", From 515ed8f119f34e5659af8ecac5f254bef2c6ec0b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 12:56:19 -0800 Subject: [PATCH 08/18] Validate providers and provisioners! --- plugins/kernel_v2/config/vm.rb | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/plugins/kernel_v2/config/vm.rb b/plugins/kernel_v2/config/vm.rb index 7a33c4e80..eda7f4117 100644 --- a/plugins/kernel_v2/config/vm.rb +++ b/plugins/kernel_v2/config/vm.rb @@ -1,6 +1,7 @@ require "pathname" require "vagrant" +require "vagrant/config/v2/util" require File.expand_path("../vm_provider", __FILE__) require File.expand_path("../vm_provisioner", __FILE__) @@ -144,7 +145,26 @@ module VagrantPlugins end end - { "vm" => errors } + # We're done with VM level errors so prepare the section + errors = { "vm" => errors } + + # Validate providers + @providers.each do |_name, vm_provider| + if vm_provider.config + provider_errors = vm_provider.config.validate(machine) + errors = Vagrant::Config::V2::Util.merge_errors(errors, provider_errors) + end + end + + # Validate provisioners + @provisioners.each do |vm_provisioner| + if vm_provisioner.config + provisioner_errors = vm_provisioner.config.validate(machine) + errors = Vagrant::Config::V2::Util.merge_errors(errors, provisioner_errors) + end + end + + errors end end end From e651eb3aa13fd5daf67ab9e026dfd0836777af16 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:03:07 -0800 Subject: [PATCH 09/18] Add a V2 config helper to merge errors since that seems common --- lib/vagrant/config/v2/root.rb | 14 ++++++++------ lib/vagrant/config/v2/util.rb | 21 +++++++++++++++++++++ test/unit/vagrant/config/v2/util_test.rb | 21 +++++++++++++++++++++ 3 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 lib/vagrant/config/v2/util.rb create mode 100644 test/unit/vagrant/config/v2/util_test.rb diff --git a/lib/vagrant/config/v2/root.rb b/lib/vagrant/config/v2/root.rb index d35b5f2f4..419319c29 100644 --- a/lib/vagrant/config/v2/root.rb +++ b/lib/vagrant/config/v2/root.rb @@ -1,3 +1,5 @@ +require "vagrant/config/v2/util" + module Vagrant module Config module V2 @@ -54,16 +56,16 @@ module Vagrant # we merge them into our total errors list. result = instance.validate(machine) if result && !result.empty? - result.each do |key, value| - if !value.empty? - errors[key] ||= [] - errors[key] += value - end - end + errors = Util.merge_errors(errors, result) end end end + # Go through and delete empty keys + errors.keys.each do |key| + errors.delete(key) if errors[key].empty? + end + errors end diff --git a/lib/vagrant/config/v2/util.rb b/lib/vagrant/config/v2/util.rb new file mode 100644 index 000000000..798517017 --- /dev/null +++ b/lib/vagrant/config/v2/util.rb @@ -0,0 +1,21 @@ +module Vagrant + module Config + module V2 + class Util + # This merges two error hashes from validate methods. + # + # @param [Hash] first + # @param [Hash] second + # @return [Hash] Merged result + def self.merge_errors(first, second) + first.dup.tap do |result| + second.each do |key, value| + result[key] ||= [] + result[key] += value + end + end + end + end + end + end +end diff --git a/test/unit/vagrant/config/v2/util_test.rb b/test/unit/vagrant/config/v2/util_test.rb new file mode 100644 index 000000000..ee0d4d2f7 --- /dev/null +++ b/test/unit/vagrant/config/v2/util_test.rb @@ -0,0 +1,21 @@ +require File.expand_path("../../../../base", __FILE__) + +require "vagrant/config/v2/util" + +describe Vagrant::Config::V2::Util do + describe "merging errors" do + it "should merge matching keys and leave the rest alone" do + first = { "one" => ["foo"], "two" => ["two"] } + second = { "one" => ["bar"], "three" => ["three"] } + + expected = { + "one" => ["foo", "bar"], + "two" => ["two"], + "three" => ["three"] + } + + result = described_class.merge_errors(first, second) + result.should == expected + end + end +end From 5e42a99ab6cd1f98dd88f625354b309d81a083ec Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:06:29 -0800 Subject: [PATCH 10/18] Update shell provisioner to latest validation api --- plugins/provisioners/shell/config.rb | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/plugins/provisioners/shell/config.rb b/plugins/provisioners/shell/config.rb index ef5d10a3e..6c9e4d223 100644 --- a/plugins/provisioners/shell/config.rb +++ b/plugins/provisioners/shell/config.rb @@ -10,32 +10,36 @@ module VagrantPlugins @upload_path = "/tmp/vagrant-shell" end - def validate(env, errors) + def validate(machine) + errors = [] + # Validate that the parameters are properly set if path && inline - errors.add(I18n.t("vagrant.provisioners.shell.path_and_inline_set")) + errors << I18n.t("vagrant.provisioners.shell.path_and_inline_set") elsif !path && !inline - errors.add(I18n.t("vagrant.provisioners.shell.no_path_or_inline")) + errors << I18n.t("vagrant.provisioners.shell.no_path_or_inline") end # Validate the existence of a script to upload if path - expanded_path = Pathname.new(path).expand_path(env.root_path) + expanded_path = Pathname.new(path).expand_path(machine.env.root_path) if !expanded_path.file? - errors.add(I18n.t("vagrant.provisioners.shell.path_invalid", - :path => expanded_path)) + errors << I18n.t("vagrant.provisioners.shell.path_invalid", + :path => expanded_path) end end # There needs to be a path to upload the script to if !upload_path - errors.add(I18n.t("vagrant.provisioners.shell.upload_path_not_set")) + errors << I18n.t("vagrant.provisioners.shell.upload_path_not_set") end # If there are args and its not a string, that is a problem if args && !args.is_a?(String) - errors.add(I18n.t("vagrant.provisioners.shell.args_not_string")) + errors << I18n.t("vagrant.provisioners.shell.args_not_string") end + + { "shell provisioner" => errors } end end end From df32c47780f96a66b85294b717165474784dd149 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:08:38 -0800 Subject: [PATCH 11/18] Update puppet provisioner config to latest validation API --- plugins/provisioners/puppet/config/puppet.rb | 21 ++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/plugins/provisioners/puppet/config/puppet.rb b/plugins/provisioners/puppet/config/puppet.rb index b4d710802..9febf3e0e 100644 --- a/plugins/provisioners/puppet/config/puppet.rb +++ b/plugins/provisioners/puppet/config/puppet.rb @@ -35,29 +35,34 @@ module VagrantPlugins end end - def validate(env, errors) + def validate(machine) + errors = [] + # Calculate the manifests and module paths based on env - this_expanded_manifests_path = expanded_manifests_path(env.root_path) - this_expanded_module_paths = expanded_module_paths(env.root_path) + this_expanded_manifests_path = expanded_manifests_path(machine.env.root_path) + this_expanded_module_paths = expanded_module_paths(machine.env.root_path) # Manifests path/file validation if !this_expanded_manifests_path.directory? - errors.add(I18n.t("vagrant.provisioners.puppet.manifests_path_missing", - :path => this_expanded_manifests_path)) + errors << I18n.t("vagrant.provisioners.puppet.manifests_path_missing", + :path => this_expanded_manifests_path) else expanded_manifest_file = this_expanded_manifests_path.join(manifest_file) if !expanded_manifest_file.file? - errors.add(I18n.t("vagrant.provisioners.puppet.manifest_missing", - :manifest => expanded_manifest_file.to_s)) + errors << I18n.t("vagrant.provisioners.puppet.manifest_missing", + :manifest => expanded_manifest_file.to_s) end end # Module paths validation this_expanded_module_paths.each do |path| if !path.directory? - errors.add(I18n.t("vagrant.provisioners.puppet.module_path_missing", :path => path)) + errors << I18n.t("vagrant.provisioners.puppet.module_path_missing", + :path => path) end end + + { "puppet provisioner" => errors } end end end From fff021e51d92155e4525039db8c84e474f3b72a0 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:12:02 -0800 Subject: [PATCH 12/18] Update Chef provisioner to new validation API --- lib/vagrant/plugin/v2/config.rb | 10 ++++------ plugins/provisioners/chef/config/base.rb | 6 ------ plugins/provisioners/chef/config/chef_client.rb | 14 +++++++++----- plugins/provisioners/chef/config/chef_solo.rb | 11 +++++++---- templates/locales/en.yml | 2 -- 5 files changed, 20 insertions(+), 23 deletions(-) diff --git a/lib/vagrant/plugin/v2/config.rb b/lib/vagrant/plugin/v2/config.rb index a74eb670c..f3db8e610 100644 --- a/lib/vagrant/plugin/v2/config.rb +++ b/lib/vagrant/plugin/v2/config.rb @@ -90,12 +90,10 @@ module Vagrant # Called after the configuration is finalized and loaded to validate # this object. # - # @param [Environment] env Vagrant::Environment object of the - # environment that this configuration has been loaded into. This - # gives you convenient access to things like the the root path - # and so on. - # @param [ErrorRecorder] errors - def validate(env, errors) + # @param [Machine] machine Access to the machine that is being + # validated. + # @return [Hash] + def validate(machine) end end end diff --git a/plugins/provisioners/chef/config/base.rb b/plugins/provisioners/chef/config/base.rb index 67bc2283a..f097d03b2 100644 --- a/plugins/provisioners/chef/config/base.rb +++ b/plugins/provisioners/chef/config/base.rb @@ -52,12 +52,6 @@ module VagrantPlugins run_list << name end - def validate(env, errors) - super - - errors.add(I18n.t("vagrant.config.chef.vagrant_as_json_key")) if json.has_key?(:vagrant) - end - def instance_variables_hash # Overridden so that the 'json' key could be removed, since its just # merged into the config anyways diff --git a/plugins/provisioners/chef/config/chef_client.rb b/plugins/provisioners/chef/config/chef_client.rb index ea734cc86..7d89aa5fd 100644 --- a/plugins/provisioners/chef/config/chef_client.rb +++ b/plugins/provisioners/chef/config/chef_client.rb @@ -22,12 +22,16 @@ module VagrantPlugins def file_backup_path; @file_backup_path || "/srv/chef/cache"; end def encrypted_data_bag_secret; @encrypted_data_bag_secret || "/tmp/encrypted_data_bag_secret"; end - def validate(env, errors) - super + def validate(machine) + errors = [] + errors << I18n.t("vagrant.config.chef.server_url_empty") if \ + !chef_server_url || chef_server_url.strip == "" + errors << I18n.t("vagrant.config.chef.validation_key_path") if \ + !validation_key_path + errors << I18n.t("vagrant.config.chef.run_list_empty") if \ + @run_list && @run_list.empty? - errors.add(I18n.t("vagrant.config.chef.server_url_empty")) if !chef_server_url || chef_server_url.strip == "" - errors.add(I18n.t("vagrant.config.chef.validation_key_path")) if !validation_key_path - errors.add(I18n.t("vagrant.config.chef.run_list_empty")) if @run_list && @run_list.empty? + { "chef client provisioner" => errors } end end end diff --git a/plugins/provisioners/chef/config/chef_solo.rb b/plugins/provisioners/chef/config/chef_solo.rb index 351e10fef..552af423b 100644 --- a/plugins/provisioners/chef/config/chef_solo.rb +++ b/plugins/provisioners/chef/config/chef_solo.rb @@ -37,11 +37,14 @@ module VagrantPlugins @nfs || false end - def validate(env, errors) - super + def validate(machine) + errors = [] + errors << I18n.t("vagrant.config.chef.cookbooks_path_empty") if \ + !cookbooks_path || [cookbooks_path].flatten.empty? + errors << I18n.t("vagrant.config.chef.run_list_empty") if \ + !run_list || run_list.empty? - errors.add(I18n.t("vagrant.config.chef.cookbooks_path_empty")) if !cookbooks_path || [cookbooks_path].flatten.empty? - errors.add(I18n.t("vagrant.config.chef.run_list_empty")) if !run_list || run_list.empty? + { "chef solo provisioner" => errors } end end end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index c42e6492e..a5751cdc1 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -311,8 +311,6 @@ en: cookbooks_path_empty: "Must specify a cookbooks path for chef solo." run_list_empty: "Run list must not be empty." server_url_empty: "Chef server URL must be populated." - vagrant_as_json_key: |- - `vagrant` cannot be a JSON key, because it is used by Vagrant already. validation_key_path: "Validation key path must be valid path to your chef server validation key." ssh: private_key_missing: "`private_key_path` file must exist: %{path}" From 37e36010e017141f214de2121b5c036036730f59 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:15:22 -0800 Subject: [PATCH 13/18] Remove the ErrorRecorder --- lib/vagrant/config.rb | 1 - lib/vagrant/config/error_recorder.rb | 19 ------------------- 2 files changed, 20 deletions(-) delete mode 100644 lib/vagrant/config/error_recorder.rb diff --git a/lib/vagrant/config.rb b/lib/vagrant/config.rb index 1bb6cd401..74a28edbd 100644 --- a/lib/vagrant/config.rb +++ b/lib/vagrant/config.rb @@ -4,7 +4,6 @@ module Vagrant module Config autoload :Base, 'vagrant/config/base' autoload :Container, 'vagrant/config/container' - autoload :ErrorRecorder, 'vagrant/config/error_recorder' autoload :Loader, 'vagrant/config/loader' autoload :VersionBase, 'vagrant/config/version_base' diff --git a/lib/vagrant/config/error_recorder.rb b/lib/vagrant/config/error_recorder.rb deleted file mode 100644 index 5a5b8e427..000000000 --- a/lib/vagrant/config/error_recorder.rb +++ /dev/null @@ -1,19 +0,0 @@ -module Vagrant - module Config - # A class which is passed into the various {Base#validate} methods and - # can be used as a helper to add error messages about a single config - # class. - class ErrorRecorder - attr_reader :errors - - def initialize - @errors = [] - end - - # Adds an error to the list of errors. - def add(message) - @errors << message - end - end - end -end From 7f55d5eac8f27f09aa9d93f46c5d19ed85461e02 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:26:14 -0800 Subject: [PATCH 14/18] Builtin: ConfigValidate will validate your configuration --- lib/vagrant/action.rb | 2 +- lib/vagrant/action/builtin/config_validate.rb | 30 +++++++++++++++++++ lib/vagrant/action/general/validate.rb | 21 ------------- lib/vagrant/errors.rb | 4 +++ plugins/providers/virtualbox/action/import.rb | 2 +- templates/config/validation_failed.erb | 6 ++-- templates/locales/en.yml | 5 ++++ 7 files changed, 44 insertions(+), 26 deletions(-) create mode 100644 lib/vagrant/action/builtin/config_validate.rb delete mode 100644 lib/vagrant/action/general/validate.rb diff --git a/lib/vagrant/action.rb b/lib/vagrant/action.rb index bb75519d2..e9606e8a8 100644 --- a/lib/vagrant/action.rb +++ b/lib/vagrant/action.rb @@ -12,6 +12,7 @@ module Vagrant autoload :BoxAdd, "vagrant/action/builtin/box_add" autoload :Call, "vagrant/action/builtin/call" autoload :Confirm, "vagrant/action/builtin/confirm" + autoload :ConfigValidate, "vagrant/action/builtin/config_validate" autoload :EnvSet, "vagrant/action/builtin/env_set" autoload :Provision, "vagrant/action/builtin/provision" autoload :SSHExec, "vagrant/action/builtin/ssh_exec" @@ -20,7 +21,6 @@ module Vagrant module General autoload :Package, 'vagrant/action/general/package' - autoload :Validate, 'vagrant/action/general/validate' end # This is the action that will add a box from a URL. This middleware diff --git a/lib/vagrant/action/builtin/config_validate.rb b/lib/vagrant/action/builtin/config_validate.rb new file mode 100644 index 000000000..87603b860 --- /dev/null +++ b/lib/vagrant/action/builtin/config_validate.rb @@ -0,0 +1,30 @@ +require "vagrant/util/template_renderer" + +module Vagrant + module Action + module Builtin + # This class validates the configuration and raises an exception + # if there are any validation errors. + class ConfigValidate + def initialize(app, env) + @app = app + end + + def call(env) + if !env.has_key?(:config_validate) || env[:config_validate] + errors = env[:machine].config.validate(env[:machine]) + + if errors + raise Errors::ConfigInvalid, + :errors => Util::TemplateRenderer.render( + "config/validation_failed", + :errors => errors) + end + end + + @app.call(env) + end + end + end + end +end diff --git a/lib/vagrant/action/general/validate.rb b/lib/vagrant/action/general/validate.rb deleted file mode 100644 index f566b793f..000000000 --- a/lib/vagrant/action/general/validate.rb +++ /dev/null @@ -1,21 +0,0 @@ -module Vagrant - module Action - module General - # Simply validates the configuration of the current Vagrant - # environment. - class Validate - def initialize(app, env) - @app = app - end - - def call(env) - if !env.has_key?(:validate) || env[:validate] - env[:machine].config.validate!(env[:machine].env) - end - - @app.call(env) - end - end - end - end -end diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index ed62e8617..4fdc5ce2c 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -150,6 +150,10 @@ module Vagrant error_key(:cli_invalid_options) end + class ConfigInvalid < VagrantError + error_key(:config_invalid) + end + class DestroyRequiresForce < VagrantError status_code(74) error_key(:destroy_requires_force) diff --git a/plugins/providers/virtualbox/action/import.rb b/plugins/providers/virtualbox/action/import.rb index e63dfc950..d7f287e90 100644 --- a/plugins/providers/virtualbox/action/import.rb +++ b/plugins/providers/virtualbox/action/import.rb @@ -40,7 +40,7 @@ module VagrantPlugins # validate the configuration here, and we don't want to confirm # we want to destroy. destroy_env = env.clone - destroy_env[:validate] = false + destroy_env[:config_validate] = false destroy_env[:force_confirm_destroy] = true env[:action_runner].run(Action.action_destroy, destroy_env) end diff --git a/templates/config/validation_failed.erb b/templates/config/validation_failed.erb index 3e8d27b09..f8de6bb2a 100644 --- a/templates/config/validation_failed.erb +++ b/templates/config/validation_failed.erb @@ -1,6 +1,6 @@ -<% errors.each do |key, container| -%> -<%= key %>: -<% container.errors.each do |error| -%> +<% errors.each do |section, list| -%> +<%= section %>: +<% list.each do |error| -%> * <%= error %> <% end -%> diff --git a/templates/locales/en.yml b/templates/locales/en.yml index a5751cdc1..f76cebe55 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -56,6 +56,11 @@ en: available below. %{help} + config_invalid: |- + There are errors in the configuration of this machine. Please fix + the following errors and try again: + + %{errors} 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 From 003ebc811ccc1c67981896931aa2ef5a4ffebd46 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:29:20 -0800 Subject: [PATCH 15/18] VirtualBox uses the new validation middleware --- plugins/providers/virtualbox/action.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/plugins/providers/virtualbox/action.rb b/plugins/providers/virtualbox/action.rb index 45bdb1e95..9a0908dd3 100644 --- a/plugins/providers/virtualbox/action.rb +++ b/plugins/providers/virtualbox/action.rb @@ -85,7 +85,7 @@ module VagrantPlugins b2.use Call, DestroyConfirm do |env2, b3| if env2[:result] - b3.use Vagrant::Action::General::Validate + b3.use ConfigValidate b3.use CheckAccessible b3.use EnvSet, :force => true b3.use action_halt @@ -144,7 +144,7 @@ module VagrantPlugins def self.action_provision Vagrant::Action::Builder.new.tap do |b| b.use CheckVirtualbox - b.use Vagrant::Action::General::Validate + b.use ConfigValidate b.use Call, Created do |env1, b2| if !env1[:result] b2.use MessageNotCreated @@ -176,7 +176,7 @@ module VagrantPlugins next end - b2.use Vagrant::Action::General::Validate + b2.use ConfigValidate b2.use action_halt b2.use action_start end @@ -228,7 +228,7 @@ module VagrantPlugins def self.action_start Vagrant::Action::Builder.new.tap do |b| b.use CheckVirtualbox - b.use Vagrant::Action::General::Validate + b.use ConfigValidate b.use Call, IsRunning do |env, b2| # If the VM is running, then our work here is done, exit next if env[:result] @@ -268,7 +268,7 @@ module VagrantPlugins def self.action_up Vagrant::Action::Builder.new.tap do |b| b.use CheckVirtualbox - b.use Vagrant::Action::General::Validate + b.use ConfigValidate b.use Call, Created do |env, b2| # If the VM is NOT created yet, then do the setup steps if !env[:result] From d64c164727fbddc5657b032df03cc2e833196e3b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:33:02 -0800 Subject: [PATCH 16/18] VM configuration only validates the active provider --- plugins/kernel_v2/config/vm.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/plugins/kernel_v2/config/vm.rb b/plugins/kernel_v2/config/vm.rb index eda7f4117..7021727b9 100644 --- a/plugins/kernel_v2/config/vm.rb +++ b/plugins/kernel_v2/config/vm.rb @@ -148,12 +148,10 @@ module VagrantPlugins # We're done with VM level errors so prepare the section errors = { "vm" => errors } - # Validate providers - @providers.each do |_name, vm_provider| - if vm_provider.config - provider_errors = vm_provider.config.validate(machine) - errors = Vagrant::Config::V2::Util.merge_errors(errors, provider_errors) - end + # Validate only the _active_ provider + if machine.provider_config + provider_errors = machine.provider_config.validate(machine) + errors = Vagrant::Config::V2::Util.merge_errors(errors, provider_errors) end # Validate provisioners From 58eac7117b7cb2d84eb7f52092aeb9f1849d08db Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:36:12 -0800 Subject: [PATCH 17/18] Only merge provider/provisioner errors if they exist --- plugins/kernel_v2/config/vm.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/plugins/kernel_v2/config/vm.rb b/plugins/kernel_v2/config/vm.rb index 7021727b9..3df3c5ca1 100644 --- a/plugins/kernel_v2/config/vm.rb +++ b/plugins/kernel_v2/config/vm.rb @@ -151,14 +151,18 @@ module VagrantPlugins # Validate only the _active_ provider if machine.provider_config provider_errors = machine.provider_config.validate(machine) - errors = Vagrant::Config::V2::Util.merge_errors(errors, provider_errors) + if provider_errors + errors = Vagrant::Config::V2::Util.merge_errors(errors, provider_errors) + end end # Validate provisioners @provisioners.each do |vm_provisioner| if vm_provisioner.config provisioner_errors = vm_provisioner.config.validate(machine) - errors = Vagrant::Config::V2::Util.merge_errors(errors, provisioner_errors) + if provisioner_errors + errors = Vagrant::Config::V2::Util.merge_errors(errors, provisioner_errors) + end end end From ffd9c1eb9c1822f2d592dde1d030ca167b4faa38 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Jan 2013 13:37:31 -0800 Subject: [PATCH 18/18] Verify we have errors to show if we're going to show them --- lib/vagrant/action/builtin/config_validate.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vagrant/action/builtin/config_validate.rb b/lib/vagrant/action/builtin/config_validate.rb index 87603b860..0f727da32 100644 --- a/lib/vagrant/action/builtin/config_validate.rb +++ b/lib/vagrant/action/builtin/config_validate.rb @@ -14,7 +14,7 @@ module Vagrant if !env.has_key?(:config_validate) || env[:config_validate] errors = env[:machine].config.validate(env[:machine]) - if errors + if errors && !errors.empty? raise Errors::ConfigInvalid, :errors => Util::TemplateRenderer.render( "config/validation_failed",