Skip to content
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

chore: add commit hooks #632

Merged
merged 2 commits into from Jul 31, 2023
Merged

chore: add commit hooks #632

merged 2 commits into from Jul 31, 2023

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented Jul 25, 2023

This change:

  • adds a pre-commit hook that runs eslint --fix on any JS file on the stage.
  • adds a commit message check that verifies the commit message is in conventional commits format.
  • adds a new CI workflow to check commit messages in pull requests which target main

We will definitely want this if we want better automation of our releases. Please see these docs for more info about commitlint and how we can further configure it. I have created a configuration which adopts the conventional commits format but also relaxes some of the more pedantic default rules.

Note: I'm unsure if using a prepare script is OK here. This command tells husky to install the hooks into the .git/ directory, and is only run in a development environment upon npm install

package.json Outdated
"preversion": "npm run rebuild",
"prepare": "husky install",
Copy link
Member

@naugtur naugtur Jul 26, 2023

Choose a reason for hiding this comment

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

prepare runs on publishing too. Why not postinstall?

sidenote: husky is fairly complex. Do we need more than https://www.npmjs.com/package/pre-commit ?
(edit)
Oh, wait - pre-commit won't handle checking the message. nevermind

Copy link
Contributor

@legobeat legobeat Jul 26, 2023

Choose a reason for hiding this comment

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

I went for https://github.com/toplenboren/simple-git-hooks last time I had to pick one FWIW. pre-commit drags along a bit too much deps too be worth it imo.

Copy link
Member

@naugtur naugtur Jul 28, 2023

Choose a reason for hiding this comment

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

sounds good. husky is also fine.

Let's address the lifecycle script choice though.

Copy link
Contributor Author

@boneskull boneskull Jul 28, 2023

Choose a reason for hiding this comment

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

I think you may be right about postinstall if we want to avoid husky install on publish--but only because we're in a workspace root. prepare should probably be calling npm run rebuild instead of preversion, then.

wait, because we have allow-scripts configured, it doesn't run automatically anyway.

anyhow, I've changed it to this:

  • postinstall runs husky install
  • prepare runs rebuild, which calls the rebuild script in workspaces where it is extant
  • preversion removed
  • add $root = true to allowScripts config

@@ -0,0 +1,17 @@
name: Validate Commit Messages
on:
[pull_request]
Copy link
Member

Choose a reason for hiding this comment

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

+1 to running it as early as possible, not only for things targeting master.
This may occasionally be annoying when doing messy experiments, but it's totally worth it.

Copy link
Contributor Author

@boneskull boneskull Jul 28, 2023

Choose a reason for hiding this comment

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

lol, I just reverted this. I'm not sure it is worth it, honestly. but I'll put it back.

note that it does not run on push because, well, if you've pushed to main it's too late anyway. maybe we need a branch protection rule (if we don't have one already) that disallows direct pushes to main.

Copy link
Member

@naugtur naugtur left a comment

Choose a reason for hiding this comment

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

Please add a section to the README documenting the commit convention matching commitlint.config.js

@boneskull
Copy link
Contributor Author

Here's an example of the commitlint job running: https://github.com/LavaMoat/LavaMoat/actions/runs/5685783177/job/15411381654?pr=632

@boneskull boneskull force-pushed the boneskull/husky branch 2 times, most recently from 175a5ba to 94e21e9 Compare July 28, 2023 17:35
@boneskull boneskull force-pushed the boneskull/husky branch 2 times, most recently from 20bc7e1 to 429b311 Compare July 31, 2023 18:23
- adds a pre-commit hook that runs `eslint --fix` on any JS file on the stage
- adds a commit message check that verifies the commit message is in conventional commits format
- adds `husky install` to workspace root `postinstall` script, to be run via `allow-scripts`
- adds a `prepare` lifecycle script which runs the `rebuild` script
- remove `preversion` script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore overhead, tests, dev env, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants