summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorThong Kuah <tkuah@gitlab.com>2019-04-12 17:28:06 +1200
committerStan Hu <stanhu@gmail.com>2019-04-29 22:55:11 -0700
commit3c8df0c944f0b23f9ee8b6b08a0a355b00456dd9 (patch)
treee17c11245edf069af55f97c3b9df1544c33db57a
parent938e90f47288901790a96c50a8c0dfa2b7eab137 (diff)
downloadgitlab-ce-3c8df0c944f0b23f9ee8b6b08a0a355b00456dd9.tar.gz
Destroy app on successful uninstallation
Rescue and put into :uninstall_errored if something goes wrong while destroying, which can happen. I think it is safe to expose the full error message from the destroy error. Remove the :uninstalled state as no longer used.
-rw-r--r--app/models/clusters/concerns/application_status.rb5
-rw-r--r--app/services/clusters/applications/check_uninstall_progress_service.rb4
-rw-r--r--spec/factories/clusters/applications/helm.rb4
-rw-r--r--spec/services/clusters/applications/check_uninstall_progress_service_spec.rb23
-rw-r--r--spec/support/shared_examples/models/cluster_application_status_shared_examples.rb11
5 files changed, 23 insertions, 24 deletions
diff --git a/app/models/clusters/concerns/application_status.rb b/app/models/clusters/concerns/application_status.rb
index 16679a21e64..54a3dda6d75 100644
--- a/app/models/clusters/concerns/application_status.rb
+++ b/app/models/clusters/concerns/application_status.rb
@@ -27,7 +27,6 @@ module Clusters
state :update_errored, value: 6
state :uninstalling, value: 7
state :uninstall_errored, value: 8
- state :uninstalled, value: 9
event :make_scheduled do
transition [:installable, :errored, :installed, :updated, :update_errored, :uninstall_errored] => :scheduled
@@ -60,10 +59,6 @@ module Clusters
transition [:scheduled] => :uninstalling
end
- event :make_uninstalled do
- transition [:uninstalling] => :uninstalled
- end
-
before_transition any => [:scheduled] do |app_status, _|
app_status.status_reason = nil
end
diff --git a/app/services/clusters/applications/check_uninstall_progress_service.rb b/app/services/clusters/applications/check_uninstall_progress_service.rb
index 2a2594a30c8..efb2bb1b67d 100644
--- a/app/services/clusters/applications/check_uninstall_progress_service.rb
+++ b/app/services/clusters/applications/check_uninstall_progress_service.rb
@@ -23,7 +23,9 @@ module Clusters
private
def on_success
- app.make_uninstalled!
+ app.destroy!
+ rescue StandardError => e
+ app.make_errored!("Application uninstalled but failed to destroy: #{e.message}")
ensure
remove_installation_pod
end
diff --git a/spec/factories/clusters/applications/helm.rb b/spec/factories/clusters/applications/helm.rb
index ac230950fce..22a0888947e 100644
--- a/spec/factories/clusters/applications/helm.rb
+++ b/spec/factories/clusters/applications/helm.rb
@@ -49,10 +49,6 @@ FactoryBot.define do
status_reason 'something went wrong'
end
- trait :uninstalled do
- status 9
- end
-
trait :timeouted do
installing
updated_at { ClusterWaitForAppInstallationWorker::TIMEOUT.ago }
diff --git a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb
index ccae7fd133f..084f29d9d2d 100644
--- a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb
+++ b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb
@@ -56,13 +56,30 @@ describe Clusters::Applications::CheckUninstallProgressService do
service.execute
end
- it 'make the application installed' do
+ it 'destroys the application' do
expect(worker_class).not_to receive(:perform_in)
service.execute
+ expect(application).to be_destroyed
+ end
+
+ context 'an error occurs while destroying' do
+ before do
+ expect(application).to receive(:destroy!).once.and_raise("destroy failed")
+ end
+
+ it 'still removes the installation POD' do
+ expect(service).to receive(:remove_installation_pod).once
- expect(application).to be_uninstalled
- expect(application.status_reason).to be_nil
+ service.execute
+ end
+
+ it 'makes the application uninstall_errored' do
+ service.execute
+
+ expect(application).to be_uninstall_errored
+ expect(application.status_reason).to eq('Application uninstalled but failed to destroy: destroy failed')
+ end
end
end
diff --git a/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb b/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb
index c56b148cb8c..233b9db8f7b 100644
--- a/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb
+++ b/spec/support/shared_examples/models/cluster_application_status_shared_examples.rb
@@ -192,16 +192,6 @@ shared_examples 'cluster application status specs' do |application_name|
expect(subject).to be_uninstalling
end
end
-
- describe '#make_uninstalled' do
- subject { create(application_name, :uninstalling) }
-
- it 'is uninstalled' do
- subject.make_uninstalled!
-
- expect(subject).to be_uninstalled
- end
- end
end
describe '#available?' do
@@ -219,7 +209,6 @@ shared_examples 'cluster application status specs' do |application_name|
:update_errored | false
:uninstalling | false
:uninstall_errored | false
- :uninstalled | false
:timeouted | false
end