Merge pull request #151 from chef/JM/fix_invalid_flag_handling

Fixes error handling when an invalid flag is passed.

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:

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

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:

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

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