FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Improve `pr-checks` workflow by mbg · Pull Request #3153 · github/codeql-action · GitHub

/ codeql-action Public

Improve pr-checks workflow - #3153

Merged
mbg merged 4 commits into
mainfrom
mbg/ci/improve-unit-tests
Sep 25, 2025
ConversationCommits (4)ChecksFiles changed
Merged

Improve pr-checks workflow#3153
mbg merged 4 commits into
mainfrom
mbg/ci/improve-unit-tests

Conversation

mbg commented Sep 25, 2025

Copy link
Copy Markdown
Member

Improves the pr-checks workflow so that:

  • All checks run, even if one fails.
  • A diff of the changes to the JS bundles is added as job summary if there are changes
  • A diff of the changes to the generated workflows is added as job summary if there are changes

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

mbg requested a review from henrymercer September 25, 2025 12:30
mbg requested a review from a team as a code owner September 25, 2025 12:30
mbg requested review from Copilot and removed request for a team September 25, 2025 12:30
Copilot AI reviewed Sep 25, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread .github/workflows/script/check-js.sh
henrymercer approved these changes Sep 25, 2025
View reviewed changes
Comment thread .github/workflows/script/check-js.sh
Comment on lines +23 to +24
git diff --output="$RUNNER_TEMP/js.diff"
cat "$RUNNER_TEMP/js.diff" >> $GITHUB_STEP_SUMMARY

henrymercer Sep 25, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Consider uploading the diff as an artifact

mbg Sep 25, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

henrymercer Sep 25, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think both are nice in case there's a big diff. But this is a very minor comment, good to go as is.

mbg Sep 25, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Let's leave it for now then, but we can add it later if we find it's needed?

Comment thread .github/workflows/script/verify-pr-checks.sh
Comment on lines +27 to +28
git diff --output="$RUNNER_TEMP/workflows.diff"
cat "$RUNNER_TEMP/workflows.diff" >> $GITHUB_STEP_SUMMARY

henrymercer Sep 25, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ditto here

henrymercer commented Sep 25, 2025

Copy link
Copy Markdown
Contributor

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. npm i && npm run build && npm test && npm run lint). We could also consider integrating a check that dependenices are up to date as part of the build process, if this is possible to do quickly.

henrymercer commented Sep 25, 2025

Copy link
Copy Markdown
Contributor

I'm not super keen on running npm i as part of npm run build just because of how slow it is, but this is also something we could consider.

mbg commented Sep 25, 2025

Copy link
Copy Markdown
Member Author

Agreed on both points. I don't think we should run npm install as part of anything we do frequently during normal development.

I'll have a look at what we can do to detect whether npm install has to be run, but separately from this PR.

mbg merged commit 8fca381 into main Sep 25, 2025
279 of 322 checks passed
mbg deleted the mbg/ci/improve-unit-tests branch September 25, 2025 13:21
github-actions Bot mentioned this pull request Sep 26, 2025
Merged
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

henrymercer henrymercer approved these changes
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL