Merge pull request #18283 from d-m-u/fixing_subservice_task_creation_…

…for_bundled_service_bundles

Fixing subservice task creation for service bundles

app/models/miq_request_task.rb

@@ -77,7 +77,7 @@ def update_request_status
     req_status = status.slice('Error', 'Timeout', 'Warn').keys.first || 'Ok'
 
     if req_state == "finished" && state != "finished"
-      req_state = (req_status == 'Ok') ? 'provisioned' : "finished"
+      req_state = req_status == 'Ok' ? completed_state : "finished"
       $log.info("Child tasks finished but current task still processing. Setting state to: [#{req_state}]...")
     end
 
@@ -94,6 +94,10 @@ def update_request_status
     update_and_notify_parent(:state => req_state, :status => req_status, :message => display_message(msg))
   end
 
+  def completed_state
+    "provisioned"
+  end
+
   def execute_callback(state, message, _result)
     unless state.to_s.downcase == "ok"
       update_and_notify_parent(:state => "finished", :status => "Error", :message => "Error: #{message}")

app/models/miq_retire_task.rb

@@ -86,6 +86,10 @@ def mark_pending_items_as_finished
     end
   end
 
+  def completed_state
+    "retired"
+  end
+
   def self.display_name(number = 1)
     n_('Retire Task', 'Retire Tasks', number)
   end

app/models/service.rb

@@ -152,6 +152,10 @@ def request_type
     'service_reconfigure'
   end
 
+  def retireable?
+    parent.present? ? true : type.present?
+  end
+
   alias root_service root
   alias services children
   alias direct_service_children children
@@ -218,10 +222,6 @@ def composite?
     children.present?
   end
 
-  def retireable?
-    type.present?
-  end
-
   def atomic?
     children.empty?
   end

app/models/service_retire_task.rb

@@ -16,7 +16,9 @@ def update_and_notify_parent(*args)
   end
 
   def task_finished
-    update_attributes(:status => status == 'Ok' ? 'Completed' : 'Failed')
+    if status != 'Ok'
+      update_attributes(:status => 'Error')
+    end
   end
 
   def task_active
@@ -26,7 +28,7 @@ def task_active
   def after_request_task_create
     update_attributes(:description => get_description)
     parent_svc = Service.find_by(:id => options[:src_ids])
-    _log.info("- creating service tasks for service <#{self.class.name}:#{id}>")
+    _log.info("- creating service subtasks for service task <#{self.class.name}:#{id}>, service <#{parent_svc.id}>")
     create_retire_subtasks(parent_svc, self)
   end
 
@@ -42,22 +44,22 @@ def create_retire_subtasks(parent_service, parent_task)
       # Initial Options[:dialog] to an empty hash so we do not pass down dialog values to child services tasks
       nh['options'][:dialog] = {}
       new_task = create_task(svc_rsc, parent_service, nh, parent_task)
-      create_retire_subtasks(svc_rsc.resource, new_task) if svc_rsc.resource.kind_of?(Service)
       new_task.after_request_task_create
       miq_request.miq_request_tasks << new_task
       new_task.tap(&:deliver_to_automate)
     end.compact!
   end
 
   def create_task(svc_rsc, parent_service, nh, parent_task)
-    (svc_rsc.resource.type.demodulize + "RetireTask").constantize.new(nh).tap do |task|
+    resource_type = svc_rsc.resource.type.presence || "Service"
+    (resource_type.demodulize + "RetireTask").constantize.new(nh).tap do |task|
       task.options.merge!(
-        :src_id              => svc_rsc.resource.id,
+        :src_ids             => [svc_rsc.resource.id],
         :service_resource_id => svc_rsc.id,
         :parent_service_id   => parent_service.id,
         :parent_task_id      => parent_task.id,
       )
-      task.request_type = svc_rsc.resource.type.demodulize.underscore.downcase + "_retire"
+      task.request_type = resource_type.demodulize.underscore.downcase + "_retire"
       task.source = svc_rsc.resource
       parent_task.miq_request_tasks << task
       task.save!

spec/models/mixins/ci_feature_mixin_spec.rb

@@ -19,23 +19,10 @@
       expect(service.service_resources.first.resource.retireable?).to eq(false)
     end
 
-    context "service" do
-      context "with type" do
-        let(:service1) { FactoryBot.create(:service_ansible_tower, :type => ServiceAnsibleTower) }
-        it "is retireable" do
-          FactoryBot.create(:service_resource, :service => service, :resource => service1)
+    it "service is retireable" do
+      FactoryBot.create(:service_resource, :service => service, :resource => FactoryBot.create(:service_ansible_tower, :type => ServiceAnsibleTower))
 
-          expect(service.service_resources.first.resource.retireable?).to eq(true)
-        end
-      end
-
-      context "without type" do
-        it "is not retireable" do
-          FactoryBot.create(:service_resource, :service => service, :resource => FactoryBot.create(:service))
-
-          expect(service.service_resources.first.resource.retireable?).to eq(false)
-        end
-      end
+      expect(service.service_resources.first.resource.retireable?).to eq(true)
     end
   end
 end

spec/models/service_retire_task_spec.rb

@@ -8,17 +8,6 @@
   let(:approver) { FactoryBot.create(:user_miq_request_approver) }
   let(:zone) { FactoryBot.create(:zone, :name => "fred") }
 
-  shared_context "service_bundle" do
-    let(:zone) { FactoryBot.create(:small_environment) }
-    let(:service_c1) { FactoryBot.create(:service, :service => service) }
-
-    before do
-      allow(MiqServer).to receive(:my_server).and_return(zone.miq_servers.first)
-      @miq_request = FactoryBot.create(:service_retire_request, :requester => user)
-      @miq_request.approve(approver, reason)
-    end
-  end
-
   it "should initialize properly" do
     expect(service_retire_task).to have_attributes(:state => 'pending', :status => 'Ok')
   end
@@ -36,7 +25,7 @@
       it "should call task_finished" do
         service_retire_task.update_and_notify_parent(:state => "finished", :status => "Ok", :message => "yabadabadoo")
 
-        expect(service_retire_task.status).to eq("Completed")
+        expect(service_retire_task.status).to eq("Ok")
       end
     end
   end
@@ -58,59 +47,51 @@
         miq_request.approve(approver, reason)
       end
 
-      it "creates subtask" do
-        resource = FactoryBot.create(:service_resource, :resource_type => "VmOrTemplate", :service_id => service.id, :resource_id => vm.id)
-        service.service_resources << resource
-        service_retire_task.after_request_task_create
+      context "resource lacks type" do
+        it "creates service retire subtask" do
+          resource = FactoryBot.create(:service_resource, :resource_type => nil, :service_id => service.id, :resource_id => vm.id)
+          service.service_resources << resource
+          service_retire_task.after_request_task_create
 
-        expect(service_retire_task.description).to eq("Service Retire for: #{service.name} - ")
-        expect(VmRetireTask.count).to eq(1)
+          expect(service_retire_task.description).to eq("Service Retire for: #{service.name} - ")
+          expect(ServiceRetireTask.count).to eq(1)
+        end
+      end
+
+      context "resource has type" do
+        it "creates vm retire subtask" do
+          resource = FactoryBot.create(:service_resource, :resource_type => "VmOrTemplate", :service_id => service.id, :resource_id => vm.id)
+          service.service_resources << resource
+          service_retire_task.after_request_task_create
+
+          expect(service_retire_task.description).to eq("Service Retire for: #{service.name} - ")
+          expect(VmRetireTask.count).to eq(1)
+        end
       end
     end
 
     context "bundled service retires all children" do
-      include_context "service_bundle"
-      let(:vm1) { FactoryBot.create(:vm_vmware) }
-      let(:service_c2) { FactoryBot.create(:service, :service => service_c1) }
+      let(:service_c1) { FactoryBot.create(:service) }
 
       before do
-        service_c1 << vm
-        service_c2 << vm1
-        service.save
-        service_c1.save
-        service_c2.save
+        service.add_resource!(service_c1)
+        service.add_resource!(FactoryBot.create(:service_template))
+        @miq_request = FactoryBot.create(:service_retire_request, :requester => user)
+        @miq_request.approve(approver, reason)
+        @service_retire_task = FactoryBot.create(:service_retire_task, :source => service, :miq_request => @miq_request, :options => {:src_ids => [service.id] })
       end
 
-      it "creates subtask" do
-        @service_retire_task = FactoryBot.create(:service_retire_task, :source => service, :miq_request_task_id => nil, :miq_request_id => @miq_request.id, :options => {:src_ids => [service.id] })
-        service.service_resources << FactoryBot.create(:service_resource, :resource_type => "VmOrTemplate", :service_id => service_c1.id, :resource_id => vm.id)
-        service.service_resources << FactoryBot.create(:service_resource, :resource_type => "VmOrTemplate", :service_id => service_c1.id, :resource_id => vm1.id)
-        service.service_resources << FactoryBot.create(:service_resource, :resource_type => "Service", :service_id => service_c1.id, :resource_id => service_c1.id)
-        service.service_resources << FactoryBot.create(:service_resource, :resource_type => "ServiceTemplate", :service_id => service_c1.id, :resource_id => service_c1.id)
-
+      it "creates subtask for services but not templates" do
         @service_retire_task.after_request_task_create
-        expect(VmRetireTask.count).to eq(2)
-        expect(VmRetireTask.all.pluck(:message)).to eq(["Automation Starting", "Automation Starting"])
-        expect(ServiceRetireTask.count).to eq(1)
+
+        expect(ServiceRetireTask.count).to eq(2)
         expect(ServiceRetireRequest.count).to eq(1)
       end
 
       it "doesn't creates subtask for ServiceTemplates" do
-        @service_retire_task = FactoryBot.create(:service_retire_task, :source => service, :miq_request_task_id => nil, :miq_request_id => @miq_request.id, :options => {:src_ids => [service.id] })
-        service.service_resources << FactoryBot.create(:service_resource, :resource_type => "ServiceTemplate", :service_id => service_c1.id, :resource_id => service_c1.id)
-
         @service_retire_task.after_request_task_create
-        expect(ServiceRetireTask.count).to eq(1)
-        expect(ServiceRetireRequest.count).to eq(1)
-      end
-
-      it "doesn't creates subtask for service resources whose resources are nil" do
-        @service_retire_task = FactoryBot.create(:service_retire_task, :source => service, :miq_request_task_id => nil, :miq_request_id => @miq_request.id, :options => {:src_ids => [service.id] })
-        service.service_resources << FactoryBot.create(:service_resource, :resource_type => "ServiceTemplate", :service_id => service_c1.id, :resource => nil)
 
-        @service_retire_task.after_request_task_create
-        expect(ServiceRetireTask.count).to eq(1)
-        expect(ServiceRetireRequest.count).to eq(1)
+        expect(ServiceRetireTask.count).to eq(2)
       end
     end
   end

spec/models/service_spec.rb

@@ -675,6 +675,31 @@
     end
   end
 
+  describe "#retireable?" do
+    let(:service_with_type) { FactoryBot.create(:service, :type => "thing") }
+    let(:service_without_type) { FactoryBot.create(:service, :type => nil) }
+    let(:service_with_parent) { FactoryBot.create(:service, :service => FactoryBot.create(:service)) }
+    context "with no parent" do
+      context "with type" do
+        it "true" do
+          expect(service_with_type.retireable?).to be(true)
+        end
+      end
+
+      context "without type" do
+        it "checks type presence" do
+          expect(service_without_type.retireable?).to be(false)
+        end
+      end
+    end
+
+    context "with parent" do
+      it "true" do
+        expect(service_with_parent.retireable?).to be(true)
+      end
+    end
+  end
+
   describe "#retired" do
     it "defaults to false" do
       service = described_class.new

spec/models/vm_retire_task_spec.rb

@@ -9,6 +9,13 @@
     expect(vm_retire_task).to have_attributes(:state => 'pending', :status => 'Ok')
   end
 
+  describe "#after_request_task_create" do
+    it "should set the task description" do
+      vm_retire_task.after_request_task_create
+      expect(vm_retire_task.description).to eq("VM Retire for: #{vm.name} - ")
+    end
+  end
+
   describe "deliver_to_automate" do
     before do
       MiqRegion.seed