GitHub Actions Workflow for Pull Request Checks

[danswann]

Summary

This is an action that will run every time a PR is opened targeting the main branch.
It is set up to run the following:

• linting with prettier
• a test Docusaurus build, which will be discarded when the workflow finishes running

If one or more of these jobs fail, they will result in a little red X in the checks section of the PR merge area.

I've only ever been responsible for CI for personal projects, so I welcome feedback. I also find it terribly difficult to test GitHub Actions workflows, so here's hoping it works on the first try.

Design Decisions

Action Dependencies

In addition to standard GitHub-created actions, this uses the @pnpm/action-setup action provided by the maintainers of the pnpm package manager itself. While they are not technically a "verified Marketplace publisher," this action is nevertheless recommended by GitHub in their actions/setup-node documentation and it seems unlikely that the pnpm maintainers would poison their own GitHub action.

Workflow Design

I looked at a lot of prominent Node-based packages on GitHub to see how they designed their PR-related workflows, and have tried to incorporate what I learned from them.

Job Definitions

There is a lot of redundancy in the job definitions (checkout, installing pnpm, installing Node, installing dependencies). This could be addressed by factoring it out into a separate composite action, or running the linter and build as consecutive steps within a single job, but I found no other prominent repository that did it like this.

Keeping them distinct adds a small maintenance overhead whenever we want to change things common to all jobs, but also means that we can tailor them individually when appropriate. For example it may be desirable to test the build on multiple versions of Node, but we are probably fine running the linter on only one version.

I've also specified latest as the version of pnpm to install to reduce maintenance, but I'm not sure if that's appropriate or if it's common for package managers to have meaningful breaking changes on major version bumps. We might watch to change it to 8 explicitly.

Concurrency

I made the decision to assign this workflow to a workflow-specific concurrency group. This is basically to stop rapid-fire commits on a single PR from launching multiple linting and building jobs when we really only care about the latest. This will not block workflow runs on other open PRs and will not interfere with the eventual deploy workflow.

I've also enabled the cancel-in-progress flag because there are no side-effects to canceling a linting run or a test build, and it just prevents running jobs that are no longer relevant when a newer commit has been added.

Additional Checks to Consider

We've also not talked about how to handle package bumps required to address security vulnerabilities, and I see dependabot already has 2 alerts for us.

I had considered adding a third check using the official GitHub dependency-review-action action, which can be configured to block a PR from being merged if one or both of the following are true:

• The package lockfile being committed has dependencies that are associated with security alerts
• The package lockfile being committed has packages with incompatible licenses

One the one hand, I like the idea of blocking PRs until vulnerable packages are addressed, but on the other hand this could lead to the addition of security-related commits to PRs that have nothing to do with security. Maybe they would be better in separate dependabot-initiated PRs created at whatever interval the bot is configured to check. I would like to hear other opinions on this.

[ghambhackmud]

Like it. Especially agree with concurrency as stated. Curious if anyone has any input on pnpm.

[seanmakesgames]

I've also specified latest as the version of pnpm to install to reduce maintenance, but I'm not sure if that's appropriate or if it's common for package managers to have meaningful breaking changes on major version bumps. We might watch to change it to 8 explicitly.

We should be able to pre-bake our runners and reduce duration. pnpm version should match our running version of node though. npm and node major versions are paired together. So, yes, we should probably change to an explicit version

I had considered adding a third check using the official GitHub dependency-review-action action, which can be configured to block a PR from being merged if one or both of the following are true:

Let's do it

One the one hand, I like the idea of blocking PRs until vulnerable packages are addressed, but on the other hand this could lead to the addition of security-related commits to PRs that have nothing to do with security.

Nope -- we'd kick off a separate PR to fix the security issues- get that merged first, then update the PR that was blocked to include them-- probably with a rebase.

[seanmakesgames]

Like it. Especially agree with concurrency as stated. Curious if anyone has any input on pnpm.

I'm definitely up to give pnpm a try. :)

[seanmakesgames]

We have two open dependabot items-- shouldn't we be failing on the dependency-review-action step?

[danswann]

We have two open dependabot items-- shouldn't we be failing on the dependency-review-action step?

Turns out I misunderstood what the security half of this does. It only looks for security issues in new or updated dependencies introduced by a PR commit.

Still very useful, but for vulnerabilities found before introducing it (such as the two pending dependabot alerts) and for new vulnerabilities disclosed that are unrelated to a PR (like a package previously thought safe is found not to be) it will not block the PR.

I don't find anything at the action level (other than explicitly running a code scanning tool) that behaves like how I thought this was going to. A quick web search says there might be a way to do it with GitHub's CodeQL and branch protection rules, but I've never messed around with the former.

Also for the licensing half of it, it operates in either whitelist or blacklist mode, so if we want to gain functionality from that we'd have to specify a list of licenses we are either explicitly okay with or explicitly not okay with. I don't think it attempts to automagically guess license compatibility.

[danswann]

Turns out I misunderstood what the security half of this does. It only looks for security issues in new or updated dependencies introduced by a PR commit.

At least, that is how I understand this quote from the action page:
"The action is supported by an API endpoint that diffs the dependencies between any two revisions on your default branch."

The API endpoint referenced says it "get[s] a diff of the dependencies between commits."

[danswann]

I think the quickest short-term solution is just to configure dependabot to open his own PRs, and either prioritize those or configure him to auto-merge them if all checks are passed.

[seanmakesgames]

Turns out I misunderstood what the security half of this does. It only looks for security issues in new or updated dependencies introduced by a PR commit.

Nope. Since this is working as intended, that's great! (this is a whole lot more complicated for them to have implemented this way and is awesome) This means that the check will only fail if the PR introduces the security issue. Which is a whole lot less disruptive. PRs are very likely to not have this churn and in fact, fixes to the security issues would make a lot of sense to be included in the PR where the checks fail.

I think the quickest short-term solution is just to configure dependabot to open his own PRs, and either prioritize those or configure him to auto-merge them if all checks are passed.

I'll get this set up.

Also for the licensing half of it, it operates in either whitelist or blacklist mode, so if we want to gain functionality from that we'd have to specify a list of licenses we are either explicitly okay with or explicitly not okay with. I don't think it attempts to automagically guess license compatibility.

Opened #5