From 5412121ddd35de6321329586713581682e040d5f Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 13 Oct 2023 20:04:43 +1100 Subject: [PATCH 1/3] feat: print warnings and allow error to be raised when unknown option is used PACT-1370 --- README.md | 8 ++++ lib/pact_broker/client/cli/custom_thor.rb | 5 ++- .../cli/thor_unknown_options_monkey_patch.rb | 38 ++++++++++++++++++ spec/integration/unknown_options_spec.rb | 39 +++++++++++++++++++ 4 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb create mode 100644 spec/integration/unknown_options_spec.rb diff --git a/README.md b/README.md index 6323cd7..3a2e381 100644 --- a/README.md +++ b/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. diff --git a/lib/pact_broker/client/cli/custom_thor.rb b/lib/pact_broker/client/cli/custom_thor.rb index ecf0021..2b0d38e 100644 --- a/lib/pact_broker/client/cli/custom_thor.rb +++ b/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 diff --git a/lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb b/lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb new file mode 100644 index 0000000..116ca9d --- /dev/null +++ b/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 arguments set PACT_BROKER_ERROR_ON_UNKNOWN_OPTION=true")) + $stderr.puts("\n") + end + end + end +end diff --git a/spec/integration/unknown_options_spec.rb b/spec/integration/unknown_options_spec.rb new file mode 100644 index 0000000..02f146d --- /dev/null +++ b/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" 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 From e47e6bdd3955d1b81b220d64dd81963657a5639e Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 13 Oct 2023 20:07:12 +1100 Subject: [PATCH 2/3] chore: update message --- lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb b/lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb index 116ca9d..bdca9b1 100644 --- a/lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb +++ b/lib/pact_broker/client/cli/thor_unknown_options_monkey_patch.rb @@ -30,7 +30,7 @@ def check_unknown_and_warn 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 arguments set PACT_BROKER_ERROR_ON_UNKNOWN_OPTION=true")) + $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 From 7e2c076c6c291d3752859869cddd59d9856dee32 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 13 Oct 2023 20:15:34 +1100 Subject: [PATCH 3/3] chore: don't try to run specs for unknown options on windows --- spec/integration/unknown_options_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/integration/unknown_options_spec.rb b/spec/integration/unknown_options_spec.rb index 02f146d..79d961c 100644 --- a/spec/integration/unknown_options_spec.rb +++ b/spec/integration/unknown_options_spec.rb @@ -4,7 +4,7 @@ # 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" do +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" }