-
-
Notifications
You must be signed in to change notification settings - Fork 739
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
Fix st2-self-check script reporting success on failed runs #5487
Conversation
When the test run contains several nested workflows the script assumes it's succeeded if at least single sub-workflow was successful. Instead of that, the check should refer to the parent workflow "status: succeeded".
Other than a small restriction on the regex pattern, this looks good to me. |
…ubstitution ``` echo ${OUTPUT} ``` results in entire output being on a single line, which is why it breaks the grep rule at the first place. Fixes #5487 when st2-self-check script reported success on failed runs.
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
Now that you've already figured it out, I'm somewhere I can look up how I've dealt with something similar. First I get an execution id (I only did match_and_execute() {
# usage: match_and_execute "<alias>" <timeout>
# returns the execution id
echoerr "chatops> ${1}"
st2 --cacert=true run chatops.match_and_execute text="${1}" source_channel=vela user=${VELA_BUILD_AUTHOR} timeout=${2:-600} --attr result.result --json | jq -r .result.result
} Then I query an execution status from bash: assert_status() {
# usage: assert_status <exec_id> <expected_status>
local exec_status
exec_status=$(st2 --cacert=true execution get --detail --json ${1} | jq -r .status)
if [ "${2}" != "${exec_status}" ]; then
echo failed
echoerr "The alias execution should be ${2} but it is ${exec_status}!"
return 4
fi
echo passed
echo
} Note how I used This is what one of my tests using those functions looks like: ALIAS_EXEC_ID=$(match_and_execute "tc list" 120)
echo ALIAS_EXEC_ID=${ALIAS_EXEC_ID}
assert_status "${ALIAS_EXEC_ID}" "succeeded" |
To be clear, I'm not recommending you change the approach for this PR. I just wanted to share it while I was thinking about it as a possible future enhancement. |
@cognifloyd That does multiple requests, adding smart code, complexity, and jq as a dependency. Form the dull code we have: OUTPUT=$(st2 run ${TEST} protocol=${PROTOCOL} token=${ST2_AUTH_TOKEN})
echo "${OUTPUT}" | grep "status" | grep -q "succeeded"
EXIT_CODE=$?
if [ ${EXIT_CODE} -ne 0 ]; then
echo "Test output: ${OUTPUT}"
((ERRORS++))
fi In this case, the workflow would show the original human-readable execution output as-is, as someone would run it. Just a different approach depending on what matters more in a particular situation. |
@amanda11 @nzlosh @cognifloyd Thanks everyone for the reviews 😉 We found a better way! |
When the test run contains several nested workflows the script assumes it's succeeded if at least a single sub-workflow was successful. Instead of that, the check should refer to the parent workflow with "status: succeeded".
Example