Fix broken test and report CI failing correctly #207
Conversation
Signed-off-by: Toshikuni Fukaya <[email protected]>
|
LGTM |
|
Could someone review this? |
|
Could some one take a look at this PR? It's a small PR and we can't detect some CI failures without applying it. |
| @@ -86,8 +90,9 @@ linters(){ | |||
| ci-checks(){ | |||
| echo "-------- beginning preflight checks" | |||
| lint | |||
| test | |||
| build | |||
| test || RET=$? | |||
There was a problem hiding this comment.
perhaps safer to define local RET=0 in all those functions instead of setting a global var?
There was a problem hiding this comment.
Yeah, It is preferable. I'll fix them
| ) | ||
| return $? |
There was a problem hiding this comment.
It looks like the goal here is to try to build every pkg, even when 1 or more fails. But the err returned is for the last failure only. Perhaps we should fail on the 1st build error, or return a list of error strings of a format like: "$p: $?". What do you think?
Same comment for test() below.
There was a problem hiding this comment.
My intention is to report the build status to CI.
If use fail-fast manner, it is sufficient for the purpose, however, it breaks current behavior which tries to build all packages.
I don't want to change the behavior because some tools may depend on such behavior.
And I personally prefer that all build error is reported.
For 2nd suggestion, it is not effective, IMO.
The script already does echo for package names, and Go also reports which package fails.
Because the script
hack/go.sh ci-checksdoes not report its failure by the exit code, we can not notice a CI failure.This fixes the issue and also fixes found broken tests.
Signed-off-by: Toshikuni Fukaya [email protected]