From cc1ccbf83a4a20f367011a6aec3d158214cfeb34 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Wed, 10 Oct 2018 16:47:27 +1300 Subject: Move non-controller code into dedicated service This should help with code re-use when we create applications for group level cluster next. Change `find_or_initialize_by` to explicitly find or build the right association based on the application name. The benefit here is that we use the associations on @cluster rather than querying from the other side of the association. --- .../projects/clusters/applications_controller.rb | 34 +++-------- app/services/applications/create_service.rb | 1 + .../clusters/applications/create_service.rb | 66 ++++++++++++++++++++++ .../clusters/applications/create_service_spec.rb | 63 +++++++++++++++++++++ 4 files changed, 137 insertions(+), 27 deletions(-) create mode 100644 app/services/clusters/applications/create_service.rb create mode 100644 spec/services/clusters/applications/create_service_spec.rb diff --git a/app/controllers/projects/clusters/applications_controller.rb b/app/controllers/projects/clusters/applications_controller.rb index c356f8d2987..a3fc4f4741b 100644 --- a/app/controllers/projects/clusters/applications_controller.rb +++ b/app/controllers/projects/clusters/applications_controller.rb @@ -2,31 +2,22 @@ class Projects::Clusters::ApplicationsController < Projects::ApplicationController before_action :cluster - before_action :application_class, only: [:create] before_action :authorize_read_cluster! before_action :authorize_create_cluster!, only: [:create] - # rubocop: disable CodeReuse/ActiveRecord def create - application = @application_class.find_or_initialize_by(cluster: @cluster) - - if application.has_attribute?(:hostname) - application.hostname = params[:hostname] - end - - if application.respond_to?(:oauth_application) - application.oauth_application = create_oauth_application(application) - end - - application.save! + application = Clusters::Applications::CreateService + .new(@cluster, current_user, create_cluster_application_params) + .execute(request) Clusters::Applications::ScheduleInstallationService.new(project, current_user).execute(application) head :no_content + rescue Clusters::Applications::CreateService::InvalidApplicationError + render_404 rescue StandardError head :bad_request end - # rubocop: enable CodeReuse/ActiveRecord private @@ -34,18 +25,7 @@ class Projects::Clusters::ApplicationsController < Projects::ApplicationControll @cluster ||= project.clusters.find(params[:id]) || render_404 end - def application_class - @application_class ||= Clusters::Cluster::APPLICATIONS[params[:application]] || render_404 - end - - def create_oauth_application(application) - oauth_application_params = { - name: params[:application], - redirect_uri: application.callback_url, - scopes: 'api read_user openid', - owner: current_user - } - - Applications::CreateService.new(current_user, oauth_application_params).execute(request) + def create_cluster_application_params + params.permit(:application, :hostname) end end diff --git a/app/services/applications/create_service.rb b/app/services/applications/create_service.rb index 3d88c4f064e..b6c30da4d3a 100644 --- a/app/services/applications/create_service.rb +++ b/app/services/applications/create_service.rb @@ -9,6 +9,7 @@ module Applications end # rubocop: enable CodeReuse/ActiveRecord + # EE would override and use `request` arg def execute(request) Doorkeeper::Application.create(@params) end diff --git a/app/services/clusters/applications/create_service.rb b/app/services/clusters/applications/create_service.rb new file mode 100644 index 00000000000..d69e8612eed --- /dev/null +++ b/app/services/clusters/applications/create_service.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module Clusters + module Applications + class CreateService + InvalidApplicationError = Class.new(StandardError) + + attr_reader :cluster, :current_user, :params + + def initialize(cluster, user, params = {}) + @cluster = cluster + @current_user = user + @params = params.dup + end + + def execute(request) + create_application.tap do |application| + if application.has_attribute?(:hostname) + application.hostname = params[:hostname] + end + + if application.respond_to?(:oauth_application) + application.oauth_application = create_oauth_application(application, request) + end + + application.save! + end + end + + private + + def create_application + builder.call(@cluster) + end + + def builder + builders[application_name] || raise(InvalidApplicationError, "invalid application: #{application_name}") + end + + def builders + { + "helm" => -> (cluster) { cluster.application_helm || cluster.build_application_helm }, + "ingress" => -> (cluster) { cluster.application_ingress || cluster.build_application_ingress }, + "prometheus" => -> (cluster) { cluster.application_prometheus || cluster.build_application_prometheus }, + "runner" => -> (cluster) { cluster.application_runner || cluster.build_application_runner }, + "jupyter" => -> (cluster) { cluster.application_jupyter || cluster.build_application_jupyter } + } + end + + def application_name + params[:application] + end + + def create_oauth_application(application, request) + oauth_application_params = { + name: params[:application], + redirect_uri: application.callback_url, + scopes: 'api read_user openid', + owner: current_user + } + + ::Applications::CreateService.new(current_user, oauth_application_params).execute(request) + end + end + end +end diff --git a/spec/services/clusters/applications/create_service_spec.rb b/spec/services/clusters/applications/create_service_spec.rb new file mode 100644 index 00000000000..58d9682602a --- /dev/null +++ b/spec/services/clusters/applications/create_service_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Clusters::Applications::CreateService do + let(:cluster) { create(:cluster) } + let(:user) { create(:user) } + let(:params) { { application: 'helm' } } + let(:service) { described_class.new(cluster, user, params) } + + let(:request) do + if Gitlab.rails5? + ActionController::TestRequest.new({ remote_ip: "127.0.0.1" }, ActionController::TestSession.new) + else + ActionController::TestRequest.new(remote_ip: "127.0.0.1") + end + end + + describe '#execute' do + subject { service.execute(request) } + + it 'creates an application' do + expect do + subject + + cluster.reload + end.to change(cluster, :application_helm) + end + + context 'jupyter application' do + let(:params) do + { + application: 'jupyter', + hostname: 'example.com' + } + end + + it 'creates the application' do + expect do + subject + + cluster.reload + end.to change(cluster, :application_jupyter) + end + + it 'sets the hostname' do + expect(subject.hostname).to eq('example.com') + end + + it 'sets the oauth_application' do + expect(subject.oauth_application).to be_present + end + end + + context 'invalid application' do + let(:params) { { application: 'non-existent' } } + + it 'raises an error' do + expect { subject }.to raise_error(Clusters::Applications::CreateService::InvalidApplicationError) + end + end + end +end -- cgit v1.2.1 From 504cbb27c1018bd702346eb6497e37372dd9f35a Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Wed, 10 Oct 2018 21:45:49 +1300 Subject: Remove un-used inheritance from service Remove the inheritance from ::BaseService which is causing us to inherit the initializer that has project as the first arg, as we will not have access to project with forthcoming group clusters. Also call install service from create service - 1 less thing to re-use Extract TestRequest code into a spec helper. Given that we need different behaviour for Rails 5.0 (and again in Rails 5.1!), it's handy to have that branching behaviour in one place --- .../projects/clusters/applications_controller.rb | 4 +--- .../clusters/applications/create_service.rb | 2 ++ .../applications/schedule_installation_service.rb | 10 +++++++-- spec/services/applications/create_service_spec.rb | 13 +++++------ .../clusters/applications/create_service_spec.rb | 26 ++++++++++++++-------- .../schedule_installation_service_spec.rb | 7 +++--- spec/support/helpers/test_request_helpers.rb | 11 +++++++++ 7 files changed, 47 insertions(+), 26 deletions(-) create mode 100644 spec/support/helpers/test_request_helpers.rb diff --git a/app/controllers/projects/clusters/applications_controller.rb b/app/controllers/projects/clusters/applications_controller.rb index a3fc4f4741b..bcea96bce94 100644 --- a/app/controllers/projects/clusters/applications_controller.rb +++ b/app/controllers/projects/clusters/applications_controller.rb @@ -6,12 +6,10 @@ class Projects::Clusters::ApplicationsController < Projects::ApplicationControll before_action :authorize_create_cluster!, only: [:create] def create - application = Clusters::Applications::CreateService + Clusters::Applications::CreateService .new(@cluster, current_user, create_cluster_application_params) .execute(request) - Clusters::Applications::ScheduleInstallationService.new(project, current_user).execute(application) - head :no_content rescue Clusters::Applications::CreateService::InvalidApplicationError render_404 diff --git a/app/services/clusters/applications/create_service.rb b/app/services/clusters/applications/create_service.rb index d69e8612eed..55f917798de 100644 --- a/app/services/clusters/applications/create_service.rb +++ b/app/services/clusters/applications/create_service.rb @@ -24,6 +24,8 @@ module Clusters end application.save! + + Clusters::Applications::ScheduleInstallationService.new(application).execute end end diff --git a/app/services/clusters/applications/schedule_installation_service.rb b/app/services/clusters/applications/schedule_installation_service.rb index 4ead4f619c8..d75ba70c27e 100644 --- a/app/services/clusters/applications/schedule_installation_service.rb +++ b/app/services/clusters/applications/schedule_installation_service.rb @@ -2,8 +2,14 @@ module Clusters module Applications - class ScheduleInstallationService < ::BaseService - def execute(application) + class ScheduleInstallationService + attr_reader :application + + def initialize(application) + @application = application + end + + def execute application.make_scheduled! ClusterInstallAppWorker.perform_async(application.name, application.id) diff --git a/spec/services/applications/create_service_spec.rb b/spec/services/applications/create_service_spec.rb index 9c43b56744b..c8134087fa1 100644 --- a/spec/services/applications/create_service_spec.rb +++ b/spec/services/applications/create_service_spec.rb @@ -1,17 +1,14 @@ +# frozen_string_literal: true + require "spec_helper" describe ::Applications::CreateService do + include TestRequestHelpers + let(:user) { create(:user) } let(:params) { attributes_for(:application) } - let(:request) do - if Gitlab.rails5? - ActionController::TestRequest.new({ remote_ip: "127.0.0.1" }, ActionController::TestSession.new) - else - ActionController::TestRequest.new(remote_ip: "127.0.0.1") - end - end subject { described_class.new(user, params) } - it { expect { subject.execute(request) }.to change { Doorkeeper::Application.count }.by(1) } + it { expect { subject.execute(test_request) }.to change { Doorkeeper::Application.count }.by(1) } end diff --git a/spec/services/clusters/applications/create_service_spec.rb b/spec/services/clusters/applications/create_service_spec.rb index 58d9682602a..056db0c5486 100644 --- a/spec/services/clusters/applications/create_service_spec.rb +++ b/spec/services/clusters/applications/create_service_spec.rb @@ -3,21 +3,19 @@ require 'spec_helper' describe Clusters::Applications::CreateService do - let(:cluster) { create(:cluster) } + include TestRequestHelpers + + let(:cluster) { create(:cluster, :project, :provided_by_gcp) } let(:user) { create(:user) } let(:params) { { application: 'helm' } } let(:service) { described_class.new(cluster, user, params) } - let(:request) do - if Gitlab.rails5? - ActionController::TestRequest.new({ remote_ip: "127.0.0.1" }, ActionController::TestSession.new) - else - ActionController::TestRequest.new(remote_ip: "127.0.0.1") + describe '#execute' do + before do + allow(ClusterInstallAppWorker).to receive(:perform_async) end - end - describe '#execute' do - subject { service.execute(request) } + subject { service.execute(test_request) } it 'creates an application' do expect do @@ -27,6 +25,12 @@ 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 'jupyter application' do let(:params) do { @@ -35,6 +39,10 @@ describe Clusters::Applications::CreateService do } end + before do + allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) + end + it 'creates the application' do expect do subject diff --git a/spec/services/clusters/applications/schedule_installation_service_spec.rb b/spec/services/clusters/applications/schedule_installation_service_spec.rb index bca1e71bef2..21797edd533 100644 --- a/spec/services/clusters/applications/schedule_installation_service_spec.rb +++ b/spec/services/clusters/applications/schedule_installation_service_spec.rb @@ -10,14 +10,13 @@ describe Clusters::Applications::ScheduleInstallationService do expect(ClusterInstallAppWorker).not_to receive(:perform_async) count_before = count_scheduled - expect { service.execute(application) }.to raise_error(StandardError) + expect { service.execute }.to raise_error(StandardError) expect(count_scheduled).to eq(count_before) end end describe '#execute' do - let(:project) { double(:project) } - let(:service) { described_class.new(project, nil) } + let(:service) { described_class.new(application) } context 'when application is installable' do let(:application) { create(:clusters_applications_helm, :installable) } @@ -25,7 +24,7 @@ describe Clusters::Applications::ScheduleInstallationService do it 'make the application scheduled' do expect(ClusterInstallAppWorker).to receive(:perform_async).with(application.name, kind_of(Numeric)).once - expect { service.execute(application) }.to change { application.class.with_status(:scheduled).count }.by(1) + expect { service.execute }.to change { application.class.with_status(:scheduled).count }.by(1) end end diff --git a/spec/support/helpers/test_request_helpers.rb b/spec/support/helpers/test_request_helpers.rb new file mode 100644 index 00000000000..187a0e07891 --- /dev/null +++ b/spec/support/helpers/test_request_helpers.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module TestRequestHelpers + def test_request(remote_ip: '127.0.0.1') + if Gitlab.rails5? + ActionController::TestRequest.new({ remote_ip: remote_ip }, ActionController::TestSession.new) + else + ActionController::TestRequest.new(remote_ip: remote_ip) + end + end +end -- cgit v1.2.1