Show help in a consistent manner

components/chef-workstation/i18n/en.yml

@@ -10,6 +10,8 @@ cli:
 # Text specific to each command
 commands:
   base:
+    version: "Show the current version of this tool"
+    version_for_help: "Chef Workstation Version: %1\n\n"
     description: |
       Congratulations! You are using chef: your gateway
       to managing everything from a single node to an entire Chef
@@ -26,9 +28,7 @@ commands:
       connection_failed: "Connection failed: %1"
 
     help: "Show usage information"
-    version: "Show the current version of this tool"
     alias_for: "Alias for: "
-    version_for_help: "Chef Workstation Version: %1\n\n"
     aliases: "ALIASES:"
     usage_msg: "USAGE: "
 
@@ -104,11 +104,6 @@ commands:
           the remote host is using a self-signed certificate.
           Current default: %1
 
-  base:
-    status:
-      connecting: "Connecting..."
-      connected: "Connected."
-      connection_failed: "Connection failed: %1"
 
 # Status updates shared across commands.
 status:

components/chef-workstation/lib/chef-workstation/cli.rb

@@ -21,6 +21,7 @@
 require "chef-workstation/builtin_commands"
 require "chef-workstation/text"
 require "chef-workstation/error"
+require "chef-workstation/log"
 require "chef-workstation/ui/terminal"
 require "chef-workstation/ui/error_printer"
 
@@ -45,15 +46,15 @@ def run
         perform_command()
       end
 
-    # rescue WrappedError => e
-    #   UI::ErrorPrinter.show_error(e)
-    #   @rc = RC_COMMAND_FAILED
-    # rescue => e
-    #   UI::ErrorPrinter.dump_unexpected_error(e)
-    #   @rc = RC_ERROR_HANDLING_FAILED
-    # ensure
-    #   Telemetry.send!
-    #   exit @rc
+    rescue WrappedError => e
+      UI::ErrorPrinter.show_error(e)
+      @rc = RC_COMMAND_FAILED
+    rescue => e
+      UI::ErrorPrinter.dump_unexpected_error(e)
+      @rc = RC_ERROR_HANDLING_FAILED
+    ensure
+      Telemetry.send!
+      exit @rc
     end
 
     def setup_cli
@@ -81,12 +82,12 @@ def perform_command
       root_command, *leftover = @argv
 
       run_command!(root_command, leftover)
-    # rescue => e
-    #   handle_perform_error(e)
+    rescue => e
+      handle_perform_error(e)
     end
 
     # This converts any use of version as a flag into a 'version' command.
-    #
+    # placed as the first command.
     def update_args_for_version
       version_included = false
       @argv.delete_if do |item|
@@ -106,9 +107,9 @@ def update_args_for_version
     def update_args_for_help
       if @argv.empty?
         @argv << "--help"
+        return
       end
 
-      #
       if @argv[0].casecmp("help") == 0
         # Make help command the last option to the specified command (if any)
         # so that it's handled by the command that is being asked about.
@@ -118,24 +119,25 @@ def update_args_for_help
         @argv.pop
         @argv.push "--help"
       else
-        # h = @argv.delete("--help") || @argv.delete("-h")
-        # @argv << h unless h.nil?
+        #
+        @argv = @argv.map { |arg| arg == "help" ? "--help" : arg }
       end
     end
 
     def run_command!(command_name, command_params)
       if command_name.nil? || %w{-h --help help}.include?(command_name.downcase)
         # hidden-root represents the base "Chef" command which knows how to report
-        # help for that top-level command.
+        # help for that top-level command.  IDeally we'll return to this
+        # and make it represent the actual 'chef' command.
         command_name = "hidden-root"
       end
 
       if have_command?(command_name)
         @cmd, command_params = commands_map.instantiate(command_name, command_params)
         @cmd.run_with_default_options(command_params)
       else
-        Logger.error("Command not found: #{command_name}")
-        raise UnknownCommand.new(command_name, commands.join(" "))
+        ChefWorkstation::Log.error("Command not found: #{command_name}")
+        raise UnknownCommand.new(command_name, available_commands.join(" "))
       end
     end
 
@@ -169,8 +171,8 @@ def have_command?(name)
       commands_map.have_command_or_alias?(name)
     end
 
-    def commands
-      commands_map.command_names
+    def available_commands
+      commands_map.command_names(true)
     end
 
     def command_aliases

components/chef-workstation/lib/chef-workstation/command/base.rb

@@ -36,13 +36,13 @@ class Base
       option :version,
         :short        => "-v",
         :long         => "--version",
-        :description  => Text.commands.base.version,
+        :description  => T.version,
         :boolean      => true
 
       option :help,
         :short        => "-h",
         :long         => "--help",
-        :description  => Text.commands.base.help,
+        :description  => T.help,
         :boolean      => true
 
       option :config_path,
@@ -127,7 +127,7 @@ def show_version
       def show_help
         root_command = @command_spec.qualified_name == "hidden-root"
         if root_command
-          UI::Terminal.output Text.commands.base.version_for_help(ChefWorkstation::VERSION)
+          UI::Terminal.output T.version_for_help(ChefWorkstation::VERSION)
         end
         UI::Terminal.output banner
         show_help_flags unless options.empty?
@@ -172,8 +172,8 @@ def show_help_subcommands
         end
 
         unless help_cmd.nil?
-          UI::Terminal.output "    #{"#{help_cmd.name}".ljust(justify_length)}#{Text.commands.base.help}"
-          UI::Terminal.output "    #{"#{version_cmd.name}".ljust(justify_length)}#{Text.commands.base.help}"
+          UI::Terminal.output "    #{"#{help_cmd.name}".ljust(justify_length)}#{T.help}"
+          UI::Terminal.output "    #{"#{version_cmd.name}".ljust(justify_length)}#{T.help}"
         end
       end
 

components/chef-workstation/lib/chef-workstation/command/target/converge.rb

@@ -69,7 +69,7 @@ def run(params)
           target = cli_arguments.shift
 
           @conn = connect(target, config)
-          UI::Terminal.spinner(TS.install.verifying, prefix: "[#{@conn.hostname}]") do |r|
+          UI::Terminal.spinner(TS.install_chef.verifying, prefix: "[#{@conn.hostname}]") do |r|
             install(r)
           end
 
@@ -87,7 +87,7 @@ def run(params)
         CB_MATCHER = '[\w\-]+'
         def validate_params(params)
           if params.size < 2
-            raise OptionValidationError.new("CHEFVAL002")
+            raise OptionValidationError.new("CHEFVAL002", self)
           end
           if params.size == 2
             # Trying to specify a recipe to run remotely, no properties
@@ -97,13 +97,13 @@ def validate_params(params)
             elsif cb =~ /^#{CB_MATCHER}$/ || cb =~ /^#{CB_MATCHER}::#{CB_MATCHER}$/
               # They are specifying a cookbook as 'cb_name' or 'cb_name::recipe'
             else
-              raise OptionValidationError.new("CHEFVAL004", cb)
+              raise OptionValidationError.new("CHEFVAL004", self, cb)
             end
           elsif params.size >= 3
             properties = params[3..-1]
             properties.each do |property|
               unless property =~ PROPERTY_MATCHER
-                raise OptionValidationError.new("CHEFVAL003", property)
+                raise OptionValidationError.new("CHEFVAL003", self, property)
               end
             end
           end

components/chef-workstation/lib/chef-workstation/commands_map.rb

@@ -162,8 +162,10 @@ def have_command_or_alias?(name)
       command_specs.key?(name) || alias_specs.key?(name)
     end
 
-    def command_names
-      command_specs.keys
+    def command_names(with_hidden = false)
+      return command_specs.keys if with_hidden
+
+      command_specs.select { |k, v| !v.hidden }.keys
     end
 
     private

components/chef-workstation/spec/unit/cli_spec.rb

@@ -31,16 +31,13 @@
 
   context "run" do
     before do
-      expect(subject).to receive(:init)
+      expect(subject).to receive(:setup_cli)
     end
 
     it "performs the steps necessary to handle the request and capture telemetry" do
       expect(subject).to receive(:perform_command)
       expect(telemetry).to receive(:timed_capture).
-        with(:run,
-             command: nil,
-             sub: nil, args: [],
-             opts: cli.options.to_h).and_yield
+        with(:run, args: []).and_yield
       expect(telemetry).to receive(:send!)
       expect { cli.run }.to raise_error SystemExit
     end
@@ -79,31 +76,33 @@
       end
     end
 
-    context "help command called on a subcommand" do
-      let(:argv) { %w{help config} }
-      it "passes the help message to the subcommand" do
-        expect(cli).to receive(:have_command?).with("config").and_return(true)
-
-        expect_any_instance_of(ChefWorkstation::Command::Base).to receive(:run_with_default_options).with(["-h"]).and_return(0)
-        expect { cli.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
+    context "help handling" do
+      context "help command called on a subcommand" do
+        let(:argv) { %w{help config} }
+        it "passes the help message to the subcommand" do
+          expect(cli).to receive(:have_command?).with("config").and_return(true)
+          expect_any_instance_of(ChefWorkstation::Command::Base).to receive(:run_with_default_options).with(["--help"]).and_return(0)
+          expect { cli.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
+        end
       end
-    end
 
-    context "help command called with no args" do
-      let(:expected_version_string) { ChefWorkstation::Text.cli.print_version(ChefWorkstation::VERSION) }
-      %w{-h --help help}.each do |flag|
-        let(:argv) { [flag] }
-        it "shows version and top-level help, and exits 0" do
-          expect(ChefWorkstation::UI::Terminal).to receive(:output).with(expected_version_string)
-          expect(subject).to receive(:show_help)
-          expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
+      context "help command called with no args" do
+        let(:expected_version_string) { ChefWorkstation::Text.commands.base.version_for_help(ChefWorkstation::VERSION) }
+        %w{-h --help help}.each do |flag|
+          let(:argv) { [flag] }
+          it "shows version and top-level help, and exits 0" do
+            expect(ChefWorkstation::UI::Terminal).to receive(:output).with(expected_version_string)
+            expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
+          end
         end
       end
     end
+
   end
 
   context "#perform_command" do
     context "help command called" do
+
       let(:argv) { ["help"] }
       it "prints the help text" do
         expect { cli.perform_command }.to output(/Congratulations!.+-c, --config PATH/m).to_terminal
@@ -126,7 +125,9 @@
     context "when an exception occurs" do
       let(:err) { "A String exception" }
       it "handles it" do
-        allow(cli).to receive(:show_help).and_raise err
+        # and hijack the first call perform_command makes
+        # to force an error.
+        allow(cli).to receive(:update_args_for_help).and_raise err
         expect(cli).to receive(:handle_perform_error)
         cli.perform_command
       end

components/chef-workstation/spec/unit/command/base_spec.rb

@@ -21,18 +21,39 @@
 RSpec.describe ChefWorkstation::Command::Base do
   let(:cmd_spec) { instance_double(ChefWorkstation::CommandsMap::CommandSpec, name: "cmd", subcommands: []) }
   subject(:cmd) do
+    allow(cmd_spec).to receive(:qualified_name).and_return "blah"
     ChefWorkstation::Command::Base.new(cmd_spec)
   end
 
   describe "run" do
-    it "raises an NotImplementedError" do
-      expect { cmd.run([]) }.to raise_error(NotImplementedError)
+    it "shows help" do
+      expect(subject).to receive(:show_help)
+      subject.run([])
     end
   end
 
   describe "run_with_default_options" do
-    it "prints the help text" do
-      expect { cmd.run_with_default_options(["help"]) }.to output(/Command banner not set.+-c, --config PATH/m).to_terminal
+    context "with no arguments" do
+      it "invokes show_help" do
+        expect(subject).to receive(:show_help)
+        subject.run_with_default_options([])
+      end
+    end
+    context "with help arguments" do
+      %w{--help -h}.each do |arg|
+        it "shows help when run with #{arg}" do
+          expect(subject).to receive(:show_help)
+          subject.run_with_default_options([arg])
+        end
+      end
+    end
+    context "with version arguments" do
+      %w{--version -v}.each do |arg|
+        it "shows version when run with #{arg}" do
+          expect(subject).to receive(:show_version)
+          subject.run_with_default_options([arg])
+        end
+      end
     end
   end
 end

components/chef-workstation/spec/unit/command/target/converge_spec.rb

@@ -19,7 +19,7 @@
 require "chef-workstation/command/target/converge"
 
 RSpec.describe ChefWorkstation::Command::Target::Converge do
-  let(:cmd_spec) { instance_double(ChefWorkstation::CommandsMap::CommandSpec) }
+  let(:cmd_spec) { ChefWorkstation::CommandsMap::CommandSpec.new }
   subject(:cmd) do
     ChefWorkstation::Command::Target::Converge.new(cmd_spec)
   end
@@ -142,7 +142,7 @@
       expect(cmd).to receive(:cli_arguments).and_return(params).exactly(3).times
       expect(cmd).to receive(:validate_params).with(params)
       expect(cmd).to receive(:connect).with("target", an_instance_of(Hash)).and_return(conn)
-      msg = ChefWorkstation::Text.status.install.verifying
+      msg = ChefWorkstation::Text.status.install_chef.verifying
       expect(ChefWorkstation::UI::Terminal).to receive(:spinner).with(msg, { prefix: "[target]" }).and_yield(reporter)
       expect(cmd).to receive(:install).with(reporter)
       msg = "other_msg"

components/chef-workstation/spec/unit/commands_map_spec.rb

@@ -20,7 +20,7 @@
 
 RSpec.describe ChefWorkstation::CommandsMap do
   subject(:mapping) { ChefWorkstation::CommandsMap.new }
-  let(:example_text) { double("text", description: "description", usage: "USAGE:\n") }
+  let(:example_text) { double("text", description: "description", usage_full: "Full usage", usage: "short usage:\n") }
   let(:example_cmd) { mapping.top_level("example", :TestCommand, example_text, "unit/fixtures/command/cli_test_command") }
   let(:subcmd1) { mapping.create("subcommand1", [:TopLevel, :Subcommand], example_text, "unit/fixtures/command/cli_test_command") }
   let(:subcmd2) { mapping.create("subcommand2", :AliasedCommand, example_text, "unit/fixtures/command/cli_test_command", cmd_alias: "subby") }
@@ -37,7 +37,7 @@
     expect(mapping.have_command_or_alias?("example")).to be true
     e = mapping.command_specs["example"]
     expect(e.require_path).to eq("unit/fixtures/command/cli_test_command")
-    expect(e.make_banner).to eq("description\n\nUSAGE:\n")
+    expect(e.make_banner).to eq("description\nFull usage")
   end
 
   it "lists the available commands" do
@@ -58,7 +58,6 @@
   end
 
   it "assigns qualified names to commands correctly" do
-    ChefWorkstation.assign_parentage!(mapping.command_specs)
     expect(subcmd1.qualified_name).to eq "top-level subcommand1"
     expect(subcmd2.qualified_name).to eq "top-level subcommand2"
     expect(example_cmd.qualified_name).to eq "example"