feat: add warnings to can-i-deploy response when bad practice selecto…

…rs are used

lib/pact_broker/api/decorators/reason_decorator.rb

@@ -44,6 +44,10 @@ def reason_text
             "WARN: Although the verification was reported as successful, the results for #{reason.consumer_selector.description} and #{reason.provider_selector.description} may be missing tests for the following interactions: #{descriptions}"
           when PactBroker::Matrix::IgnoreSelectorDoesNotExist
             "WARN: Cannot ignore #{reason.selector.description}"
+          when PactBroker::Matrix::SelectorWithoutPacticipantVersionNumberSpecified
+            "WARN: For production use of can-i-deploy, it is recommended to specify the version number (rather than latest tag or branch) of each pacticipant to avoid race conditions."
+          when PactBroker::Matrix::NoEnvironmentSpecified
+            "WARN: For production use of can-i-deploy, it is recommended to specify the environment into which you are deploying. Without the environment, this result will not be reliable."
           else
             reason
           end

lib/pact_broker/matrix/deployment_status_summary.rb

@@ -55,10 +55,44 @@ def error_messages
       end
 
       def warning_messages
-        resolved_ignore_selectors.select(&:pacticipant_or_version_does_not_exist?).collect { | s | IgnoreSelectorDoesNotExist.new(s) }
-          # ignored_rows.select{ | row | row.success.nil? }.collect{ |row | IgnoredReason.new(pact_not_ever_verified_by_provider(row)) } +
-          # specified_selectors_that_do_not_exist.select(&:ignore?).collect { | selector | IgnoredReason.new(SpecifiedVersionDoesNotExist.new(selector)) } +
-          # ignored_rows.select{ |row| row.success == false }.collect { | row | IgnoredReason.new(VerificationFailed.new(*selectors_for(row))) }
+        resolved_ignore_selectors.select(&:pacticipant_or_version_does_not_exist?).collect { | s | IgnoreSelectorDoesNotExist.new(s) } +
+          bad_practice_warnings
+      end
+
+      def bad_practice_warnings
+        warnings = []
+
+        if no_to_tag_or_environment_specified?
+          warnings << NoEnvironmentSpecified.new
+        end
+
+        if selector_without_pacticipant_version_number_specified?
+          warnings << SelectorWithoutPacticipantVersionNumberSpecified.new
+        end
+
+        warnings
+      end
+
+      def selector_without_pacticipant_version_number_specified?
+        # If only the pacticipant name is specified, it can't be a can-i-deploy query, must be a matrix UI query
+        resolved_selectors
+          .select(&:specified?)
+          .reject(&:only_pacticipant_name_specified?)
+          .reject(&:pacticipant_version_specified_in_original_selector?)
+          .any?
+      end
+
+
+      def more_than_one_selector_specified?
+        # If only the pacticipant name is specified, it can't be a can-i-deploy query, must be a matrix UI query
+        resolved_selectors
+          .select(&:specified?)
+          .reject(&:only_pacticipant_name_specified?)
+          .any?
+      end
+
+      def no_to_tag_or_environment_specified?
+        !(query_results.options[:tag] || query_results.options[:environment_name])
       end
 
       def considered_specified_selectors_that_do_not_exist
@@ -177,8 +211,8 @@ def create_dummy_selectors
 
       def dummy_selectors_from_integrations
         integrations.collect do | row |
-          dummy_consumer_selector = ResolvedSelector.for_pacticipant(row.consumer, :inferred, false)
-          dummy_provider_selector = ResolvedSelector.for_pacticipant(row.provider, :inferred, false)
+          dummy_consumer_selector = ResolvedSelector.for_pacticipant(row.consumer, {}, :inferred, false)
+          dummy_provider_selector = ResolvedSelector.for_pacticipant(row.provider, {}, :inferred, false)
           [dummy_consumer_selector, dummy_provider_selector]
         end.flatten
       end
@@ -188,7 +222,7 @@ def dummy_selectors_from_considered_rows
           dummy_consumer_selector = ResolvedSelector.for_pacticipant_and_version(row.consumer, row.consumer_version, {}, :inferred, false)
           dummy_provider_selector = row.provider_version ?
             ResolvedSelector.for_pacticipant_and_version(row.provider, row.provider_version, {}, :inferred, false) :
-            ResolvedSelector.for_pacticipant(row.provider, :inferred, false)
+            ResolvedSelector.for_pacticipant(row.provider, {}, :inferred, false)
           [dummy_consumer_selector, dummy_provider_selector]
         end.flatten
       end

lib/pact_broker/matrix/reason.rb

@@ -107,6 +107,10 @@ class Warning < Reason
       def selectors
         raise NotImplementedError
       end
+
+      def type
+        :warning
+      end
     end
 
     class IgnoreSelectorDoesNotExist < Warning
@@ -127,9 +131,17 @@ def selectors
       def to_s
         "#{self.class} selector=#{selector}"
       end
+    end
 
-      def type
-        :warning
+    class NoEnvironmentSpecified < Warning
+      def selectors
+        []
+      end
+    end
+
+    class SelectorWithoutPacticipantVersionNumberSpecified < Warning
+      def selectors
+        []
       end
     end
 

lib/pact_broker/matrix/repository.rb

@@ -198,7 +198,7 @@ def build_resolved_selectors(pacticipant, versions, unresolved_selector, selecto
             end
           end
         else
-          selector_for_all_versions_of_a_pacticipant(pacticipant, selector_type, resolved_ignore_selectors)
+          selector_for_all_versions_of_a_pacticipant(pacticipant, unresolved_selector, selector_type, resolved_ignore_selectors)
         end
       end
 
@@ -243,13 +243,13 @@ def selector_for_found_version(pacticipant, version, unresolved_selector, select
       end
       # rubocop: enable Metrics/ParameterLists
 
-      def selector_for_all_versions_of_a_pacticipant(pacticipant, selector_type, resolved_ignore_selectors)
+      def selector_for_all_versions_of_a_pacticipant(pacticipant, unresolved_selector, selector_type, resolved_ignore_selectors)
         # Doesn't make sense to ignore this, as you can't have a can-i-deploy query
         # for "all versions of a pacticipant". But whatever.
         ignore = resolved_ignore_selectors.any? do | s |
           s.pacticipant_id == pacticipant.id && s.only_pacticipant_name_specified?
         end
-        ResolvedSelector.for_pacticipant(pacticipant, selector_type, ignore)
+        ResolvedSelector.for_pacticipant(pacticipant, unresolved_selector, selector_type, ignore)
       end
 
       # only relevant for ignore selectors, validation stops this happening for the normal

lib/pact_broker/matrix/resolved_selector.rb

@@ -16,12 +16,13 @@ def initialize(params = {})
         merge!(params)
       end
 
-      def self.for_pacticipant(pacticipant, type, ignore)
+      def self.for_pacticipant(pacticipant, original_selector, type, ignore)
         ResolvedSelector.new(
           pacticipant_id: pacticipant.id,
           pacticipant_name: pacticipant.name,
           type: type,
-          ignore: ignore
+          ignore: ignore,
+          original_selector: original_selector
         )
       end
 
@@ -30,7 +31,8 @@ def self.for_non_existing_pacticipant(original_selector, type, ignore)
           pacticipant_id: NULL_PACTICIPANT_ID,
           pacticipant_name: original_selector[:pacticipant_name],
           type: type,
-          ignore: ignore
+          ignore: ignore,
+          original_selector: original_selector
         )
       end
 
@@ -47,7 +49,8 @@ def self.for_pacticipant_and_version(pacticipant, version, original_selector, ty
           environment_name: original_selector[:environment_name],
           type: type,
           ignore: ignore,
-          one_of_many: one_of_many
+          one_of_many: one_of_many,
+          original_selector: original_selector
         )
       end
       # rubocop: enable Metrics/ParameterLists
@@ -63,10 +66,15 @@ def self.for_pacticipant_and_non_existing_version(pacticipant, original_selector
           branch: original_selector[:branch],
           environment_name: original_selector[:environment_name],
           type: type,
-          ignore: ignore
+          ignore: ignore,
+          original_selector: original_selector
         )
       end
 
+      def pacticipant_version_specified_in_original_selector?
+        !!self.dig(:original_selector, :pacticipant_version_number)
+      end
+
       def pacticipant_id
         self[:pacticipant_id]
       end
@@ -108,7 +116,7 @@ def most_specific_criterion
       end
 
       def only_pacticipant_name_specified?
-        pacticipant_name && !tag && !latest? && !pacticipant_version_number
+        !!pacticipant_name && !branch && !tag && !latest? && !pacticipant_version_number
       end
 
       def latest_tagged?

spec/lib/pact_broker/api/decorators/reason_decorator_spec.rb

@@ -8,9 +8,9 @@ module Decorators
         REASON_CLASSES = ObjectSpace.each_object(Class).select { |klass| klass < PactBroker::Matrix::Reason && klass.name&.start_with?("PactBroker") }
 
         describe "the number of Reason classes" do
-          let(:expected_number_of_reason_classes) { 12 }
+          let(:expected_number_of_reason_classes) { 14 }
 
-          it "is 12 - this is a reminder to add another spec here if a new Reason is added" do
+          it "is 14 - this is a reminder to add another spec here if a new Reason is added" do
             expect(REASON_CLASSES.size).to eq expected_number_of_reason_classes
           end
         end
@@ -76,6 +76,18 @@ module Decorators
 
             its(:to_s) { is_expected.to eq "WARN: Although the verification was reported as successful, the results for version 2 of Foo and version 6 of Bar may be missing tests for the following interactions: desc1 given p2; desc1 given desc3, desc4" }
           end
+
+          context "when the reason is NoEnvironmentSpecified" do
+            let(:reason) { PactBroker::Matrix::NoEnvironmentSpecified.new }
+
+            its(:to_s) { is_expected.to start_with "WARN: For production use of can-i-deploy, it is recommended to specify the environment" }
+          end
+
+          context "when the reason is SelectorWithoutPacticipantVersionNumberSpecified" do
+            let(:reason) { PactBroker::Matrix::SelectorWithoutPacticipantVersionNumberSpecified.new }
+
+            its(:to_s) { is_expected.to start_with "WARN: For production use of can-i-deploy, it is recommended to specify the version number" }
+          end
         end
       end
     end

spec/lib/pact_broker/matrix/deployment_status_summary_spec.rb

@@ -105,10 +105,13 @@ module Matrix
             resolved_selectors: resolved_selectors,
             resolved_ignore_selectors: resolved_ignore_selectors,
             integrations: integrations,
-            rows: rows + ignored_rows
+            rows: rows + ignored_rows,
+            options: options
           )
         end
 
+        let(:options) { { environment_name: "prod" } }
+
         subject { DeploymentStatusSummary.new(query_results) }
 
         context "when there is a row for all integrations" do
@@ -248,7 +251,8 @@ module Matrix
               environment_name: nil,
               type: :inferred,
               ignore: false,
-              one_of_many: false
+              one_of_many: false,
+              original_selector: {}
             )
           end
 
@@ -267,6 +271,29 @@ module Matrix
 
           its(:reasons) { is_expected.to eq [IgnoreSelectorDoesNotExist.new(resolved_ignore_selectors.first), PactBroker::Matrix::Successful.new] }
         end
+
+        context "when there is a selector without a specified pacticipant version number" do
+          let(:resolved_selectors) do
+            [
+              ResolvedSelector.new(
+                pacticipant_id: foo.id,
+                pacticipant_name: foo.name,
+                pacticipant_version_number: foo_version.number,
+                pacticipant_version_id: foo_version.id,
+                type: :specified,
+                original_selector: { latest: true, tag: "asdf" }
+              )
+            ]
+          end
+
+          its(:reasons) { is_expected.to include SelectorWithoutPacticipantVersionNumberSpecified.new }
+        end
+
+        context "when there is no to tag or environment specified" do
+          let(:options) { { } }
+
+          its(:reasons) { is_expected.to include NoEnvironmentSpecified.new }
+        end
       end
     end
   end

spec/lib/pact_broker/matrix/every_row_spec.rb

@@ -19,11 +19,11 @@ module Matrix
         end
 
         let(:selector_1) do
-          PactBroker::Matrix::ResolvedSelector.for_pacticipant(foo, :specified, false)
+          PactBroker::Matrix::ResolvedSelector.for_pacticipant(foo, {}, :specified, false)
         end
 
         let(:selector_2) do
-          PactBroker::Matrix::ResolvedSelector.for_pacticipant(bar, :specified, false)
+          PactBroker::Matrix::ResolvedSelector.for_pacticipant(bar, {}, :specified, false)
         end
 
         let(:selectors) { [selector_1, selector_2] }

spec/lib/pact_broker/matrix/integration_spec.rb

@@ -380,8 +380,8 @@ module Matrix
             end
 
             it "is not deployable because of the missing verification for Cat v20" do
-              expect(subject.deployment_status_summary.reasons.size).to eq 1
-              expect(subject.deployment_status_summary.reasons.first).to be_a_pact_never_verified_for_consumer "Cat"
+              expect(subject.deployment_status_summary.reasons.size).to eq 2
+              expect(subject.deployment_status_summary.reasons.last).to be_a_pact_never_verified_for_consumer "Cat"
             end
           end