Skip to content
This repository has been archived by the owner on May 30, 2022. It is now read-only.

Push catalog info after the checks #804

Merged

Conversation

dottorblaster
Copy link
Contributor

Right now if the database goes bananas we don't repopulate the table containing the catalog data. This PR attempts to address the issue making that happen after the checks ran.

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Hey @dottorblaster ,
Thanks! Unfortunately this will add some overload to the checks execution, but well it is what it is. Hopefully in the future we can add some kind of logic to the runner to get commands from the web itself (or we might even remove the runner at all, who knows hehe).

I recommend that once we add the metadata execution in the loop, we could remove the initial execution (line 87), so we only execute meta and checks within the loop. And I would execute the metadata and checks in this order.

runner/runner.go Outdated
return
}

checkRunner.RunPlaybook()

if err = metaRunner.RunPlaybook(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the order a bit strange. Shouldn't we run and publish the catalog information before running the checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree on the order, I moved it on top!

@dottorblaster
Copy link
Contributor Author

@arbulu89 100% agree about the order of events, about the removal of the first run on boot I honestly don't know, because I prefer to run this also at the boot to ensure the user doesn't see an empty screen for too long at first. What do you think?

@diegoakechi
Copy link
Contributor

@dottorblaster I agree with running this on boot. Considering the expansion of the catalog checks and the time it takes to run, I see as natural that the execution frequency will become longer (hourly, eventually daily), so the first execution populates the initial status.

@arbulu89
Copy link
Contributor

@arbulu89 100% agree about the order of events, about the removal of the first run on boot I honestly don't know, because I prefer to run this also at the boot to ensure the user doesn't see an empty screen for too long at first. What do you think?

I don't think removing this would change anything. This is the current logic:

run_meta()
while true:
   run_meta()
   run_checks()

If we remove the first run_meta(), we will still have 1 run_meta executed in the 1st place. Or am I missing something?

@dottorblaster
Copy link
Contributor Author

@arbulu89 I was assuming that the first tick executed after the first interval, sorry about that 😅 I just noticed that the first tick runs immediately. I'm going to remove the first call then 😄

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Thanks @dottorblaster ! Here we go!!!

@dottorblaster dottorblaster merged commit f658fd4 into trento-project:main Feb 17, 2022
@dottorblaster dottorblaster deleted the make-catalog-resilient branch February 17, 2022 10:17
@dottorblaster dottorblaster added the bug Something isn't working label Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

None yet

3 participants