From 9eeca2db6db51e1d7b121a0efdfe14b517dea4b6 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Tue, 12 Feb 2019 16:13:59 +1300 Subject: [PATCH 1/3] Move common method to application concern This could be potentially useful to all cluster applications. Address followup issue https://gitlab.com/gitlab-org/gitlab-ce/issues/56524 --- .../clusters/applications/prometheus.rb | 5 -- .../clusters/concerns/application_status.rb | 4 ++ .../clusters/applications/prometheus_spec.rb | 59 ------------------- ...ster_application_status_shared_examples.rb | 26 ++++++++ 4 files changed, 30 insertions(+), 64 deletions(-) diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index 52c440ffb2f..fa7ce363531 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -6,7 +6,6 @@ module Clusters include PrometheusAdapter VERSION = '6.7.3' - READY_STATUS = [:installed, :updating, :updated, :update_errored].freeze self.table_name = 'clusters_applications_prometheus' @@ -25,10 +24,6 @@ module Clusters end end - def ready? - READY_STATUS.include?(status_name) - end - def chart 'stable/prometheus' end diff --git a/app/models/clusters/concerns/application_status.rb b/app/models/clusters/concerns/application_status.rb index 5c0164831bc..9ecd7229604 100644 --- a/app/models/clusters/concerns/application_status.rb +++ b/app/models/clusters/concerns/application_status.rb @@ -80,6 +80,10 @@ module Clusters installed? || updated? end + def ready? + installed? || updating? || updated? || update_errored? + end + def update_in_progress? updating? end diff --git a/spec/models/clusters/applications/prometheus_spec.rb b/spec/models/clusters/applications/prometheus_spec.rb index caf59b0fc31..4996088f921 100644 --- a/spec/models/clusters/applications/prometheus_spec.rb +++ b/spec/models/clusters/applications/prometheus_spec.rb @@ -39,65 +39,6 @@ describe Clusters::Applications::Prometheus do end end - describe '#ready' do - let(:project) { create(:project) } - let(:cluster) { create(:cluster, projects: [project]) } - - it 'returns true when installed' do - application = build(:clusters_applications_prometheus, :installed, cluster: cluster) - - expect(application).to be_ready - end - - it 'returns false when not_installable' do - application = build(:clusters_applications_prometheus, :not_installable, cluster: cluster) - - expect(application).not_to be_ready - end - - it 'returns false when installable' do - application = build(:clusters_applications_prometheus, :installable, cluster: cluster) - - expect(application).not_to be_ready - end - - it 'returns false when scheduled' do - application = build(:clusters_applications_prometheus, :scheduled, cluster: cluster) - - expect(application).not_to be_ready - end - - it 'returns false when installing' do - application = build(:clusters_applications_prometheus, :installing, cluster: cluster) - - expect(application).not_to be_ready - end - - it 'returns false when errored' do - application = build(:clusters_applications_prometheus, :errored, cluster: cluster) - - expect(application).not_to be_ready - end - - it 'returns true when updating' do - application = build(:clusters_applications_prometheus, :updating, cluster: cluster) - - expect(application).to be_ready - end - - it 'returns true when updated' do - application = build(:clusters_applications_prometheus, :updated, cluster: cluster) - - expect(application).to be_ready - end - - it 'returns true when errored' do - application = build(:clusters_applications_prometheus, :update_errored, cluster: cluster) - - expect(application).to be_ready - end - end - describe '#prometheus_client' do context 'cluster is nil' do it 'returns nil' do diff --git a/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb b/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb index c96a65cb56a..18b279658bc 100644 --- a/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb +++ b/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb @@ -138,6 +138,32 @@ shared_examples 'cluster application status specs' do |application_name| end end + describe '#ready?' do + using RSpec::Parameterized::TableSyntax + + where(:trait, :ready) do + :not_installable | false + :installable | false + :scheduled | false + :installing | false + :installed | true + :updating | true + :updated | true + :errored | false + :update_errored | true + end + + with_them do + subject { build(application_name, trait) } + + if params[:ready] + it { is_expected.to be_ready } + else + it { is_expected.not_to be_ready } + end + end + end + describe '#available?' do using RSpec::Parameterized::TableSyntax From 4e5494081e95adf05ab9df47c284bd796ecc4b01 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Fri, 15 Feb 2019 12:41:10 +1300 Subject: [PATCH 2/3] Spec: applications not yet installed doesn't count We should not consider an application that is not yet installed or where the install errored as available. Write a spec to assert this --- spec/services/prometheus/adapter_service_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/services/prometheus/adapter_service_spec.rb b/spec/services/prometheus/adapter_service_spec.rb index 335fc5844aa..5fe9668fd9d 100644 --- a/spec/services/prometheus/adapter_service_spec.rb +++ b/spec/services/prometheus/adapter_service_spec.rb @@ -22,6 +22,14 @@ describe Prometheus::AdapterService do context "prometheus service can't execute queries" do let(:prometheus_service) { double(:prometheus_service, can_query?: false) } + context 'with cluster with prometheus not yet installed' do + let!(:prometheus) { create(:clusters_applications_prometheus, :installable, cluster: cluster) } + + it 'returns nil' do + expect(subject.prometheus_adapter).to be_nil + end + end + context 'with cluster with prometheus installed' do let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: cluster) } From 13bb704d50cc885935bc7b0f850d47c1ace5cdd7 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Fri, 15 Feb 2019 12:43:44 +1300 Subject: [PATCH 3/3] Remove #ready? method in favor of #available? Given https://github.com/helm/helm/issues/3338, I think that we should exclude applications that are :update_errored, :updating as well. --- .../clusters/concerns/application_status.rb | 4 --- app/services/prometheus/adapter_service.rb | 2 +- .../prometheus/adapter_service_spec.rb | 4 +-- ...ster_application_status_shared_examples.rb | 26 ------------------- 4 files changed, 3 insertions(+), 33 deletions(-) diff --git a/app/models/clusters/concerns/application_status.rb b/app/models/clusters/concerns/application_status.rb index 9ecd7229604..5c0164831bc 100644 --- a/app/models/clusters/concerns/application_status.rb +++ b/app/models/clusters/concerns/application_status.rb @@ -80,10 +80,6 @@ module Clusters installed? || updated? end - def ready? - installed? || updating? || updated? || update_errored? - end - def update_in_progress? updating? end diff --git a/app/services/prometheus/adapter_service.rb b/app/services/prometheus/adapter_service.rb index a791845ba20..3be958e1613 100644 --- a/app/services/prometheus/adapter_service.rb +++ b/app/services/prometheus/adapter_service.rb @@ -30,7 +30,7 @@ module Prometheus return unless deployment_platform.respond_to?(:cluster) cluster = deployment_platform.cluster - return unless cluster.application_prometheus&.ready? + return unless cluster.application_prometheus&.available? cluster.application_prometheus end diff --git a/spec/services/prometheus/adapter_service_spec.rb b/spec/services/prometheus/adapter_service_spec.rb index 5fe9668fd9d..505e2935e93 100644 --- a/spec/services/prometheus/adapter_service_spec.rb +++ b/spec/services/prometheus/adapter_service_spec.rb @@ -22,7 +22,7 @@ describe Prometheus::AdapterService do context "prometheus service can't execute queries" do let(:prometheus_service) { double(:prometheus_service, can_query?: false) } - context 'with cluster with prometheus not yet installed' do + context 'with cluster with prometheus not available' do let!(:prometheus) { create(:clusters_applications_prometheus, :installable, cluster: cluster) } it 'returns nil' do @@ -30,7 +30,7 @@ describe Prometheus::AdapterService do end end - context 'with cluster with prometheus installed' do + context 'with cluster with prometheus available' do let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: cluster) } it 'returns application handling all environments' do diff --git a/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb b/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb index 18b279658bc..c96a65cb56a 100644 --- a/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb +++ b/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb @@ -138,32 +138,6 @@ shared_examples 'cluster application status specs' do |application_name| end end - describe '#ready?' do - using RSpec::Parameterized::TableSyntax - - where(:trait, :ready) do - :not_installable | false - :installable | false - :scheduled | false - :installing | false - :installed | true - :updating | true - :updated | true - :errored | false - :update_errored | true - end - - with_them do - subject { build(application_name, trait) } - - if params[:ready] - it { is_expected.to be_ready } - else - it { is_expected.not_to be_ready } - end - end - end - describe '#available?' do using RSpec::Parameterized::TableSyntax