Merge branch 'sy-service-selector-refactor' into 'master'

Refactor dashboard ServiceSelector to use consistent selection API

See merge request gitlab-org/gitlab!23488

app/services/metrics/dashboard/default_embed_service.rb

@@ -20,6 +20,12 @@ module Metrics
         system_metrics_kubernetes_container_cores_total
       ).freeze
 
+      class << self
+        def valid_params?(params)
+          params[:embedded].present?
+        end
+      end
+
       # Returns a new dashboard with only the matching
       # metrics from the system dashboard, stripped of groups.
       # @return [Hash]

app/services/metrics/dashboard/predefined_dashboard_service.rb

@@ -15,6 +15,10 @@ module Metrics
       ].freeze
 
       class << self
+        def valid_params?(params)
+          matching_dashboard?(params[:dashboard_path])
+        end
+
         def matching_dashboard?(filepath)
           filepath == self::DASHBOARD_PATH
         end

app/services/metrics/dashboard/project_dashboard_service.rb

@@ -9,6 +9,10 @@ module Metrics
       DASHBOARD_ROOT = ".gitlab/dashboards"
 
       class << self
+        def valid_params?(params)
+          params[:dashboard_path].present?
+        end
+
         def all_dashboard_paths(project)
           file_finder(project)
             .list_files_for(DASHBOARD_ROOT)

ee/app/services/metrics/dashboard/cluster_dashboard_service.rb

@@ -14,6 +14,12 @@ module Metrics
         STAGES::Sorter
       ].freeze
 
+      class << self
+        def valid_params?(params)
+          params[:cluster].present?
+        end
+      end
+
       # Permissions are handled at the controller level
       def allowed?
         true

ee/lib/ee/gitlab/metrics/dashboard/service_selector.rb

@@ -7,11 +7,18 @@ module EE
         module ServiceSelector
           extend ActiveSupport::Concern
 
+          EE_SERVICES = [
+            ::Metrics::Dashboard::ClusterDashboardService
+          ].freeze
+
           class_methods do
-            def call(params)
-              return ::Metrics::Dashboard::ClusterDashboardService if params[:cluster]
+            extend ::Gitlab::Utils::Override
+
+            private
 
-              super
+            override :services
+            def services
+              EE_SERVICES + super
             end
           end
         end

ee/spec/services/metrics/dashboard/cluster_dashboard_service_spec.rb

@@ -14,7 +14,21 @@ describe Metrics::Dashboard::ClusterDashboardService, :use_clean_rails_memory_st
     project.add_maintainer(user)
   end
 
-  describe 'get_dashboard' do
+  describe '.valid_params?' do
+    let(:params) { { cluster: cluster } }
+
+    subject { described_class.valid_params?(params) }
+
+    it { is_expected.to be_truthy }
+
+    context 'missing cluster' do
+      let(:params) { {} }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#get_dashboard' do
     let(:service_params) { [project, user, { cluster: cluster, cluster_type: :project }] }
     let(:service_call) { described_class.new(*service_params).get_dashboard }
 

lib/gitlab/metrics/dashboard/service_selector.rb

@@ -8,50 +8,39 @@ module Gitlab
   module Metrics
     module Dashboard
       class ServiceSelector
-        SERVICES = ::Metrics::Dashboard
-
         class << self
           include Gitlab::Utils::StrongMemoize
 
+          SERVICES = [
+            ::Metrics::Dashboard::CustomMetricEmbedService,
+            ::Metrics::Dashboard::GrafanaMetricEmbedService,
+            ::Metrics::Dashboard::DynamicEmbedService,
+            ::Metrics::Dashboard::DefaultEmbedService,
+            ::Metrics::Dashboard::SystemDashboardService,
+            ::Metrics::Dashboard::PodDashboardService,
+            ::Metrics::Dashboard::ProjectDashboardService
+          ].freeze
+
           # Returns a class which inherits from the BaseService
-          # class that can be used to obtain a dashboard.
+          # class that can be used to obtain a dashboard for
+          # the provided params.
           # @return [Gitlab::Metrics::Dashboard::Services::BaseService]
           def call(params)
-            return SERVICES::CustomMetricEmbedService if custom_metric_embed?(params)
-            return SERVICES::GrafanaMetricEmbedService if grafana_metric_embed?(params)
-            return SERVICES::DynamicEmbedService if dynamic_embed?(params)
-            return SERVICES::DefaultEmbedService if params[:embedded]
-            return SERVICES::SystemDashboardService if system_dashboard?(params[:dashboard_path])
-            return SERVICES::PodDashboardService if pod_dashboard?(params[:dashboard_path])
-            return SERVICES::ProjectDashboardService if params[:dashboard_path]
-
-            default_service
+            service = services.find do |service_class|
+              service_class.valid_params?(params)
             end
 
-          private
-
-          def default_service
-            SERVICES::SystemDashboardService
+            service || default_service
           end
 
-          def system_dashboard?(filepath)
-            SERVICES::SystemDashboardService.matching_dashboard?(filepath)
-          end
-
-          def pod_dashboard?(filepath)
-            SERVICES::PodDashboardService.matching_dashboard?(filepath)
-          end
-
-          def custom_metric_embed?(params)
-            SERVICES::CustomMetricEmbedService.valid_params?(params)
-          end
+          private
 
-          def grafana_metric_embed?(params)
-            SERVICES::GrafanaMetricEmbedService.valid_params?(params)
+          def services
+            SERVICES
           end
 
-          def dynamic_embed?(params)
-            SERVICES::DynamicEmbedService.valid_params?(params)
+          def default_service
+            ::Metrics::Dashboard::SystemDashboardService
           end
         end
       end

spec/services/metrics/dashboard/default_embed_service_spec.rb

@@ -13,6 +13,26 @@ describe Metrics::Dashboard::DefaultEmbedService, :use_clean_rails_memory_store_
     project.add_maintainer(user)
   end
 
+  describe '.valid_params?' do
+    let(:params) { { embedded: true } }
+
+    subject { described_class.valid_params?(params) }
+
+    it { is_expected.to be_truthy }
+
+    context 'missing embedded' do
+      let(:params) { {} }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'not embedded' do
+      let(:params) { { embedded: false } }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
   describe '#get_dashboard' do
     let(:service_params) { [project, user, { environment: environment }] }
     let(:service_call) { described_class.new(*service_params).get_dashboard }

spec/services/metrics/dashboard/pod_dashboard_service_spec.rb

@@ -13,7 +13,27 @@ describe Metrics::Dashboard::PodDashboardService, :use_clean_rails_memory_store_
     project.add_maintainer(user)
   end
 
-  describe 'get_dashboard' do
+  describe '.valid_params?' do
+    let(:params) { { dashboard_path: described_class::DASHBOARD_PATH } }
+
+    subject { described_class.valid_params?(params) }
+
+    it { is_expected.to be_truthy }
+
+    context 'missing dashboard_path' do
+      let(:params) { {} }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'non-matching dashboard_path' do
+      let(:params) { { dashboard_path: 'path/to/bunk.yml' } }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#get_dashboard' do
     let(:dashboard_path) { described_class::DASHBOARD_PATH }
     let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
     let(:service_call) { described_class.new(*service_params).get_dashboard }

spec/services/metrics/dashboard/project_dashboard_service_spec.rb

@@ -13,7 +13,7 @@ describe Metrics::Dashboard::ProjectDashboardService, :use_clean_rails_memory_st
     project.add_maintainer(user)
   end
 
-  describe 'get_dashboard' do
+  describe '#get_dashboard' do
     let(:dashboard_path) { '.gitlab/dashboards/test.yml' }
     let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
     let(:service_call) { described_class.new(*service_params).get_dashboard }
@@ -62,7 +62,7 @@ describe Metrics::Dashboard::ProjectDashboardService, :use_clean_rails_memory_st
     end
   end
 
-  describe '::all_dashboard_paths' do
+  describe '.all_dashboard_paths' do
     let(:all_dashboards) { described_class.all_dashboard_paths(project) }
 
     context 'when there are no project dashboards' do
@@ -87,4 +87,24 @@ describe Metrics::Dashboard::ProjectDashboardService, :use_clean_rails_memory_st
       end
     end
   end
+
+  describe '.valid_params?' do
+    let(:params) { { dashboard_path: '.gitlab/dashboard/test.yml' } }
+
+    subject { described_class.valid_params?(params) }
+
+    it { is_expected.to be_truthy }
+
+    context 'missing dashboard_path' do
+      let(:params) { {} }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'empty dashboard_path' do
+      let(:params) { { dashboard_path: '' } }
+
+      it { is_expected.to be_falsey }
+    end
+  end
 end

spec/services/metrics/dashboard/system_dashboard_service_spec.rb

@@ -13,7 +13,7 @@ describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memory_sto
     project.add_maintainer(user)
   end
 
-  describe 'get_dashboard' do
+  describe '#get_dashboard' do
     let(:dashboard_path) { described_class::DASHBOARD_PATH }
     let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
     let(:service_call) { described_class.new(*service_params).get_dashboard }
@@ -30,7 +30,7 @@ describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memory_sto
     end
   end
 
-  describe '::all_dashboard_paths' do
+  describe '.all_dashboard_paths' do
     it 'returns the dashboard attributes' do
       all_dashboards = described_class.all_dashboard_paths(project)
 
@@ -44,4 +44,24 @@ describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memory_sto
       )
     end
   end
+
+  describe '.valid_params?' do
+    let(:params) { { dashboard_path: described_class::DASHBOARD_PATH } }
+
+    subject { described_class.valid_params?(params) }
+
+    it { is_expected.to be_truthy }
+
+    context 'missing dashboard_path' do
+      let(:params) { {} }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'non-matching dashboard_path' do
+      let(:params) { { dashboard_path: 'path/to/bunk.yml' } }
+
+      it { is_expected.to be_falsey }
+    end
+  end
 end