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

greenboot: warn users of missing disabled healthchecks #141

Merged
merged 1 commit into from
May 2, 2024

Conversation

djach7
Copy link
Contributor

@djach7 djach7 commented Apr 19, 2024

Resolves #139. Prints a warning in the logs when a user specifies a healthcheck to be disabled that cannot be found.

Addresses fedora-iot#139. Prints a warning in the logs when a user specifies a healthcheck to be disabled that cannot be found

Signed-off-by: djach7 <[email protected]>
Copy link

@mmartinv mmartinv left a comment

Choose a reason for hiding this comment

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

There's no need to run source_configuration_file before every function call, just make sure source_configuration_file is run before you run any other command (the function blocks are just declarations and are not run until invoked from other parts in the program).

I also think you are overcomplicating the whole thing. You just need to make sure all the disabled health-check scripts can be found in any of the script directories and that are executable. The pseudo code would be something like:

FOR DISABLED_CHECK in DISABLED_CHECKS
   FOUND=false
   FOR DIR IN SCRIPTS_DIRS 
       IF DISABLED_CHECK exists in DIR and is executable
           FOUND=true
  DONE
DONE
IF NOT FOUND  PRINT_MESSAGE.

Implementing the code above and running it before the case $1 in code block (and after sourcing the configuration file) should be enough.

@djach7
Copy link
Contributor Author

djach7 commented Apr 22, 2024

I don't disagree that I might've overcomplicated it. I think I ended up with the map solution because I'm comfortable with them and I was trying to figure out how to avoid duplicating looping through the script directories (since it's already done with the script runner). (Frankly I thought it was just kind of a cool way to do it too.) I'm not at all against revising it, though, and if that's what the consensus is then I can work on that.

And thank you for the info about source_configuration_file, I'll make that change then.

@runcom
Copy link
Member

runcom commented May 1, 2024

I'm torn as I like both approaches 😝 the O(1) lookup is a cool thing to see even if we're not really expecting tons of elements and/or searches anyway. Miguel's approach is definitely more concise and to the point w/o a global call to populate the map.
I guess I'm leaning towards what Miguel suggested but the idea of the map is certainly cool in general (where it probably makes more sense)

@7flying
Copy link
Member

7flying commented May 2, 2024

I like both approaches but I find using a map more elegant, and if we all agree that both things are OK I would lean towards what's already done to avoid pointless rewrites if there is no change in runtime performance.

@runcom
Copy link
Member

runcom commented May 2, 2024

certainly no performance hits given the amount of elements we're going to cycle on 👍

Copy link
Member

@7flying 7flying left a comment

Choose a reason for hiding this comment

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

LGTM, the other mentioned approach is also valid but this one is too and between one that is already done and a rewrite I favour the one that's already done

@7flying 7flying merged commit e99a3a2 into fedora-iot:main May 2, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn the user when a disabled health check cannot be found
4 participants