diff options
author | Thong Kuah <tkuah@gitlab.com> | 2019-04-12 17:28:06 +1200 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2019-04-29 22:55:11 -0700 |
commit | 3c8df0c944f0b23f9ee8b6b08a0a355b00456dd9 (patch) | |
tree | e17c11245edf069af55f97c3b9df1544c33db57a | |
parent | 938e90f47288901790a96c50a8c0dfa2b7eab137 (diff) | |
download | gitlab-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.
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 |