Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

chris1984
Copy link
Member

@chris1984 chris1984 commented Jan 14, 2025

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.

Copy link
Collaborator

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -->

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
Copy link
Collaborator

parthaa commented Jan 14, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants