Split debug artifacts PR check into two jobs - #1160
angelapwen merged 10 commits into
Conversation
Sorry, something went wrong.
|
Hmmm...or you can move debug-artifacts.yml out of the sync directory and avoid generating it altogether. |
Sorry, something went wrong.
Yeah, I think that will be easier. Will do that now. |
Sorry, something went wrong.
angelapwen
left a comment
There was a problem hiding this comment.
I changed the workflow to a non-generated workflow so that it can include 2 jobs, but kept all of the fields from the generated workflow.
I'm not entirely sure how to test this to make sure I haven't messed up any of the syntax, though.
Sorry, something went wrong.
|
Re testing: because this workflow has a pull_request trigger that is satisfied by this PR, it should run as a PR check on this same PR. |
Sorry, something went wrong.
Thanks! I didn't know that there was a view that showed syntax errors. |
Sorry, something went wrong.
|
Hm... it seems that the Download and check debug artifacts job didn't run this time around. I found the runs but no syntax error annotations on the workflow file used for the run. I think I misread. It looks like the "Debug artifact upload" check is the only one pending, and I think it refers to the PR Check that I deleted, so it should be removed for future PRs? The new upload and download jobs seem to be succeeding 🥳 It seems like the download job is still running using the matrix even though I hard-coded it to run only on ubuntu though 🤔 |
Sorry, something went wrong.
|
I think this PR is ready for final review! The download/check part of the job only runs on one machine and successfully checks all uploaded artifacts. See https://github.com/github/codeql-action/runs/7649251536?check_suite_focus=true |
Sorry, something went wrong.
adityasharad
left a comment
There was a problem hiding this comment.
Looks good! Minor suggestions only.
Sorry, something went wrong.
| LANGUAGES="cpp csharp go java javascript python" | ||
| for os in $OPERATING_SYSTEMS; do | ||
| for version in $VERSIONS; do | ||
| cd ./my-debug-artifacts-$os-$version |
There was a problem hiding this comment.
Quotes not essential but a good habit.
I observe this cd will fail if the artifact dir doesn't exist, but I think that's ok and will give us a reasonable error message. We can improve it in future if necessary.
| cd ./my-debug-artifacts-$os-$version | |
| cd "./my-debug-artifacts-$os-$version" |
Sorry, something went wrong.
There was a problem hiding this comment.
Ah interesting.. I wonder if it fails if it will continue onto the next directory? Probably not..
Sorry, something went wrong.
There was a problem hiding this comment.
It will not. This is because by default Actions runs bash steps with set -e (set -eo pipefail specifically; see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell), so any shell command exiting non-zero will fail the entire step.
You can turn this off in a given step by adding set +e at the beginning, but we don't need that here.
Sorry, something went wrong.
There was a problem hiding this comment.
Interesting! Thanks!
Sorry, something went wrong.
|
Hm.. it looks like this is blocking on the 12 expected checks, which are the checks that I removed in this PR 🤔 is there any way to force green? |
Sorry, something went wrong.
The current debug artifacts PR check executes just after the analyze step, which works at the moment but fails in #1159 which moves the artifact uploading to post: hooks. This PR attempts to split the PR check into 2 jobs, so that the post: hooks from the first job complete before the job that attempts to download and check the artifacts.
As the sync.py script doesn't yet work with a file that has multiple jobs, I stopped using the generator for this workflow.
Merge / deployment checklist