Make validate_execution_environment/2 use a different tuple tag for failures

Per discussion with @hairyhum.

deps/rabbitmq_cli/lib/rabbitmq/cli/core/validators.ex

@@ -32,6 +32,18 @@ defmodule RabbitMQ.CLI.Core.Validators do
   def chain([], _) do
     :ok
   end
+  def chain([validator | rest], args, error_key) do
+    case apply(validator, args) do
+      :ok                        -> chain(rest, args, error_key)
+      {:ok, _}                   -> chain(rest, args, error_key)
+      {:validation_failure, err} -> {error_key, err}
+      {:error, err}              -> {error_key, err}
+    end
+  end
+  def chain([], _, _) do
+    :ok
+  end
+
 
   def node_is_not_running(_, %{node: node_name}) do
     case Helpers.node_running?(node_name) do
@@ -67,7 +79,7 @@ defmodule RabbitMQ.CLI.Core.Validators do
   end
   def rabbit_is_running_or_offline_flag_used(args, opts) do
     rabbit_is_running(args, opts)
-  end  
+  end
 
   def rabbit_is_not_running(args, opts) do
     case rabbit_app_state(args, opts) do

deps/rabbitmq_cli/lib/rabbitmq/cli/plugins/commands/disable_command.ex

@@ -50,7 +50,8 @@ defmodule RabbitMQ.CLI.Plugins.Commands.DisableCommand do
                       &Helpers.require_rabbit_and_plugins/2,
                       &PluginHelpers.enabled_plugins_file/2,
                       &Helpers.plugins_dir/2],
-                     [args, opts])
+                     [args, opts],
+                     :environment_validation_failure)
   end
 
   def usage, do: "disable <plugin>|--all [--offline] [--online]"

deps/rabbitmq_cli/lib/rabbitmq/cli/plugins/commands/enable_command.ex

@@ -50,7 +50,8 @@ defmodule RabbitMQ.CLI.Plugins.Commands.EnableCommand do
                       &Helpers.require_rabbit_and_plugins/2,
                       &PluginHelpers.enabled_plugins_file/2,
                       &Helpers.plugins_dir/2],
-                     [args, opts])
+                     [args, opts],
+                     :environment_validation_failure)
   end
 
   def usage, do: "enable <plugin>|--all [--offline] [--online]"

deps/rabbitmq_cli/lib/rabbitmq/cli/plugins/commands/list_command.ex

@@ -55,7 +55,8 @@ defmodule RabbitMQ.CLI.Plugins.Commands.ListCommand do
                       &Helpers.require_rabbit_and_plugins/2,
                       &PluginHelpers.enabled_plugins_file/2,
                       &Helpers.plugins_dir/2],
-                     [args, opts])
+                     [args, opts],
+                     :environment_validation_failure)
   end
 
   def usage, do: "list [pattern] [--verbose] [--minimal] [--enabled] [--implicitly-enabled]"

deps/rabbitmq_cli/lib/rabbitmq/cli/plugins/commands/set_command.ex

@@ -43,7 +43,8 @@ defmodule RabbitMQ.CLI.Plugins.Commands.SetCommand do
                       &Helpers.require_rabbit_and_plugins/2,
                       &PluginHelpers.enabled_plugins_file/2,
                       &Helpers.plugins_dir/2],
-                     [args, opts])
+                     [args, opts],
+                     :environment_validation_failure)
   end
 
   def usage, do: "set [<plugin>] [--offline] [--online]"

deps/rabbitmq_cli/lib/rabbitmqctl.ex

@@ -62,8 +62,8 @@ defmodule RabbitMQCtl do
         {:error, ExitCodes.exit_usage, suggest_message};
       {_, [_|_]} ->
 
-        validation_error({:bad_option, invalid}, command,
+        argument_validation_error_output({:bad_option, invalid}, command,
-                         unparsed_command, parsed_options);
+          unparsed_command, parsed_options);
       _ ->
         options = parsed_options |> merge_all_defaults |> normalize_options
         {arguments, options} = command.merge_defaults(arguments, options)
@@ -120,7 +120,9 @@ defmodule RabbitMQCtl do
       {:error, _} = err ->
         format_error(err, options, command);
       {:validation_failure, err} ->
-        validation_error(err, command, unparsed_command, options);
+        argument_validation_error_output(err, command, unparsed_command, options);
+      {:environment_validation_failure, err} ->
+        environment_validation_error_output(err, command, unparsed_command, options);
       _  ->
         output_fun.(output, command, options)
     end
@@ -250,9 +252,19 @@ defmodule RabbitMQCtl do
     end
   end
 
-  defp validation_error(err_detail, command, unparsed_command, options) do
+  defp argument_validation_error_output(err_detail, command, unparsed_command, options) do
-    err = format_validation_error(err_detail) # TODO format the error better
+    err = format_validation_error(err_detail)
-    base_error = "Error: #{err}\nGiven:\n\t#{unparsed_command |> Enum.join(" ")}"
+    base_error = "Error (argument validation): #{err}\nArguments given:\n\t#{unparsed_command |> Enum.join(" ")}"
+    validation_error_output(err_detail, base_error, command, options)
+  end
+
+  defp environment_validation_error_output(err_detail, command, unparsed_command, options) do
+    err = format_validation_error(err_detail)
+    base_error = "Error: #{err}\nArguments given:\n\t#{unparsed_command |> Enum.join(" ")}"
+    validation_error_output(err_detail, base_error, command, options)
+  end
+
+  defp validation_error_output(err_detail, base_error, command, options) do
     usage = HelpCommand.base_usage(command, options)
     message = base_error <> "\n" <> usage
     {:error, ExitCodes.exit_code_for({:validation_failure, err_detail}), message}
@@ -268,6 +280,9 @@ defmodule RabbitMQCtl do
     header = "Invalid options for this command:"
     Enum.join([header | for {key, val} <- opts do "#{key} : #{val}" end], "\n")
   end
+  defp format_validation_error({:bad_info_key, keys}), do: "Info key(s) #{Enum.join(keys, ",")} are not supported"
+  defp format_validation_error(:rabbit_app_is_stopped), do: "this command requires the 'rabbit' app to be running on the target node. Start it with rabbitmqctl start_app."
+  defp format_validation_error(:rabbit_app_is_running), do: "this command requires the 'rabbit' app to be stopped on the target node. Stop it with rabbitmqctl stop_app."
   defp format_validation_error(err), do: inspect err
 
   defp exit_program(code) do

deps/rabbitmq_cli/test/plugins/disable_plugins_command_test.exs

@@ -43,19 +43,16 @@ defmodule DisablePluginsCommandTest do
       set_enabled_plugins(enabled_plugins, :online, get_rabbit_hostname(), opts)
     end)
 
-
     {:ok, opts: opts}
   end
 
   setup context do
-
     set_enabled_plugins([:rabbitmq_stomp, :rabbitmq_federation],
                         :online,
                         get_rabbit_hostname(),
                         context[:opts])
 
 
-
     {
       :ok,
       opts: Map.merge(context[:opts], %{
@@ -81,12 +78,12 @@ defmodule DisablePluginsCommandTest do
 
   test "validate_execution_environment: not specifying an enabled_plugins_file is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.delete(context[:opts], :enabled_plugins_file)) ==
-      {:validation_failure, :no_plugins_file}
+      {:environment_validation_failure, :no_plugins_file}
   end
 
   test "validate_execution_environment: not specifying a plugins_dir is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.delete(context[:opts], :plugins_dir)) ==
-      {:validation_failure, :no_plugins_dir}
+      {:environment_validation_failure, :no_plugins_dir}
   end
 
 
@@ -96,11 +93,11 @@ defmodule DisablePluginsCommandTest do
 
   test "validate_execution_environment: specifying a non-existent plugins_dir is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.merge(context[:opts], %{plugins_dir: "none"})) ==
-      {:validation_failure, :plugins_dir_does_not_exist}
+      {:environment_validation_failure, :plugins_dir_does_not_exist}
   end
 
   test "validate_execution_environment: failure to load the rabbit application is reported as an error", context do
-    assert {:validation_failure, {:unable_to_load_rabbit, _}} =
+    assert {:environment_validation_failure, {:unable_to_load_rabbit, _}} =
       @command.validate_execution_environment(["a"], Map.delete(context[:opts], :rabbitmq_home))
   end
 

deps/rabbitmq_cli/test/plugins/enable_plugins_command_test.exs

@@ -78,12 +78,12 @@ defmodule EnablePluginsCommandTest do
 
   test "validate_execution_environment: not specifying an enabled_plugins_file is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.delete(context[:opts], :enabled_plugins_file)) ==
-      {:validation_failure, :no_plugins_file}
+      {:environment_validation_failure, :no_plugins_file}
   end
 
   test "validate_execution_environment: not specifying a plugins_dir is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.delete(context[:opts], :plugins_dir)) ==
-      {:validation_failure, :no_plugins_dir}
+      {:environment_validation_failure, :no_plugins_dir}
   end
 
 
@@ -93,11 +93,11 @@ defmodule EnablePluginsCommandTest do
 
   test "validate_execution_environment: specifying a non-existent plugins_dir is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.merge(context[:opts], %{plugins_dir: "none"})) ==
-      {:validation_failure, :plugins_dir_does_not_exist}
+      {:environment_validation_failure, :plugins_dir_does_not_exist}
   end
 
   test "validate: failure to load the rabbit application is reported as an error", context do
-    assert {:validation_failure, {:unable_to_load_rabbit, _}} =
+    assert {:environment_validation_failure, {:unable_to_load_rabbit, _}} =
       @command.validate_execution_environment(["a"], Map.delete(context[:opts], :rabbitmq_home))
   end
 

deps/rabbitmq_cli/test/plugins/list_plugins_command_test.exs

@@ -82,12 +82,12 @@ defmodule ListPluginsCommandTest do
 
   test "validate_execution_environment: not specifying enabled_plugins_file is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.delete(context[:opts], :enabled_plugins_file)) ==
-      {:validation_failure, :no_plugins_file}
+      {:environment_validation_failure, :no_plugins_file}
   end
 
   test "validate_execution_environment: not specifying plugins_dir is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.delete(context[:opts], :plugins_dir)) ==
-      {:validation_failure, :no_plugins_dir}
+      {:environment_validation_failure, :no_plugins_dir}
   end
 
 
@@ -97,11 +97,11 @@ defmodule ListPluginsCommandTest do
 
   test "validate_execution_environment: specifying non existent plugins_dir is reported as an error", context do
     assert @command.validate_execution_environment(["a"], Map.merge(context[:opts], %{plugins_dir: "none"})) ==
-      {:validation_failure, :plugins_dir_does_not_exist}
+      {:environment_validation_failure, :plugins_dir_does_not_exist}
   end
 
   test "validate_execution_environment: failure to load rabbit application is reported as an error", context do
-    assert {:validation_failure, {:unable_to_load_rabbit, _}} =
+    assert {:environment_validation_failure, {:unable_to_load_rabbit, _}} =
       @command.validate_execution_environment(["a"], Map.delete(context[:opts], :rabbitmq_home))
   end
 

deps/rabbitmq_cli/test/plugins/set_plugins_command_test.exs

@@ -71,12 +71,12 @@ defmodule SetPluginsCommandTest do
 
   test "validate_execution_environment: not specifying enabled_plugins_file is reported as an error", context do
     assert @command.validate_execution_environment([], Map.delete(context[:opts], :enabled_plugins_file)) ==
-      {:validation_failure, :no_plugins_file}
+      {:environment_validation_failure, :no_plugins_file}
   end
 
   test "validate_execution_environment: not specifying plugins_dir is reported as an error", context do
     assert @command.validate_execution_environment([], Map.delete(context[:opts], :plugins_dir)) ==
-      {:validation_failure, :no_plugins_dir}
+      {:environment_validation_failure, :no_plugins_dir}
   end
 
   test "validate_execution_environment: specifying a non-existent enabled_plugins_file is fine", context do
@@ -86,11 +86,11 @@ defmodule SetPluginsCommandTest do
 
   test "validate_execution_environment: specifying non existent plugins_dir is reported as an error", context do
     assert @command.validate_execution_environment([], Map.merge(context[:opts], %{plugins_dir: "none"})) ==
-      {:validation_failure, :plugins_dir_does_not_exist}
+      {:environment_validation_failure, :plugins_dir_does_not_exist}
   end
 
   test "validate_execution_environment: failure to load rabbit application is reported as an error", context do
-    assert {:validation_failure, {:unable_to_load_rabbit, _}} =
+    assert {:environment_validation_failure, {:unable_to_load_rabbit, _}} =
       @command.validate_execution_environment([], Map.delete(context[:opts], :rabbitmq_home))
   end