From 3c8df0c944f0b23f9ee8b6b08a0a355b00456dd9 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Fri, 12 Apr 2019 17:28:06 +1200
Subject: 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.
---
 app/models/clusters/concerns/application_status.rb |  5 -----
 .../check_uninstall_progress_service.rb            |  4 +++-
 spec/factories/clusters/applications/helm.rb       |  4 ----
 .../check_uninstall_progress_service_spec.rb       | 23 +++++++++++++++++++---
 .../cluster_application_status_shared_examples.rb  | 11 -----------
 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
 
-- 
cgit v1.2.1