From c10452d285134b7eac63551f5a09606fb5d4f5e0 Mon Sep 17 00:00:00 2001 From: Chris Baumbauer Date: Sat, 3 Nov 2018 10:48:48 -0700 Subject: [PATCH] Resolve issues raised by Kamil --- .../clusters/stores/clusters_store.js | 4 +- app/models/clusters/applications/knative.rb | 26 ++-- lib/gitlab/kubernetes/helm/install_command.rb | 19 +-- .../clusters/stores/clusters_store_spec.js | 2 +- .../clusters/applications/knative_spec.rb | 128 ++++++++++++++++-- spec/models/clusters/cluster_spec.rb | 2 +- .../applications/create_service_spec.rb | 33 +++++ 7 files changed, 176 insertions(+), 38 deletions(-) diff --git a/app/assets/javascripts/clusters/stores/clusters_store.js b/app/assets/javascripts/clusters/stores/clusters_store.js index e9c580c5dfa..26566b60ba6 100644 --- a/app/assets/javascripts/clusters/stores/clusters_store.js +++ b/app/assets/javascripts/clusters/stores/clusters_store.js @@ -52,7 +52,7 @@ export default class ClusterStore { statusReason: null, requestStatus: null, requestReason: null, - hostname: '' + hostname: null }, }, }; @@ -102,7 +102,7 @@ export default class ClusterStore { ? `jupyter.${this.state.applications.ingress.externalIp}.nip.io` : ''); } else if (appId === KNATIVE) { - this.state.applications.knative.hostname = serverAppEntry.hostname ? serverAppEntry.hostname : ''; + this.state.applications.knative.hostname = serverAppEntry.hostname ? serverAppEntry.hostname : null; } }); } diff --git a/app/models/clusters/applications/knative.rb b/app/models/clusters/applications/knative.rb index 76d23870f4c..ecb42f6011e 100644 --- a/app/models/clusters/applications/knative.rb +++ b/app/models/clusters/applications/knative.rb @@ -18,18 +18,19 @@ module Clusters include ::Clusters::Concerns::ApplicationData default_value_for :version, VERSION - default_value_for :hostname, '' + default_value_for :hostname, nil + + validates :hostname, presence: true def chart 'knative/knative' end - def install_command - args = [] - if !hostname.nil? && !hostname.eql?('') - args = ["domain=" + hostname] - end + def values + content_values.to_yaml + end + def install_command Gitlab::Kubernetes::Helm::InstallCommand.new( name: name, version: VERSION, @@ -37,7 +38,6 @@ module Clusters chart: chart, files: files, repository: REPOSITORY, - setargs: args, script: install_script ) end @@ -46,8 +46,16 @@ module Clusters ['/usr/bin/kubectl', 'apply', '-f', ISTIO_CRDS] end - def client - cluster&.platform_kubernetes&.kubeclient&.core_client + private + + def content_values + YAML.load_file(chart_values_file).deep_merge!(knative_configs) + end + + def knative_configs + { + "domain" => hostname + } end end end diff --git a/lib/gitlab/kubernetes/helm/install_command.rb b/lib/gitlab/kubernetes/helm/install_command.rb index aa5fddf1b44..09a4e494b64 100644 --- a/lib/gitlab/kubernetes/helm/install_command.rb +++ b/lib/gitlab/kubernetes/helm/install_command.rb @@ -4,16 +4,15 @@ module Gitlab class InstallCommand include BaseCommand - attr_reader :name, :files, :chart, :version, :repository, :setargs, :script + attr_reader :name, :files, :chart, :version, :repository, :script - def initialize(name:, chart:, files:, rbac:, version: nil, repository: nil, setargs: nil, script: nil) + def initialize(name:, chart:, files:, rbac:, version: nil, repository: nil, script: nil) @name = name @chart = chart @version = version @rbac = rbac @files = files @repository = repository - @setargs = setargs @script = script end @@ -61,15 +60,13 @@ module Gitlab name_flag = ['--name', name] namespace_flag = ['--namespace', Gitlab::Kubernetes::Helm::NAMESPACE] value_flag = ['-f', "/data/helm/#{name}/config/values.yaml"] - args_flag = optional_install_set_args_flag name_flag + optional_tls_flags + optional_version_flag + optional_rbac_create_flag + namespace_flag + - value_flag + - args_flag + value_flag end def optional_rbac_create_flag @@ -80,16 +77,6 @@ module Gitlab %w[--set rbac.create=true,rbac.enabled=true] end - def optional_install_set_args_flag - return [] unless setargs - - args = [] - setargs.each do |s| - args.push("--set", s) - end - args - end - def optional_version_flag return [] unless version diff --git a/spec/javascripts/clusters/stores/clusters_store_spec.js b/spec/javascripts/clusters/stores/clusters_store_spec.js index 0586724d451..34ed36afa5b 100644 --- a/spec/javascripts/clusters/stores/clusters_store_spec.js +++ b/spec/javascripts/clusters/stores/clusters_store_spec.js @@ -106,7 +106,7 @@ describe('Clusters Store', () => { statusReason: mockResponseData.applications[5].status_reason, requestStatus: null, requestReason: null, - hostname: '', + hostname: null, }, }, }); diff --git a/spec/models/clusters/applications/knative_spec.rb b/spec/models/clusters/applications/knative_spec.rb index 78124c13be9..09b023adcd3 100644 --- a/spec/models/clusters/applications/knative_spec.rb +++ b/spec/models/clusters/applications/knative_spec.rb @@ -1,18 +1,129 @@ require 'rails_helper' describe Clusters::Applications::Knative do - let(:knative) { create(:clusters_applications_knative) } + let(:knative) { create(:clusters_applications_knative, hostname: 'example.com') } include_examples 'cluster application core specs', :clusters_applications_knative - include_examples 'cluster application status specs', :clusters_applications_knative + + describe '#status' do + let(:cluster) { create(:cluster, :provided_by_gcp) } + + subject { described_class.new(cluster: cluster) } + + it 'sets a default status' do + expect(subject.status_name).to be(:not_installable) + end + + context 'when application helm is scheduled' do + before do + create(:clusters_applications_helm, :scheduled, cluster: cluster) + end + + it 'defaults to :not_installable' do + expect(subject.status_name).to be(:not_installable) + end + end + + context 'when application is scheduled' do + before do + create(:clusters_applications_helm, :installed, cluster: cluster) + end + + it 'sets a default status' do + expect(subject.status_name).to be(:installable) + end + end + end + + describe 'status state machine' do + describe '#make_installing' do + subject { create(:clusters_applications_knative, :scheduled, hostname: 'example.com') } + + it 'is installing' do + subject.make_installing! + + expect(subject).to be_installing + end + end + + describe '#make_installed' do + subject { create(:clusters_applications_knative, :installing, hostname: 'example.com') } + + it 'is installed' do + subject.make_installed + + expect(subject).to be_installed + end + end + + describe '#make_errored' do + subject { create(:clusters_applications_knative, :installing, hostname: 'example.com') } + let(:reason) { 'some errors' } + + it 'is errored' do + subject.make_errored(reason) + + expect(subject).to be_errored + expect(subject.status_reason).to eq(reason) + end + end + describe '#make_scheduled' do + subject { create(:clusters_applications_knative, :installable, hostname: 'example.com') } + + it 'is scheduled' do + subject.make_scheduled + + expect(subject).to be_scheduled + end + + describe 'when was errored' do + subject { create(:clusters_applications_knative, :errored, hostname: 'example.com') } + + it 'clears #status_reason' do + expect(subject.status_reason).not_to be_nil + + subject.make_scheduled! + + expect(subject.status_reason).to be_nil + end + end + end + end + + describe '#available?' do + using RSpec::Parameterized::TableSyntax + + where(:trait, :available) do + :not_installable | false + :installable | false + :scheduled | false + :installing | false + :installed | true + :updating | false + :updated | true + :errored | false + :update_errored | false + :timeouted | false + end + + with_them do + subject { build(:clusters_applications_knative, trait) } + + if params[:available] + it { is_expected.to be_available } + else + it { is_expected.not_to be_available } + end + end + end describe '.installed' do subject { described_class.installed } - let!(:cluster) { create(:clusters_applications_knative, :installed) } + let!(:cluster) { create(:clusters_applications_knative, :installed, hostname: 'example.com') } before do - create(:clusters_applications_knative, :errored) + create(:clusters_applications_knative, :errored, hostname: 'example.com') end it { is_expected.to contain_exactly(cluster) } @@ -24,7 +135,7 @@ describe Clusters::Applications::Knative do end context 'application install previously errored with older version' do - let(:application) { create(:clusters_applications_knative, :scheduled, version: '0.1.3') } + let(:application) { create(:clusters_applications_knative, :scheduled, version: '0.1.3', hostname: 'example.com') } it 'updates the application version' do expect(application.reload.version).to eq('0.1.3') @@ -35,10 +146,10 @@ describe Clusters::Applications::Knative do describe '#make_installed' do subject { described_class.installed } - let!(:cluster) { create(:clusters_applications_knative, :installed) } + let!(:cluster) { create(:clusters_applications_knative, :installed, hostname: 'example.com') } before do - create(:clusters_applications_knative, :errored) + create(:clusters_applications_knative, :errored, hostname: 'example.com') end it { is_expected.to contain_exactly(cluster) } @@ -54,11 +165,10 @@ describe Clusters::Applications::Knative do expect(subject.chart).to eq('knative/knative') expect(subject.version).to eq('0.1.3') expect(subject.files).to eq(knative.files) - expect(subject.setargs).to eq([]) end context 'application failed to install previously' do - let(:knative) { create(:clusters_applications_knative, :errored, version: 'knative') } + let(:knative) { create(:clusters_applications_knative, :errored, version: 'knative', hostname: 'example.com') } it 'should be initialized with the locked version' do expect(subject.version).to eq('0.1.3') diff --git a/spec/models/clusters/cluster_spec.rb b/spec/models/clusters/cluster_spec.rb index 10b9ca1a778..176eab3ce3a 100644 --- a/spec/models/clusters/cluster_spec.rb +++ b/spec/models/clusters/cluster_spec.rb @@ -314,7 +314,7 @@ describe Clusters::Cluster do let!(:prometheus) { create(:clusters_applications_prometheus, cluster: cluster) } let!(:runner) { create(:clusters_applications_runner, cluster: cluster) } let!(:jupyter) { create(:clusters_applications_jupyter, cluster: cluster) } - let!(:knative) { create(:clusters_applications_knative, cluster: cluster) } + let!(:knative) { create(:clusters_applications_knative, cluster: cluster, hostname: 'example.com') } it 'returns a list of created applications' do is_expected.to contain_exactly(helm, ingress, prometheus, runner, jupyter, knative) diff --git a/spec/services/clusters/applications/create_service_spec.rb b/spec/services/clusters/applications/create_service_spec.rb index 056db0c5486..a9985133b93 100644 --- a/spec/services/clusters/applications/create_service_spec.rb +++ b/spec/services/clusters/applications/create_service_spec.rb @@ -67,5 +67,38 @@ describe Clusters::Applications::CreateService do expect { subject }.to raise_error(Clusters::Applications::CreateService::InvalidApplicationError) end end + + context 'knative application' do + let(:params) do + { + application: 'knative', + 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_knative) + end + + it 'sets the hostname' do + expect(subject.hostname).to eq('example.com') + 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