Skip to content
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

Split debug artifacts PR check into two jobs #1160

Merged
merged 10 commits into from
Aug 4, 2022

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Jul 29, 2022

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

  • Confirm this change is backwards compatible with existing workflows.
  • [N/A] Confirm the readme has been updated if necessary.
  • [N/A] Confirm the changelog has been updated if necessary.

@aeisenberg
Copy link
Contributor

Hmmm...or you can move debug-artifacts.yml out of the sync directory and avoid generating it altogether.

@angelapwen
Copy link
Contributor Author

Hmmm...or you can move debug-artifacts.yml out of the sync directory and avoid generating it altogether.

Yeah, I think that will be easier. Will do that now.

@angelapwen angelapwen force-pushed the angelapwen/refactor-debug-artifacts-pr-check branch from fa8d4bd to 81c5b2d Compare August 1, 2022 09:09
Copy link
Contributor Author

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.github/workflows/debug-artifacts.yml Outdated Show resolved Hide resolved
.github/workflows/debug-artifacts.yml Outdated Show resolved Hide resolved
.github/workflows/debug-artifacts.yml Outdated Show resolved Hide resolved
@adityasharad
Copy link
Contributor

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.
However, looking at the Checks panel of this PR, we see that the Debug artifact upload checks are still pending, which means the modified workflow didn't run, suggesting a workflow syntax error. Debug by going to the checks tab, then the run for this workflow, then the workflow file used for the run, which shows syntax errors as annotations.

@angelapwen
Copy link
Contributor Author

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. However, looking at the Checks panel of this PR, we see that the Debug artifact upload checks are still pending, which means the modified workflow didn't run, suggesting a workflow syntax error. Debug by going to the checks tab, then the run for this workflow, then the workflow file used for the run, which shows syntax errors as annotations.

Thanks! I didn't know that there was a view that showed syntax errors.

@angelapwen
Copy link
Contributor Author

angelapwen commented Aug 2, 2022

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 🤔

@angelapwen
Copy link
Contributor Author

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

@angelapwen angelapwen requested a review from adityasharad August 3, 2022 11:53
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Minor suggestions only.

.github/workflows/debug-artifacts.yml Outdated Show resolved Hide resolved
LANGUAGES="cpp csharp go java javascript python"
for os in $OPERATING_SYSTEMS; do
for version in $VERSIONS; do
cd ./my-debug-artifacts-$os-$version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
cd ./my-debug-artifacts-$os-$version
cd "./my-debug-artifacts-$os-$version"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah interesting.. I wonder if it fails if it will continue onto the next directory? Probably not..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! Thanks!

@angelapwen angelapwen enabled auto-merge August 3, 2022 15:57
@angelapwen
Copy link
Contributor Author

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?

@angelapwen angelapwen merged commit 9990b40 into main Aug 4, 2022
@angelapwen angelapwen deleted the angelapwen/refactor-debug-artifacts-pr-check branch August 4, 2022 08:35
@github-actions github-actions bot mentioned this pull request Aug 19, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants