From df26f70c9b8a936a2576d60a62b2e8c97d08e86c Mon Sep 17 00:00:00 2001 From: Jon Morrow Date: Fri, 18 May 2018 13:07:01 -0700 Subject: [PATCH] Fixes error handling when an invalid flag is passed. Signed-off-by: Jon Morrow --- components/chef-cli/i18n/en.yml | 5 + .../chef-cli/lib/chef-cli/command/base.rb | 21 +++-- components/chef-run/i18n/en.yml | 6 ++ components/chef-run/lib/chef-run/cli.rb | 29 ++++-- components/chef-run/spec/unit/cli_spec.rb | 91 +++++++++---------- 5 files changed, 89 insertions(+), 63 deletions(-) diff --git a/components/chef-cli/i18n/en.yml b/components/chef-cli/i18n/en.yml index b56d65d84..f3bd3fd77 100644 --- a/components/chef-cli/i18n/en.yml +++ b/components/chef-cli/i18n/en.yml @@ -317,6 +317,11 @@ errors: CHEFVAL009: | File extension '%1' is unsupported. Currently recipes must be specified with a `.rb` extension. + CHEFVAL010: | + The flag '%1' does not exist. + + Available flags are: + %2 # General errors/unknown errors are handled with CHEFINT CHEFINT001: | An unexpected error has occurred: diff --git a/components/chef-cli/lib/chef-cli/command/base.rb b/components/chef-cli/lib/chef-cli/command/base.rb index f1a56a6b4..2c9ba50f8 100644 --- a/components/chef-cli/lib/chef-cli/command/base.rb +++ b/components/chef-cli/lib/chef-cli/command/base.rb @@ -69,6 +69,12 @@ def run_with_default_options(params = []) else run(params) end + rescue OptionParser::InvalidOption => e + # Using nil here is a bit gross but it prevents usage from printing. + raise OptionValidationError.new("CHEFVAL010", nil, + e.message.split(":")[1].strip, # only want the flag + format_flags.lines[1..-1].join # remove 'FLAGS:' header + ) end # This is normally overridden by the command implementations, but @@ -153,8 +159,11 @@ def show_help end def show_help_flags - UI::Terminal.output "" - UI::Terminal.output "FLAGS:" + UI::Terminal.output "\n#{format_flags}" + end + + def format_flags + flag_text = "FLAGS:\n" justify_length = 0 options.each_value do |spec| justify_length = [justify_length, spec[:long].length + 4].max @@ -168,16 +177,16 @@ def show_help_flags short = "#{short}, " end flags = "#{short}#{flag_spec[:long]}" - UI::Terminal.write(" #{flags.ljust(justify_length)} ") + flag_text << " #{flags.ljust(justify_length)} " ml_padding = " " * (justify_length + 8) first = true flag_spec[:description].split("\n").each do |d| - UI::Terminal.write(ml_padding) unless first + flag_text << ml_padding unless first first = false - UI::Terminal.write(d) - UI::Terminal.write("\n") + flag_text << "#{d}\n" end end + flag_text end def show_help_subcommands diff --git a/components/chef-run/i18n/en.yml b/components/chef-run/i18n/en.yml index 0d32944f4..9a76fbb8c 100644 --- a/components/chef-run/i18n/en.yml +++ b/components/chef-run/i18n/en.yml @@ -281,6 +281,12 @@ errors: CHEFVAL009: | File extension '%1' is unsupported. Currently recipes must be specified with a `.rb` extension. + CHEFVAL010: | + The flag '%1' does not exist. + + Available flags are: + %2 + # General errors/unknown errors are handled with CHEFINT CHEFINT001: | An unexpected error has occurred: diff --git a/components/chef-run/lib/chef-run/cli.rb b/components/chef-run/lib/chef-run/cli.rb index d91b570e5..271230132 100644 --- a/components/chef-run/lib/chef-run/cli.rb +++ b/components/chef-run/lib/chef-run/cli.rb @@ -200,6 +200,13 @@ def perform_run run_multi_target(initial_status_msg, target_hosts, local_policy_path) end end + rescue OptionParser::InvalidOption => e + # Using nil here is a bit gross but it prevents usage from printing. + ove = OptionValidationError.new("CHEFVAL010", nil, + e.message.split(":")[1].strip, # only want the flag + format_flags.lines[1..-1].join # remove 'FLAGS:' header + ) + handle_perform_error(ove) rescue => e handle_perform_error(e) ensure @@ -475,13 +482,17 @@ def capture_exception_backtrace(e) end def show_help - UI::Terminal.output banner - show_help_flags + UI::Terminal.output format_help end - def show_help_flags - UI::Terminal.output "" - UI::Terminal.output "FLAGS:" + def format_help + help_text = banner.clone # This prevents us appending to the banner text + help_text << "\n" + help_text << format_flags + end + + def format_flags + flag_text = "FLAGS:\n" justify_length = 0 options.each_value do |spec| justify_length = [justify_length, spec[:long].length + 4].max @@ -495,16 +506,16 @@ def show_help_flags short = "#{short}, " end flags = "#{short}#{flag_spec[:long]}" - UI::Terminal.write(" #{flags.ljust(justify_length)} ") + flag_text << " #{flags.ljust(justify_length)} " ml_padding = " " * (justify_length + 8) first = true flag_spec[:description].split("\n").each do |d| - UI::Terminal.write(ml_padding) unless first + flag_text << ml_padding unless first first = false - UI::Terminal.write(d) - UI::Terminal.write("\n") + flag_text << "#{d}\n" end end + flag_text end def usage diff --git a/components/chef-run/spec/unit/cli_spec.rb b/components/chef-run/spec/unit/cli_spec.rb index b30a90a05..40b2bdb07 100644 --- a/components/chef-run/spec/unit/cli_spec.rb +++ b/components/chef-run/spec/unit/cli_spec.rb @@ -38,54 +38,49 @@ allow(subject).to receive(:inspect).and_return("The subject instance") end describe "run" do - # it "t" do - # expect(subject).to receive(:parse_options).with(argv) - # subject.run - # end - - # it "sets up the cli" do - # expect(subject).to receive(:setup_cli) - # expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) } - # end - # - # # TODO - test for Sender.new.run in thread - # it "performs the steps necessary to capture telemetry" do - # expect(telemetry).to receive(:timed_run_capture).and_yield - # expect(telemetry).to receive(:commit) - # expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) } - # end - # - # it "calls perform_run" do - # expect(subject).to receive(:perform_run) - # expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) } - # end - # - # context "perform_run raises WrappedError" do - # let(:e) { ChefRun::WrappedError.new(RuntimeError.new("Test"), "host") } - # - # it "prints the error and exits" do - # expect(subject).to receive(:perform_run).and_raise(e) - # expect(ChefRun::UI::ErrorPrinter).to receive(:show_error).with(e) - # expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(1) } - # end - # end - # - # context "perform_run raises SystemExit" do - # it "exits with same exit code" do - # expect(subject).to receive(:perform_run).and_raise(SystemExit.new(99)) - # expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(99) } - # end - # end - # - # context "perform_run raises any other exception" do - # let(:e) { Exception.new("test") } - # - # it "exits with same exit code" do - # expect(subject).to receive(:perform_run).and_raise(e) - # expect(ChefRun::UI::ErrorPrinter).to receive(:dump_unexpected_error).with(e) - # expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(64) } - # end - # end + it "sets up the cli" do + expect(subject).to receive(:setup_cli) + expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) } + end + + # TODO - test for Sender.new.run in thread + it "performs the steps necessary to capture telemetry" do + expect(telemetry).to receive(:timed_run_capture).and_yield + expect(telemetry).to receive(:commit) + expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) } + end + + it "calls perform_run" do + expect(subject).to receive(:perform_run) + expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) } + end + + context "perform_run raises WrappedError" do + let(:e) { ChefRun::WrappedError.new(RuntimeError.new("Test"), "host") } + + it "prints the error and exits" do + expect(subject).to receive(:perform_run).and_raise(e) + expect(ChefRun::UI::ErrorPrinter).to receive(:show_error).with(e) + expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(1) } + end + end + + context "perform_run raises SystemExit" do + it "exits with same exit code" do + expect(subject).to receive(:perform_run).and_raise(SystemExit.new(99)) + expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(99) } + end + end + + context "perform_run raises any other exception" do + let(:e) { Exception.new("test") } + + it "exits with same exit code" do + expect(subject).to receive(:perform_run).and_raise(e) + expect(ChefRun::UI::ErrorPrinter).to receive(:dump_unexpected_error).with(e) + expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(64) } + end + end end describe "#perform_run" do