diff options
author | João Cunha <j.a.cunha@gmail.com> | 2019-02-26 19:44:31 +0300 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2019-02-26 19:44:31 +0300 |
commit | 4c974f50f6e85165f9750cc42760e4a2fbd81e6f (patch) | |
tree | a856956ba1072007ba5ddce36b10ef2b8e808a63 | |
parent | b7a3c8f6404ba2890409e0e79697fa34354cf46e (diff) |
Get rid of ScheduleInstallationService
- deletes schedule_installation_service.rb
- moves schedule_installation_service.rb logic to create_service.rb
- moves specs as well
Removes code duplication
Remove unecessary spec block
Abide review suggestions
Test installable applications which are not associated to a cluster
Fix a typo
Removes duplciated expectation
Reuse variable instead of redefining
Remove method in favor of a local scoped lambda
Improve 'failing service' shared examples
Test the increase of status count
Remove duplicated test
Enable fronzen literal
6 files changed, 157 insertions, 190 deletions
diff --git a/app/services/clusters/applications/create_service.rb b/app/services/clusters/applications/create_service.rb index 92c2c1b9834..12f8c849d41 100644 --- a/app/services/clusters/applications/create_service.rb +++ b/app/services/clusters/applications/create_service.rb @@ -27,9 +27,11 @@ module Clusters application.oauth_application = create_oauth_application(application, request) end - application.save! + worker = application.updateable? ? ClusterUpgradeAppWorker : ClusterInstallAppWorker - Clusters::Applications::ScheduleInstallationService.new(application).execute + application.make_scheduled! + + worker.perform_async(application.name, application.id) end end diff --git a/app/services/clusters/applications/schedule_installation_service.rb b/app/services/clusters/applications/schedule_installation_service.rb deleted file mode 100644 index 15c93f1e79b..00000000000 --- a/app/services/clusters/applications/schedule_installation_service.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module Clusters - module Applications - class ScheduleInstallationService - attr_reader :application - - def initialize(application) - @application = application - end - - def execute - application.updateable? ? schedule_upgrade : schedule_install - end - - private - - def schedule_upgrade - application.make_scheduled! - - ClusterUpgradeAppWorker.perform_async(application.name, application.id) - end - - def schedule_install - application.make_scheduled! - - ClusterInstallAppWorker.perform_async(application.name, application.id) - end - end - end -end diff --git a/spec/lib/gitlab/profiler_spec.rb b/spec/lib/gitlab/profiler_spec.rb index 8bb0c1a0b8a..9f2214f7ce7 100644 --- a/spec/lib/gitlab/profiler_spec.rb +++ b/spec/lib/gitlab/profiler_spec.rb @@ -1,8 +1,6 @@ require 'spec_helper' describe Gitlab::Profiler do - RSpec::Matchers.define_negated_matcher :not_change, :change - let(:null_logger) { Logger.new('/dev/null') } let(:private_token) { 'private' } @@ -187,7 +185,7 @@ describe Gitlab::Profiler do end it 'does not modify the standard Rails loggers' do - expect { described_class.with_custom_logger(nil) { } } + expect { described_class.with_custom_logger(nil) {} } .to not_change { ActiveRecord::Base.logger } .and not_change { ActionController::Base.logger } .and not_change { ActiveSupport::LogSubscriber.colorize_logging } @@ -204,7 +202,7 @@ describe Gitlab::Profiler do end it 'cleans up ApplicationController afterwards' do - expect { described_class.with_user(user) { } } + expect { described_class.with_user(user) {} } .to not_change { ActionController.instance_methods(false) } end end @@ -213,7 +211,7 @@ describe Gitlab::Profiler do it 'does not define methods on ApplicationController' do expect(ApplicationController).not_to receive(:define_method) - described_class.with_user(nil) { } + described_class.with_user(nil) {} end end end diff --git a/spec/services/clusters/applications/create_service_spec.rb b/spec/services/clusters/applications/create_service_spec.rb index 3f621ed5944..cbdef008b07 100644 --- a/spec/services/clusters/applications/create_service_spec.rb +++ b/spec/services/clusters/applications/create_service_spec.rb @@ -26,12 +26,6 @@ describe Clusters::Applications::CreateService do end.to change(cluster, :application_helm) end - it 'schedules an install via worker' do - expect(ClusterInstallAppWorker).to receive(:perform_async).with('helm', anything).once - - subject - end - context 'application already installed' do let!(:application) { create(:clusters_applications_helm, :installed, cluster: cluster) } @@ -42,88 +36,101 @@ describe Clusters::Applications::CreateService do end it 'schedules an upgrade for the application' do - expect(Clusters::Applications::ScheduleInstallationService).to receive(:new).with(application).and_call_original + expect(ClusterUpgradeAppWorker).to receive(:perform_async) subject end end - context 'cert manager application' do - let(:params) do - { - application: 'cert_manager', - email: 'test@example.com' - } - end - + context 'known applications' do before do - allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) + create(:clusters_applications_helm, :installed, cluster: cluster) end - it 'creates the application' do - expect do - subject + context 'cert manager application' do + let(:params) do + { + application: 'cert_manager', + email: 'test@example.com' + } + end - cluster.reload - end.to change(cluster, :application_cert_manager) - end + before do + expect_any_instance_of(Clusters::Applications::CertManager) + .to receive(:make_scheduled!) + .and_call_original + end - it 'sets the email' do - expect(subject.email).to eq('test@example.com') - end - end + it 'creates the application' do + expect do + subject - context 'jupyter application' do - let(:params) do - { - application: 'jupyter', - hostname: 'example.com' - } - end + cluster.reload + end.to change(cluster, :application_cert_manager) + end - before do - allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) + it 'sets the email' do + expect(subject.email).to eq('test@example.com') + end end - it 'creates the application' do - expect do - subject + context 'jupyter application' do + let(:params) do + { + application: 'jupyter', + hostname: 'example.com' + } + end - cluster.reload - end.to change(cluster, :application_jupyter) - end + before do + create(:clusters_applications_ingress, :installed, external_ip: "127.0.0.0", cluster: cluster) + expect_any_instance_of(Clusters::Applications::Jupyter) + .to receive(:make_scheduled!) + .and_call_original + end - it 'sets the hostname' do - expect(subject.hostname).to eq('example.com') - end + it 'creates the application' do + expect do + subject - it 'sets the oauth_application' do - expect(subject.oauth_application).to be_present - end - end + cluster.reload + end.to change(cluster, :application_jupyter) + end - context 'knative application' do - let(:params) do - { - application: 'knative', - hostname: 'example.com' - } - end + it 'sets the hostname' do + expect(subject.hostname).to eq('example.com') + end - before do - allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) + it 'sets the oauth_application' do + expect(subject.oauth_application).to be_present + end end - it 'creates the application' do - expect do - subject + context 'knative application' do + let(:params) do + { + application: 'knative', + hostname: 'example.com' + } + end - cluster.reload - end.to change(cluster, :application_knative) - end + before do + expect_any_instance_of(Clusters::Applications::Knative) + .to receive(:make_scheduled!) + .and_call_original + end - it 'sets the hostname' do - expect(subject.hostname).to eq('example.com') + it 'creates the application' do + expect do + subject + + cluster.reload + end.to change(cluster, :application_knative) + end + + it 'sets the hostname' do + expect(subject.hostname).to eq('example.com') + end end end @@ -140,19 +147,21 @@ describe Clusters::Applications::CreateService do using RSpec::Parameterized::TableSyntax - before do - allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) - end - - where(:application, :association, :allowed) do - 'helm' | :application_helm | true - 'ingress' | :application_ingress | true - 'runner' | :application_runner | false - 'jupyter' | :application_jupyter | false - 'prometheus' | :application_prometheus | false + where(:application, :association, :allowed, :pre_create_helm) do + 'helm' | :application_helm | true | false + 'ingress' | :application_ingress | true | true + 'runner' | :application_runner | false | true + 'jupyter' | :application_jupyter | false | true + 'prometheus' | :application_prometheus | false | true end with_them do + before do + klass = "Clusters::Applications::#{application.titleize}" + allow_any_instance_of(klass.constantize).to receive(:make_scheduled!).and_call_original + create(:clusters_applications_helm, :installed, cluster: cluster) if pre_create_helm + end + let(:params) { { application: application } } it 'executes for each application' do @@ -168,5 +177,68 @@ describe Clusters::Applications::CreateService do end end end + + context 'when application is installable' do + shared_examples 'installable applications' do + it 'makes the application scheduled' do + expect do + subject + end.to change { Clusters::Applications::Helm.with_status(:scheduled).count }.by(1) + end + + it 'schedules an install via worker' do + expect(ClusterInstallAppWorker) + .to receive(:perform_async) + .with(*worker_arguments) + .once + + subject + end + end + + context 'when application is associated with a cluster' do + let(:application) { create(:clusters_applications_helm, :installable, cluster: cluster) } + let(:worker_arguments) { [application.name, application.id] } + + it_behaves_like 'installable applications' + end + + context 'when application is not associated with a cluster' do + let(:worker_arguments) { [params[:application], kind_of(Numeric)] } + + it_behaves_like 'installable applications' + end + end + + context 'when installation is already in progress' do + let!(:application) { create(:clusters_applications_helm, :installing, cluster: cluster) } + + it 'raises an exception' do + expect { subject } + .to raise_exception(StateMachines::InvalidTransition) + .and not_change(application.class.with_status(:scheduled), :count) + end + + it 'does not schedule a cluster worker' do + expect(ClusterInstallAppWorker).not_to receive(:perform_async) + end + end + + context 'when application is installed' do + %i(installed updated).each do |status| + let(:application) { create(:clusters_applications_helm, status, cluster: cluster) } + + it 'schedules an upgrade via worker' do + expect(ClusterUpgradeAppWorker) + .to receive(:perform_async) + .with(application.name, application.id) + .once + + subject + + expect(application.reload).to be_scheduled + end + end + end end end diff --git a/spec/services/clusters/applications/schedule_installation_service_spec.rb b/spec/services/clusters/applications/schedule_installation_service_spec.rb deleted file mode 100644 index 8380932dfaa..00000000000 --- a/spec/services/clusters/applications/schedule_installation_service_spec.rb +++ /dev/null @@ -1,77 +0,0 @@ -require 'spec_helper' - -describe Clusters::Applications::ScheduleInstallationService do - def count_scheduled - application&.class&.with_status(:scheduled)&.count || 0 - end - - shared_examples 'a failing service' do - it 'raise an exception' do - expect(ClusterInstallAppWorker).not_to receive(:perform_async) - count_before = count_scheduled - - expect { service.execute }.to raise_error(StandardError) - expect(count_scheduled).to eq(count_before) - end - end - - describe '#execute' do - let(:service) { described_class.new(application) } - - context 'when application is installable' do - let(:application) { create(:clusters_applications_helm, :installable) } - - it 'make the application scheduled' do - expect(ClusterInstallAppWorker).to receive(:perform_async).with(application.name, kind_of(Numeric)).once - - expect { service.execute }.to change { application.class.with_status(:scheduled).count }.by(1) - end - end - - context 'when installation is already in progress' do - let(:application) { create(:clusters_applications_helm, :installing) } - - it_behaves_like 'a failing service' - end - - context 'when application is nil' do - let(:application) { nil } - - it_behaves_like 'a failing service' - end - - context 'when application cannot be persisted' do - let(:application) { create(:clusters_applications_helm) } - - before do - expect(application).to receive(:make_scheduled!).once.and_raise(ActiveRecord::RecordInvalid) - end - - it_behaves_like 'a failing service' - end - - context 'when application is installed' do - let(:application) { create(:clusters_applications_helm, :installed) } - - it 'schedules an upgrade via worker' do - expect(ClusterUpgradeAppWorker).to receive(:perform_async).with(application.name, application.id).once - - service.execute - - expect(application).to be_scheduled - end - end - - context 'when application is updated' do - let(:application) { create(:clusters_applications_helm, :updated) } - - it 'schedules an upgrade via worker' do - expect(ClusterUpgradeAppWorker).to receive(:perform_async).with(application.name, application.id).once - - service.execute - - expect(application).to be_scheduled - end - end - end -end diff --git a/spec/support/matchers/not_changed_matcher.rb b/spec/support/matchers/not_changed_matcher.rb new file mode 100644 index 00000000000..8ef4694982d --- /dev/null +++ b/spec/support/matchers/not_changed_matcher.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +RSpec::Matchers.define_negated_matcher :not_change, :change |