diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2018-10-15 13:55:20 +0000 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2018-10-15 13:55:20 +0000 |
commit | 076398d898e6f4cd7d3c3992de2444404f3a11d0 (patch) | |
tree | a0e7c96085464aa017c8cf52bb6ff04ab2df372f | |
parent | 9bbad400a622f901f37c75fc639e0354c20a7f5c (diff) | |
parent | 504cbb27c1018bd702346eb6497e37372dd9f35a (diff) | |
download | gitlab-ce-076398d898e6f4cd7d3c3992de2444404f3a11d0.tar.gz |
Merge branch '34758-fix-code-reuse-clusters-applications_controller' into 'master'
Fix code reuse issue in Projects::Clusters::ApplicationController#index
See merge request gitlab-org/gitlab-ce!22182
8 files changed, 174 insertions, 43 deletions
diff --git a/app/controllers/projects/clusters/applications_controller.rb b/app/controllers/projects/clusters/applications_controller.rb index c356f8d2987..bcea96bce94 100644 --- a/app/controllers/projects/clusters/applications_controller.rb +++ b/app/controllers/projects/clusters/applications_controller.rb @@ -2,31 +2,20 @@ 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! - - Clusters::Applications::ScheduleInstallationService.new(project, current_user).execute(application) + Clusters::Applications::CreateService + .new(@cluster, current_user, create_cluster_application_params) + .execute(request) head :no_content + rescue Clusters::Applications::CreateService::InvalidApplicationError + render_404 rescue StandardError head :bad_request end - # rubocop: enable CodeReuse/ActiveRecord private @@ -34,18 +23,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..55f917798de --- /dev/null +++ b/app/services/clusters/applications/create_service.rb @@ -0,0 +1,68 @@ +# 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! + + Clusters::Applications::ScheduleInstallationService.new(application).execute + 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/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 new file mode 100644 index 00000000000..056db0c5486 --- /dev/null +++ b/spec/services/clusters/applications/create_service_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Clusters::Applications::CreateService do + 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) } + + describe '#execute' do + before do + allow(ClusterInstallAppWorker).to receive(:perform_async) + end + + subject { service.execute(test_request) } + + it 'creates an application' do + expect do + subject + + cluster.reload + 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 + { + application: 'jupyter', + hostname: 'example.com' + } + end + + before do + allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute) + 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 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 |