Merge branch 'refactor/get_rid_of_schedule_installation_service' into 'master'

Get rid of ScheduleInstallationService

See merge request gitlab-org/gitlab-ce!25420

app/services/clusters/applications/create_service.rb

@@ -27,9 +27,11 @@ def execute(request)
             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
 

app/services/clusters/applications/schedule_installation_service.rb

-# 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

spec/lib/gitlab/profiler_spec.rb

 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 @@
       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 @@
       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 @@
       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

spec/services/clusters/applications/create_service_spec.rb

@@ -26,12 +26,6 @@
       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 @@
       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 @@
 
       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 @@
         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

spec/services/clusters/applications/schedule_installation_service_spec.rb

-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

spec/support/matchers/not_changed_matcher.rb

+# frozen_string_literal: true
+
+RSpec::Matchers.define_negated_matcher :not_change, :change