-
Notifications
You must be signed in to change notification settings - Fork 402
Improve pr-checks
workflow
#3153
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
Conversation
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.
Pull Request Overview
This PR improves the pr-checks
workflow by ensuring all checks run even if one fails and adds helpful diff summaries to job outputs when there are changes to JS bundles or generated workflows.
- Adds
if: always()
conditions to allow all checks to run regardless of previous failures - Implements diff output to GitHub step summaries for both JS bundle changes and generated workflow changes
- Includes a git checkout command to reset bundled files after displaying the diff
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
.github/workflows/pr-checks.yml |
Adds if: always() conditions to ensure all workflow steps run |
.github/workflows/script/check-js.sh |
Adds JS diff output to step summary and resets bundled files |
.github/workflows/script/verify-pr-checks.sh |
Adds generated workflows diff output to step summary |
git diff --output="$RUNNER_TEMP/js.diff" | ||
cat "$RUNNER_TEMP/js.diff" >> $GITHUB_STEP_SUMMARY |
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.
Consider uploading the diff as an artifact
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.
I initially had that, but thought this was nicer because you can see the syntax-highlighted diff without having to download a file, unpack it, and open it.
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.
I think both are nice in case there's a big diff. But this is a very minor comment, good to go as is.
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.
Let's leave it for now then, but we can add it later if we find it's needed?
git diff --output="$RUNNER_TEMP/workflows.diff" | ||
cat "$RUNNER_TEMP/workflows.diff" >> $GITHUB_STEP_SUMMARY |
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.
Ditto here
A common gotcha is having out of date dependencies locally. We could consider posting a comment on the PR with the command to run (e.g. |
I'm not super keen on running |
Agreed on both points. I don't think we should run I'll have a look at what we can do to detect whether |
Improves the
pr-checks
workflow so that:Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist