[ci] check all-jobs-succeed depends on all jobs #720
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
.github/workflows/ci.yml
Outdated
| # Effectively, this checks that all-jobs-succeed does not depend on | ||
| # itself. Checking for the existence of a job that we don't want |
There was a problem hiding this comment.
IIUC, it's slightly different than this - it checks that the set of jobs that all-jobs-succeeded does not depend on is exactly itself. That distinction is important because it also ensures that there are no jobs besides itself that it doesn't depend on. Do I understand that correctly? If so, could you update the comment?
There was a problem hiding this comment.
You make perfect sense and this comment definitely should be updated. In updating the comment though, I found the language hard to get right. So I've eliminated this check. I think it makes it easier to understand with or without comments.
.github/workflows/ci.yml
Outdated
| if [ "$jobs" != "all-jobs-succeed" ] | ||
| then | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Oh also: On failure, could you echo $jobs so that a developer can see from the Actions output what the offending jobs are?
| if [ "$jobs" != "all-jobs-succeed" ] | |
| then | |
| exit 1 | |
| fi | |
| if [ "$jobs" != "all-jobs-succeed" ] | |
| then | |
| missing_jobs="$(echo "$jobs" | tr ' ' '\n' | grep -v '^all-jobs-succeed$')" | |
| echo "all-jobs-succeed missing dependencies on some jobs: $missing_jobs" | tee -a $GITHUB_STEP_SUMMARY | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Ahh! Sorry I did not see the code suggestion. I added my own message here.
This also changes the logic somewhat (hopefully in a way that improves readability). There was a bit of a self-check in place before that has been removed because it made the code hard to explain (too many double negatives).
.github/workflows/ci.yml
Outdated
| # The sed call here excludes all-jobs-succeed from the list of jobs that | ||
| # all-jobs-succeed does not depend on. If all-jobs-succeed does | ||
| # not depend on itself, we do not care about it. | ||
| done | sort | uniq | sed '/^all-jobs-succeed$/d' |
There was a problem hiding this comment.
IMO more developers are familiar with grep -v than with sed with the /d suffix. It's also easier to look up since you can just look up the -v flag in the grep manpage (I tried looking up the /d suffix in the sed manpage and wasn't sure how to search for it).
| done | sort | uniq | sed '/^all-jobs-succeed$/d' | |
| done | sort | uniq | grep -v '^all-jobs-succeed$' |
There was a problem hiding this comment.
The only trouble here is grep exits with 1 in this case. This is easy enough to ignore though.
|
Thanks again for your help with this, @tommy-gilligan! |
|
No worries! Happy to help! Let me know if there's anything else you'd like me to look at. |
Potentially a new per-PR job could be added to CI without it being included in
all-jobs-succeed's dependencies. This prevents that.I have successfully constructed a failure (by deleting a job from
all-jobs-succeed's dependencies)Closes #444