fix:fail early for any required script failure #198
Conversation
say-paul
force-pushed
the
required_failed_early
branch
from
a5d67cc to
8500356
Compare
say-paul
force-pushed
the
required_failed_early
branch
from
8500356 to
6b5427e
Compare
say-paul
force-pushed
the
required_failed_early
branch
from
c30ca82 to
1e6f7c2
Compare
|
I just wanted to note that having all tests run and not breaking after the first failure is actually a feature, not a bug, as in case of multiple failures each one will be reported (otherwise following failures will only be uncovered when the first one is fixed and the device is booted again). Maybe that isn't needed, but OTOH this could be made optional with e.g. an |
| if [[ "$required_hc_failed" == true && "$mode" == "strict" ]]; then | ||
| echo "<5>'$(basename "$script")' was skipped due to a previous failure" | ||
| elif is_disabled "$(basename "$script")"; then | ||
| echo "<5>'$(basename "$script")' was skipped, as specified in config" |
There was a problem hiding this comment.
can we pls update warning /error msg on failure on line 48 & 52.
"relaxed")
echo "<2>$failure_msg. Continuing..." >&2 -> continuing since relaxed mode or something
and for
"strict")
required_hc_failed=true
echo "<0>$failure_msg. Continuing..." >&2 -> Exiting due to required script failure
There was a problem hiding this comment.
This is internal to greenboot as when strict/relaxed is used , it logs for users in terms of required/wanted/green/red scripts adding relax/strict will add to the confusion.
| @@ -82,10 +64,12 @@ case "$1" in | |||
| "check") | |||
| rc=0 | |||
| for health_check_path in "${SCRIPTS_CHECK_PATHS[@]}"; do | |||
| script_runner "$health_check_path/required.d" "strict" "Running Required Health Check Scripts..." || rc=1 | |||
| script_runner "$health_check_path/required.d" "strict" "Running Required Health Check Scripts..." || { | |||
There was a problem hiding this comment.
I understand the logic part here but why do we need OR (||) operator here?
can we not just use something like
If return val of script_runner !=0 i.e failure then break .
(or is it the bash thing !?)
There was a problem hiding this comment.
both approach yields same result, I don't see what if..then .. offers more.
right, this could be enhanced for sure to have something that specifies that we want to exit as soon as a failure happens. Although if even one of the healthcheck fails, we're going to rollback anyway and surely at the next upgrade something else may fail but that's the flow anyway right? |
say-paul
force-pushed
the
required_failed_early
branch
from
1e6f7c2 to
283683e
Compare
|
I agree with @runcom that even if one or multiple scripts fail, greenboot will follow the same path of running red scripts.. then restart/rollback. |
When greenboot encounters an error in required scripts,it braks out of the healthcheck loop and moves to redscript excution skipping the other required and wanted scripts.Logs are added to notify users of the skipped required scripts due to critical failure.Test modified to validate only one error is reported.Also cleasning code base of redundant codes. Signed-off-by: saypaul <[email protected]>
say-paul
force-pushed
the
required_failed_early
branch
from
4e9e152 to
b943b39
Compare
When greenboot encounters an error in required scripts, it breaks out the healtcheck loop and moves to red script execution. Logs are added to notify users of the skipped scripts due to critical failure.
fixes: #143