From 1c345cce25e9276b38d8603bc1a4a79c8b999c2e Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Sat, 29 Aug 2026 18:50:47 -0700 Subject: [PATCH] fix: fail with actionable errors on misconfigured verifier options Every one of these is a plausible kitchen.yml typo that used to surface as something unrelated, or not at all: - `install_modules`, `register_repository`, `bootstrap.modules` and `copy_folders` are lists. Written as a single YAML mapping -- easy to do, it is one missing `- ` -- `Array()` turned the mapping into a list of `[key, value]` pairs and we generated `Install-Module -Name '[:Name, "PSScriptAnalyzer"]'` without complaint, or raised `TypeError: no implicit conversion of Symbol into Integer` from inside the verifier. - A mapping entry with no `Name` raised `NoMethodError: undefined method 'gsub' for nil` for `install_modules`, and for the other two emitted an empty `${}` that failed on the instance a long way from its cause. - A `test_folder` that does not exist raised `Errno::ENOENT: No such file or directory @ rb_check_realpath_internal`, which names neither the option nor kitchen.yml. - A `downloads` entry with a blank value raised `NoMethodError: undefined method 'gsub' for nil`. They now raise `Kitchen::UserError` naming the option at fault and the shape it expects. Valid configuration is unaffected, and there is a spec for each message alongside the shapes that must keep working. Signed-off-by: Tim Smith --- lib/kitchen/verifier/pester.rb | 87 +++++++++-- .../verifier/pester_config_errors_spec.rb | 147 ++++++++++++++++++ 2 files changed, 222 insertions(+), 12 deletions(-) create mode 100644 spec/kitchen/verifier/pester_config_errors_spec.rb diff --git a/lib/kitchen/verifier/pester.rb b/lib/kitchen/verifier/pester.rb index 43a3624..6cb34e9 100644 --- a/lib/kitchen/verifier/pester.rb +++ b/lib/kitchen/verifier/pester.rb @@ -221,7 +221,13 @@ def resolve_downloads_paths! config[:downloads] = config[:downloads] .map do |source, destination| source = source.to_s - destination = destination.gsub("%{instance_name}", instance.name) + if destination.nil? + raise UserError, "The verifier's 'downloads' entry for '#{source}' has no local " \ + "destination. Every entry needs one, for example " \ + "'#{source}: ./testresults/'." + end + + destination = destination.to_s.gsub("%{instance_name}", instance.name) info(" resolving remote source's absolute path.") unless source.match?(%r{^/|^[a-zA-Z]:[\\/]}) # is Absolute? info(" '#{source}' is a relative path, resolving to: #{File.join(config[:root_path], source)}") @@ -373,12 +379,13 @@ def get_powershell_modules_from_nugetapi gallery_url_param = bootstrap[:repository_url] ? "-GalleryUrl '#{bootstrap[:repository_url]}'" : "" info("Bootstrapping environment without PowerShellGet Provider...") - Array(bootstrap[:modules]).map do |powershell_module| + config_list("bootstrap.modules", bootstrap[:modules]).map do |powershell_module| if powershell_module.is_a? Hash + module_name = module_name!("bootstrap.modules", powershell_module) <<-PS1 - ${#{powershell_module[:Name]}} = #{ps_hash(powershell_module)} + ${#{module_name}} = #{ps_hash(powershell_module)} - Install-ModuleFromNuget -Module ${#{powershell_module[:Name]}} #{gallery_url_param} + Install-ModuleFromNuget -Module ${#{module_name}} #{gallery_url_param} PS1 else <<-PS1 @@ -397,13 +404,14 @@ def register_psrepository_scriptblock return if config[:register_repository].nil? info("Registering a new PowerShellGet Repository") - Array(config[:register_repository]).map do |psrepo| + config_list("register_repository", config[:register_repository]).map do |psrepo| + repo_name = module_name!("register_repository", psrepo) # Using Set-PSRepo from ../../*/*/*/PesterUtil.psm1 - debug("Command to set PSRepo #{psrepo[:Name]}.") + debug("Command to set PSRepo #{repo_name}.") <<-PS1 - Write-Host 'Registering psrepo #{psrepo[:Name]}...' - ${#{psrepo[:Name]}} = #{ps_hash(psrepo)} - Set-PSRepo -Repository ${#{psrepo[:Name]}} + Write-Host 'Registering psrepo #{repo_name}...' + ${#{repo_name}} = #{ps_hash(psrepo)} + Set-PSRepo -Repository ${#{repo_name}} PS1 end end @@ -439,10 +447,10 @@ def install_pester def install_modules_from_gallery return if config[:install_modules].nil? - Array(config[:install_modules]).map do |powershell_module| + config_list("install_modules", config[:install_modules]).map do |powershell_module| if powershell_module.is_a? Hash # Sanitize variable name so that $powershell-yaml becomes $powershell_yaml - module_name = powershell_module[:Name].gsub(/[\W]/, "_") + module_name = module_name!("install_modules", powershell_module).gsub(/[\W]/, "_") # so we can splat that variable to install module <<-PS1 $#{module_name} = #{ps_hash(powershell_module)} @@ -785,7 +793,7 @@ def prepare_copy_folders info("Preparing to copy specified folders to #{sandbox_module_path}.") kitchen_root_path = config[:kitchen_root] - config[:copy_folders].each do |folder| + config_list("copy_folders", config[:copy_folders]).each do |folder| debug("copying #{folder}") folder_to_copy = File.join(kitchen_root_path, folder) copy_if_src_exists(folder_to_copy, sandbox_module_path) @@ -867,6 +875,61 @@ def absolute_test_folder path = (Pathname.new config[:test_folder]).realpath integration_path = File.join(path, "integration") Dir.exist?(integration_path) ? integration_path : path.to_s + rescue Errno::ENOENT + raise UserError, "The verifier's 'test_folder' is set to " \ + "'#{config[:test_folder]}', which does not exist. It is resolved " \ + "relative to the directory kitchen runs in, so give it a path that " \ + "exists there or an absolute one." + end + + # Returns the entries of a config option that is documented as a list. + # + # YAML makes it easy to write a single mapping where a list of mappings + # was meant -- leaving off the leading `- ` is enough. `Array()` turns + # such a mapping into a list of `[key, value]` pairs, which then renders + # as nonsense PowerShell instead of failing, so reject it here where we + # can still say which option is at fault. + # + # @param key [String] the option's name, for the error message + # @param value [Object] the configured value + # @return [Array] the entries to iterate over + # @raise [Kitchen::UserError] when a single mapping was given + # @api private + def config_list(key, value) + if value.is_a?(Hash) + raise UserError, "The verifier's '#{key}' must be a list, but a single mapping " \ + "was given. Put a '- ' in front of each entry in kitchen.yml." + end + + Array(value) + end + + # Returns the Name of a mapping-shaped entry in one of the module or + # repository lists. + # + # The name becomes a PowerShell variable that the generated script splats, + # so a missing one either blows up here or emits an empty `${}` that fails + # on the instance a long way from its cause. + # + # @param key [String] the option's name, for the error message + # @param entry [Hash] the entry to read the name from + # @return [String] the entry's Name + # @raise [Kitchen::UserError] when the entry is not a mapping, or has no + # usable Name + # @api private + def module_name!(key, entry) + unless entry.is_a?(Hash) + raise UserError, "Every entry under the verifier's '#{key}' must be a mapping with " \ + "at least a 'Name'; #{entry.inspect} is a #{entry.class}." + end + + name = entry[:Name] || entry["Name"] + if name.to_s.empty? + raise UserError, "Every mapping under the verifier's '#{key}' needs a 'Name'; " \ + "#{entry.inspect} has none." + end + + name.to_s end # Returns the final segment of a path that lives on the SUT. diff --git a/spec/kitchen/verifier/pester_config_errors_spec.rb b/spec/kitchen/verifier/pester_config_errors_spec.rb new file mode 100644 index 0000000..3267b57 --- /dev/null +++ b/spec/kitchen/verifier/pester_config_errors_spec.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +# Copyright (c) 2015 Steven Murawski +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. + +require_relative "../../spec_helper" + +# Covers what happens when kitchen.yml is wrong rather than when it is right. +# +# Every option below is documented as a list of mappings or as a mapping with a +# Name, and YAML makes all of them easy to get subtly wrong -- a missing `- `, +# a forgotten key, a value left blank. The verifier interpolates these straight +# into PowerShell, so an unchecked mistake either raises somewhere unrelated in +# Ruby or ships a broken script to the instance and fails there. These specs +# pin the messages that say which option is at fault. +describe Kitchen::Verifier::Pester do + describe "a list option given as a single mapping" do + it "rejects install_modules" do + verifier = build_verifier({ install_modules: { Name: "PSScriptAnalyzer" } }) + + error = _ { verifier.install_modules_from_gallery }.must_raise Kitchen::UserError + _(error.message).must_include "'install_modules' must be a list" + _(error.message).must_include "Put a '- ' in front of each entry" + end + + it "rejects register_repository" do + verifier = build_verifier({ register_repository: { Name: "MyRepo", SourceLocation: "https://example.test/api/v2" } }) + + error = _ { verifier.register_psrepository_scriptblock }.must_raise Kitchen::UserError + _(error.message).must_include "'register_repository' must be a list" + end + + it "rejects bootstrap.modules" do + verifier = build_verifier({ bootstrap: { repository_url: "https://example.test/api/v2", modules: { Name: "PowerShellGet" } } }) + + error = _ { verifier.get_powershell_modules_from_nugetapi }.must_raise Kitchen::UserError + _(error.message).must_include "'bootstrap.modules' must be a list" + end + + it "rejects copy_folders" do + verifier = build_verifier({ copy_folders: { output: "MyModule" }, kitchen_root: "/kitchen" }) + verifier.stubs(:sandbox_path).returns("/kitchen/sandbox") + + error = _ { verifier.send(:prepare_copy_folders) }.must_raise Kitchen::UserError + _(error.message).must_include "'copy_folders' must be a list" + end + + it "still accepts a list, which is the documented shape" do + verifier = build_verifier({ install_modules: [{ Name: "PSScriptAnalyzer" }] }) + + _(verifier.install_modules_from_gallery.first).must_include "Install-Module @PSScriptAnalyzer" + end + + it "still accepts a bare string entry" do + verifier = build_verifier({ install_modules: %w{PSScriptAnalyzer} }) + + _(verifier.install_modules_from_gallery.first).must_include "Install-Module -Name 'PSScriptAnalyzer'" + end + end + + describe "a mapping entry with no Name" do + it "rejects an install_modules entry" do + verifier = build_verifier({ install_modules: [{ Repository: "MyRepo" }] }) + + error = _ { verifier.install_modules_from_gallery }.must_raise Kitchen::UserError + _(error.message).must_include "'install_modules' needs a 'Name'" + _(error.message).must_include "Repository" + end + + it "rejects a register_repository entry" do + verifier = build_verifier({ register_repository: [{ SourceLocation: "https://example.test/api/v2" }] }) + + error = _ { verifier.register_psrepository_scriptblock }.must_raise Kitchen::UserError + _(error.message).must_include "'register_repository' needs a 'Name'" + end + + it "rejects a bootstrap.modules entry" do + verifier = build_verifier({ bootstrap: { repository_url: "https://example.test/api/v2", modules: [{ Version: "2.2.5" }] } }) + + error = _ { verifier.get_powershell_modules_from_nugetapi }.must_raise Kitchen::UserError + _(error.message).must_include "'bootstrap.modules' needs a 'Name'" + end + + it "rejects a Name that is present but empty" do + verifier = build_verifier({ install_modules: [{ Name: "" }] }) + + _ { verifier.install_modules_from_gallery }.must_raise Kitchen::UserError + end + + it "rejects a register_repository entry that is a bare string, since it is splatted" do + verifier = build_verifier({ register_repository: %w{MyRepo} }) + + error = _ { verifier.register_psrepository_scriptblock }.must_raise Kitchen::UserError + _(error.message).must_include "must be a mapping" + end + + it "accepts a string key, since not every config path symbolizes keys" do + verifier = build_verifier({ install_modules: [{ "Name" => "PSScriptAnalyzer" }] }) + + _(verifier.install_modules_from_gallery.first).must_include "Install-Module @PSScriptAnalyzer" + end + end + + describe "a test_folder that does not exist" do + it "names the option and the path instead of raising Errno::ENOENT" do + verifier = build_verifier({ test_folder: "definitely/not/here" }) + + error = _ { verifier.send(:test_folder) }.must_raise Kitchen::UserError + _(error.message).must_include "'test_folder'" + _(error.message).must_include "definitely/not/here" + _(error.message).must_include "does not exist" + end + + it "surfaces through create_sandbox, where the folder is first read" do + verifier = build_verifier({ test_folder: "definitely/not/here" }) + + _ { verifier.create_sandbox }.must_raise Kitchen::UserError + end + end + + describe "a downloads entry with no destination" do + it "names the source it belongs to" do + verifier = build_verifier({ downloads: { "PesterTestResults.xml" => nil }, root_path: "/tmp/verifier" }) + + error = _ { verifier.resolve_downloads_paths! }.must_raise Kitchen::UserError + _(error.message).must_include "has no local destination" + _(error.message).must_include "PesterTestResults.xml" + end + end +end