SAT-30393 - Skip recurring logic tasks if on prem is enabled

[chris1984]

This PR adds a skip to inventory/insights/generate report scheduled sync tasks if on_prem is enabled. This includes a change @jeremylenz has in his pr, once that is merged, I will remove it from here.

[jeremylenz]

I notice this approach just skips planning the scheduled sync tasks if we're using the local advisor engine. This should work fine, but you'd still see actions in Dynflow that are empty and successful.

Another option is to avoid scheduling them at all here -->

foreman_rh_cloud/lib/foreman_rh_cloud/engine.rb

Line 168 in dc80828

ForemanRhCloud::Engine.register_scheduled_task(InsightsCloud::Async::InsightsScheduledSync, '0 0 * * *')

But that may be a bit less safe because of where and when ForemanRhCloud.with_local_advisor_engine? is defined..? I'm a bit unclear on if that will work, but if it does it seems cleaner to me.

@ShimShtein @parthaa thoughts?

[parthaa]

I notice this approach just skips planning the scheduled sync tasks if we're using the local advisor engine. This should work fine, but you'd still see actions in Dynflow that are empty and successful.

While I agree you'd rather not have tasks scheduled if not needed. I am thinking in the future when we may want the hybrid option. I have a feeling we are better of scheduling but skipping.

[parthaa]

Hmm how about a seed file that has this logic so that we disable the recurring task rather than just returning.

if ForemanRhCloud.with_local_advisor_engine?
  ::ForemanTasks::RecurringLogic.joins(:tasks).where(tasks: { label: ["InventorySync::Async::InventoryScheduledSync", "ForemanInventoryUpload::Async::GenerateAllReportsJob", "InsightsCloud::Async::InsightsScheduledSync"] }, state: "active").distinct(:id).each {|logic| logic.update!(enabled: false)}
end

[jeremylenz]

Do we support turning local off after it's on? if so, we may want to do it both ways (e.g. enable or disable the logic.)

[chris1984]

Do we support turning local off after it's on? if so, we may want to do it both ways (e.g. enable or disable the logic.)

We do support that with the installer flag setting it to false, we can't do this in a seed file then. We would have to do something in the engine rb right @parthaa

[jeremylenz]

Side question: Why disable the insights scheduled sync one? Wouldn't that just go to the local iop-advisor-engine?

[chris1984]

Side question: Why disable the insights scheduled sync one? Wouldn't that just go to the local iop-advisor-engine?

Fair point, I was just doing it since the engine writes to the api and we modify the DB right then. There is no harm in having it sync to the engine. Will take that out of the pr.

[jeremylenz]

the engine writes to the api and we modify the DB right then. There is no harm in having it sync to the engine.

If it's also not doing anything useful, that'd be an argument for disabling it as well.

[parthaa]

APJ. I thought about disabling tasks when advisor is on but feel its not worth the complexity and having to deal with enabling tasks when user decides to turn the advisor off.

[chris1984]

Working correctly now

[parthaa]

Works well. ACK

[parthaa]

Recommended patch

diff --git a/test/jobs/inventory_scheduled_sync_test.rb b/test/jobs/inventory_scheduled_sync_test.rb
index 338d900..48a2f88 100644
--- a/test/jobs/inventory_scheduled_sync_test.rb
+++ b/test/jobs/inventory_scheduled_sync_test.rb
@@ -15,13 +15,12 @@ class InventoryScheduledSyncTest < ActiveSupport::TestCase
   end
 
   test 'Skips execution if with_local_advisor_engine? is true' do
-    Setting[:allow_auto_inventory_upload] = true
-
     ForemanRhCloud.stubs(:with_local_advisor_engine?).returns(true)
-
     InventorySync::Async::InventoryScheduledSync.any_instance.expects(:plan_org_sync).never
 
-    ForemanTasks.sync_task(InventorySync::Async::InventoryScheduledSync)
+    task = ForemanTasks.sync_task(InventorySync::Async::InventoryScheduledSync)
+    status = task.output[:status].to_s
+    assert_match(/Foreman is configured with the use_local_advisor_engine option/, status)
   end
 
   test 'Skips execution if auto upload is disabled' do