Merge pull request #147 from pact-foundation/feat/error-on-unknown-op…

…tion

feat: print warnings and allow error to be raised when unknown option is used

README.md

@@ -42,6 +42,14 @@ Basic auth parameters can be specified using the `$PACT_BROKER_USERNAME` and `$P
 
 Authentication using a bearer token can be specified using the environment variable `$PACT_BROKER_TOKEN` or the `-k` or `--broker-token` parameters. This bearer token authentication is used by [PactFlow](https://pactflow.io) and is not available in the [OSS Pact Broker](https://docs.pact.io/pact_broker/), which only supports basic auth.
 
+### Handling unknown options
+
+By default, the underlying library used for the Pact Broker CLI (Thor) does not raise an error in every situation where there is an unknown option used.
+This can allow invalid commands to be executed that do not perform as expected.
+
+From version 1.73.0, warnings will be printed for unknown options, and if the environment variable `PACT_BROKER_ERROR_ON_UNKNOWN_OPTION=true` is set, an error will be raised when an unknown option is used.
+
+In the next major version, an error will be raised by default.
 
 <!-- start-autogenerated-docs -->
 

lib/pact_broker/client/cli/custom_thor.rb

@@ -1,21 +1,24 @@
 require 'thor'
+require 'pact_broker/client/cli/thor_unknown_options_monkey_patch'
 require 'pact_broker/client/hash_refinements'
 
 module PactBroker
   module Client
     module CLI
       class AuthError < ::Thor::Error; end
-
       ##
       # Custom Thor task allows the following:
       #
       # `--option 1 --option 2` to be interpreted as `--option 1 2` (the standard Thor format for multiple value options)
       #
+
       class CustomThor < ::Thor
         using PactBroker::Client::HashRefinements
 
         EM_DASH = "\u2014"
 
+        check_unknown_options!
+
         no_commands do
           def self.exit_on_failure?
             true

lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb

@@ -0,0 +1,38 @@
+require "thor"
+require "term/ansicolor"
+
+# Monkey patch Thor so we can print out a warning when there are unknown options, rather than raising an error.
+# If PACT_BROKER_ERROR_ON_UNKNOWN_OPTION=true, raise the error, as the user will have opted in to this behaviour.
+# This is for backwards compatibility reasons, and in the next major version, the flag will be removed.
+
+class Thor
+  class Options < Arguments
+
+    alias_method :original_check_unknown!, :check_unknown!
+
+    # Replace the original check_unknown! method with an implementation
+    # that will print a warning rather than raising an error
+    # unless PACT_BROKER_ERROR_ON_UNKNOWN_OPTION=true is set.
+    def check_unknown!
+      if raise_error_on_unknown_options?
+        original_check_unknown!
+      else
+        check_unknown_and_warn
+      end
+    end
+
+    def raise_error_on_unknown_options?
+      ENV["PACT_BROKER_ERROR_ON_UNKNOWN_OPTION"] == "true"
+    end
+
+    def check_unknown_and_warn
+      begin
+        original_check_unknown!
+      rescue UnknownArgumentError => e
+        $stderr.puts(::Term::ANSIColor.yellow(e.message))
+        $stderr.puts(::Term::ANSIColor.yellow("This is a warning rather than an error so as not to break backwards compatibility. To raise an error for unknown options set PACT_BROKER_ERROR_ON_UNKNOWN_OPTION=true"))
+        $stderr.puts("\n")
+      end
+    end
+  end
+end

spec/integration/unknown_options_spec.rb

@@ -0,0 +1,39 @@
+require "open3"
+require "pact_broker/client/cli/broker"
+
+# This is not the ideal way to write a test, but I tried to write it with an in memory invocation,
+# and I couldn't get the capture to work, and it became super complicated.
+
+RSpec.describe "using unknown options", skip_windows: true do
+  let(:unknown_switches_text) { "Unknown switches" }
+  let(:warning_text) { "This is a warning"}
+  let(:command) { "bundle exec bin/pact-broker can-i-deploy --pacticipant Foo --foo --broker-base-url http://example.org" }
+
+  it "prints an 'unknown switches' warning to stderr and also includes the normal output of the command" do
+    stderr_lines = nil
+
+    Open3.popen3(command) do |stdin, stdout, stderr, thread|
+       stderr_lines = stderr.readlines
+    end
+
+    expect(stderr_lines.join("\n")).to include(unknown_switches_text)
+    expect(stderr_lines.join("\n")).to include(warning_text)
+
+    expect(stderr_lines.size).to be > 2
+  end
+
+
+  context "with PACT_BROKER_ERROR_ON_UNKNOWN_OPTION=true" do
+    it "prints an 'unknown switches' message to stderr and does NOT include the normal output of the command as it exits straight after" do
+      stderr_lines = nil
+
+      Open3.popen3({ "PACT_BROKER_ERROR_ON_UNKNOWN_OPTION" => "true" }, command) do |stdin, stdout, stderr, thread|
+         stderr_lines = stderr.readlines
+      end
+
+      expect(stderr_lines.first).to include(unknown_switches_text)
+      expect(stderr_lines.join("\n")).to_not include(warning_text)
+      expect(stderr_lines.size).to eq 1
+    end
+  end
+end