Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .eslintignore

This file was deleted.

3 changes: 0 additions & 3 deletions .eslintrc

This file was deleted.

21 changes: 21 additions & 0 deletions .github/dependabot.yml

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.

Let's remove this from this PR specifically. I am not a fan of the dependabot workflow and think Renovate Dependency Dashboard is a better pattern but that requires a lot more effort and a longer discussion.

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# To get started with Dependabot version updates, you'll need to specify which
# package ecosystems to update and where the package manifests are located.
# Please see the documentation for all configuration options:
# https://docs.github.com/github/administering-a-repository/configuration-options-for-dependency-updates

version: 2
updates:
- package-ecosystem: "npm"
directory: "/" # Location of package manifests
Comment thread
nmccready marked this conversation as resolved.
schedule:
interval: "daily"
cooldown:
default-days: 7
groups:
all-dependencies:
patterns:
- "*"
commit-message:
prefix: fix
prefix-development: chore
include: scope
28 changes: 28 additions & 0 deletions .github/workflows/_publish.yml_disable

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.

Let's drop this completely. Publishing from CI is an attack vector for the spreading of worms in package registries.

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# disabling until we get 2FA working in github
name: publish

on:
push:
tags:
- "v*"

jobs:
tests:
uses: ./.github/workflows/tests.yml
publish-npm:
needs: [tests]
runs-on: ubuntu-latest
permissions:
id-token: write # Required for OIDC
steps:
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6
with:
node-version: '20.x'
registry-url: 'https://registry.npmjs.org'

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.

Suggested change
registry-url: 'https://registry.npmjs.org'
registry-url: 'https://registry.npmjs.org

typo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Were you meaning to get rid of both quotes?

- name: Publish to npm
run: | # npm 11.15.1 for OIDC support
npm install -g npm@11

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.

perhaps use a newer node to publish to avoid needing to install an unpinned npm version

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, currently still running with mainly 20 for most things and older npm, only recently moved to 11 NPM for OIDC support.

npm ci
npm publish
40 changes: 40 additions & 0 deletions .github/workflows/commitlint.yml

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.

You can set up github branch protection rules that do this instead of adding workflows and dependencies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Example? I know this works, been using this for a while.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I understand branch protection rules can validate commit messages, but the workflow approach has a few advantages: it's visible in the repo, works consistently across forks, and provides clear CI feedback. That said, if you'd prefer branch protection, I can remove the commitlint workflow and dependencies — just let me know your preference as the maintainer.

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.

Remove this workflow

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
name: commitlint

on:
workflow_call:
push:
pull_request:
branches: ["master"]

jobs:
commitlint:
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
with:
fetch-depth: 0
- name: Install required dependencies
run: |
sudo apt update
sudo apt install -y sudo
sudo apt install -y git curl

- uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3
with:
node-version: 20
- name: node setup
run: npm i
- name: Print versions
run: |
git --version
node --version
npm --version
npx commitlint --version

- name: Validate current commit (last commit) with commitlint
if: github.event_name == 'push'
run: npx commitlint --last --verbose

- name: Validate PR commits with commitlint
if: github.event_name == 'pull_request'
run: npx commitlint --from ${{ github.event.pull_request.head.sha }}~${{ github.event.pull_request.commits }} --to ${{ github.event.pull_request.head.sha }} --verbose
37 changes: 37 additions & 0 deletions .github/workflows/release.yml

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.

Remove this. We'll add release-please if it becomes necessary.

Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
name: release

on:
push:
branches: ["master"]
tags-ignore: ['**']

jobs:
tests:
uses: ./.github/workflows/tests.yml
tag-release:
runs-on: ubuntu-latest
needs: [tests]
steps:
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
with: # important, must be defined on checkout to kick publish (defining in setup/node doesn't work)
token: ${{ secrets.GH_TOKEN }}
Comment on lines +16 to +17

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.

All my projects use release-please to avoid this. It opens a PR so the merge commit triggers CI correctly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

URL to copy?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I looked into release-please — it's a solid option! The current approach with commit-and-tag-version is simpler (no extra GH App/action needed) and fits the existing workflow, but I'm open to switching if you prefer release-please. For reference: https://github.com/googleapis/release-please-action

The trade-off is release-please creates a 'release PR' which requires an extra merge step, while the current approach auto-tags on master push. Both are valid — your call as maintainer.

- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3
with:
node-version: '20.x'
cache: "npm"

- name: tag release
run: |
# ignore if commit message is chore(release): ...
if [[ $(git log -1 --pretty=%B) =~ ^chore\(release\):.* ]]; then
echo "Commit message starts with 'chore(release):', skipping release"
exit 0
fi
git config --local user.email "creadbot@github.com"
git config --local user.name "creadbot_github"
set -v
npm ci
npx commit-and-tag-version
git push
git push --tags
24 changes: 24 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
name: tests

on:
workflow_call:

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.

Suggested change
workflow_call:

This can be useful for debugging but let us keep this super focused.

pull_request:
branches: ["master"]

jobs:
test:
strategy:
matrix:
node-version: ['20.x', '22.x', '24.x']
runs-on: ubuntu-latest

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.

We want Windows and Mac in the matrix since we are losing appveyor

steps:
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3
with:
node-version: ${{ matrix.node-version }}
cache: "npm"

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.

No cache please. While uncommon, cache poisoning has been used in supply chain attacks

- run: npm ci
- run: npm run lint
- run: npm audit --omit dev

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.

I could take or leave the audit. npm audit is really shitty and flags things that aren't actually an issue. Socket does it better but that is out of scope for this PR.

- run: npm test
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,5 @@ node_modules
test/fixtures/tmp
test/fixtures/out
.nyc_output

# package-lock.json # OIDC
Comment on lines +37 to +38

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.

Suggested change
# package-lock.json # OIDC

I think it is common practice to check in the package-lock these days so there is no need to add it commented out with a comment to why.

9 changes: 0 additions & 9 deletions .travis.yml

This file was deleted.

25 changes: 0 additions & 25 deletions appveyor.yml

This file was deleted.

6 changes: 6 additions & 0 deletions commitlint.config.js

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.

Delete this

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
extends: ['@commitlint/config-conventional'],
rules: {
'body-max-line-length': [2, 'always', 200],
},
};
10 changes: 10 additions & 0 deletions eslint.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const gulpConfig = require('eslint-config-gulp');

module.exports = [
{
files: ["test/fixtures/*.js"],
rules: {
"no-unused-vars": "off",
},
},
];
Loading