Commit 4c974f50 authored by João Cunha's avatar João Cunha Committed by Sean McGivern

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
parent b7a3c8f6
...@@ -27,9 +27,11 @@ module Clusters ...@@ -27,9 +27,11 @@ module Clusters
application.oauth_application = create_oauth_application(application, request) application.oauth_application = create_oauth_application(application, request)
end 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
end end
......
# 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
require 'spec_helper' require 'spec_helper'
describe Gitlab::Profiler do describe Gitlab::Profiler do
RSpec::Matchers.define_negated_matcher :not_change, :change
let(:null_logger) { Logger.new('/dev/null') } let(:null_logger) { Logger.new('/dev/null') }
let(:private_token) { 'private' } let(:private_token) { 'private' }
...@@ -187,7 +185,7 @@ describe Gitlab::Profiler do ...@@ -187,7 +185,7 @@ describe Gitlab::Profiler do
end end
it 'does not modify the standard Rails loggers' do 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 } .to not_change { ActiveRecord::Base.logger }
.and not_change { ActionController::Base.logger } .and not_change { ActionController::Base.logger }
.and not_change { ActiveSupport::LogSubscriber.colorize_logging } .and not_change { ActiveSupport::LogSubscriber.colorize_logging }
...@@ -204,7 +202,7 @@ describe Gitlab::Profiler do ...@@ -204,7 +202,7 @@ describe Gitlab::Profiler do
end end
it 'cleans up ApplicationController afterwards' do 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) } .to not_change { ActionController.instance_methods(false) }
end end
end end
...@@ -213,7 +211,7 @@ describe Gitlab::Profiler do ...@@ -213,7 +211,7 @@ describe Gitlab::Profiler do
it 'does not define methods on ApplicationController' do it 'does not define methods on ApplicationController' do
expect(ApplicationController).not_to receive(:define_method) expect(ApplicationController).not_to receive(:define_method)
described_class.with_user(nil) { } described_class.with_user(nil) {}
end end
end end
end end
......
...@@ -26,12 +26,6 @@ describe Clusters::Applications::CreateService do ...@@ -26,12 +26,6 @@ describe Clusters::Applications::CreateService do
end.to change(cluster, :application_helm) end.to change(cluster, :application_helm)
end 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 context 'application already installed' do
let!(:application) { create(:clusters_applications_helm, :installed, cluster: cluster) } let!(:application) { create(:clusters_applications_helm, :installed, cluster: cluster) }
...@@ -42,12 +36,17 @@ describe Clusters::Applications::CreateService do ...@@ -42,12 +36,17 @@ describe Clusters::Applications::CreateService do
end end
it 'schedules an upgrade for the application' do 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 subject
end end
end end
context 'known applications' do
before do
create(:clusters_applications_helm, :installed, cluster: cluster)
end
context 'cert manager application' do context 'cert manager application' do
let(:params) do let(:params) do
{ {
...@@ -57,7 +56,9 @@ describe Clusters::Applications::CreateService do ...@@ -57,7 +56,9 @@ describe Clusters::Applications::CreateService do
end end
before do before do
allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) expect_any_instance_of(Clusters::Applications::CertManager)
.to receive(:make_scheduled!)
.and_call_original
end end
it 'creates the application' do it 'creates the application' do
...@@ -82,7 +83,10 @@ describe Clusters::Applications::CreateService do ...@@ -82,7 +83,10 @@ describe Clusters::Applications::CreateService do
end end
before do before do
allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) 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 end
it 'creates the application' do it 'creates the application' do
...@@ -111,7 +115,9 @@ describe Clusters::Applications::CreateService do ...@@ -111,7 +115,9 @@ describe Clusters::Applications::CreateService do
end end
before do before do
allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) expect_any_instance_of(Clusters::Applications::Knative)
.to receive(:make_scheduled!)
.and_call_original
end end
it 'creates the application' do it 'creates the application' do
...@@ -126,6 +132,7 @@ describe Clusters::Applications::CreateService do ...@@ -126,6 +132,7 @@ describe Clusters::Applications::CreateService do
expect(subject.hostname).to eq('example.com') expect(subject.hostname).to eq('example.com')
end end
end end
end
context 'invalid application' do context 'invalid application' do
let(:params) { { application: 'non-existent' } } let(:params) { { application: 'non-existent' } }
...@@ -140,19 +147,21 @@ describe Clusters::Applications::CreateService do ...@@ -140,19 +147,21 @@ describe Clusters::Applications::CreateService do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
before do where(:application, :association, :allowed, :pre_create_helm) do
allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) '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 end
where(:application, :association, :allowed) do with_them do
'helm' | :application_helm | true before do
'ingress' | :application_ingress | true klass = "Clusters::Applications::#{application.titleize}"
'runner' | :application_runner | false allow_any_instance_of(klass.constantize).to receive(:make_scheduled!).and_call_original
'jupyter' | :application_jupyter | false create(:clusters_applications_helm, :installed, cluster: cluster) if pre_create_helm
'prometheus' | :application_prometheus | false
end end
with_them do
let(:params) { { application: application } } let(:params) { { application: application } }
it 'executes for each application' do it 'executes for each application' do
...@@ -168,5 +177,68 @@ describe Clusters::Applications::CreateService do ...@@ -168,5 +177,68 @@ describe Clusters::Applications::CreateService do
end end
end 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
end end
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
# frozen_string_literal: true
RSpec::Matchers.define_negated_matcher :not_change, :change
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment