Conversation
Create a shared workflow to `npm audit fix`: * Creates branch, commits the dependency changes, and opens a PR * Parameterizes branch name, commit title, and cleanup behavior for different use cases * By default, cleans up branches with the same name and associated PRs Currently, workflow fails if `npm audit fix` returns non-zero exit code - meaning PRs will not be created for breaking changes or conflicts in dependency resolution.
Reference new workflow file in CHANGELOG.md
Adds `content: write` to the job permissions, allowing the workflow to push branch deletion to the main repo.
| cleanup_previous: | ||
| description: Whether to cleanup unresolved PR/branch and start over | ||
| type: boolean | ||
| required: false | ||
| default: true |
There was a problem hiding this comment.
This controls the execution of step that looks for a branch name conflict, locates and deletes the associated PR (if present) then deletes the branch. This seemed like desirable behavior if this is going to run on some kind of schedule - but that it might also be nice to be able to switch that behavior off in certain use cases.
Including it as a parameter has the added benefit of increasing the visibility of a pretty impactful side-effect of the workflow.
There was a problem hiding this comment.
I can't think of a reason to want the workflow to leave obsolete PRs and stale branches in the repository. What did you have in mind?
There was a problem hiding this comment.
The discussion of failure modes in README.md indicates that the workflow will just fail if cleanup_previous is false and the branch already exists. If that's true, this option is a bit of a footgun, and I'm inclined to eliminate it.
There was a problem hiding this comment.
As I was writing the workflow, I had some half-formed idea that this could be useful - but I'm having a hard time coming up with a concrete use case. If we want to trigger this from another workflow, we should just be able to specify a non-conflicting branch name and avoid issues - so I'm happy to remove this until a use case becomes clear.
| echo "AUDIT_REPORT<<EOF" >> "$GITHUB_OUTPUT" | ||
| npm audit fix &>> "$GITHUB_OUTPUT" | ||
| echo "EOF" >> "$GITHUB_OUTPUT" | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| echo "CHANGED=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "CHANGED=false" >> "$GITHUB_OUTPUT" | ||
| fi |
There was a problem hiding this comment.
I picked up a couple tricks from AI suggestions:
- Capturing the npm output in
AUDIT_REPORTallows me to use that in the PR body, and - Establising a
CHANGEDoutput flag lets me skip most of the rest of the job the package files were not changed
| git config --global user.name "github-actions[bot]" | ||
|
|
||
| - name: Clean up previous branch/PR | ||
| if: ${{ inputs.cleanup_previous }} |
There was a problem hiding this comment.
This is the only step after audit-step that isn't gated by the CHANGED flag. Since we may occasionally update these dependencies manually, it seems useful for the workflow to clean up old PRs - even if we aren't creating a new one.
| description: Text for the title line of the git commit | ||
| type: string | ||
| required: false | ||
| default: 'chore: Node security update' |
There was a problem hiding this comment.
Except for Erik (because of habit from AI-READI), we don't typically use the conventional commits labels. Any particular reason you picked this default?
There was a problem hiding this comment.
Not a particularly strong reason. I believe I'd seen it used in a few places and examples on the web, and my brain found it natural to try and substitute something in the place where we typically place the ticket number. I'm happy to shorten this to "Node security update", or whatever would most fit with the team conventions.
There was a problem hiding this comment.
I think "Node security update" is fine. I just don't want to adopt conventional commits without agreement from the whole team.
heathharrelson
left a comment
heathharrelson
left a comment
There was a problem hiding this comment.
Nice! I'm curious to see how this works in practice.
Incorporate feedback from the PR: * Simplify commit message default * Remove `cleanup_previous` flag, given absense of compelling use case
Ensure that git user/email are setup, regardless of whether files have changed, because the workflow will always attempt to cleanup previous PRs.
2 participants
Overview
Create a shared workflow to
npm audit fix:Currently, workflow fails if
npm audit fixreturns non-zero exit codeIssues
CIS-3816
[x] Added to CHANGELOG.md
Discussion
I'll leave some more specific points as comments in the code, but the main idea here is that the primary use case is a cron-based dispatch in the project workflow files (with a few things parameterized to support other use cases).