-
Notifications
You must be signed in to change notification settings - Fork 29
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
Conversation
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]>
There was a problem hiding this 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.
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 |
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 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. |
certainly no performance hits given the amount of elements we're going to cycle on 👍 |
There was a problem hiding this 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
Resolves #139. Prints a warning in the logs when a user specifies a healthcheck to be disabled that cannot be found.