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: introduce husky commit-msg hook and pr title linter #1385

Merged
merged 4 commits into from
Feb 12, 2024
Merged

chore: introduce husky commit-msg hook and pr title linter #1385

merged 4 commits into from
Feb 12, 2024

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Feb 12, 2024

No description provided.

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (17549b8) 99.8% compared to head (2029c02) 99.8%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #1385    +/-   ##
=======================================
  Coverage    99.8%   99.8%            
=======================================
  Files         199     199            
  Lines       21557   21557            
  Branches     6937    7203   +266     
=======================================
  Hits        21493   21493            
+ Misses         64      58     -6     
- Partials        0       6     +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding
Copy link
Owner

will it check for current PR? I mean even your current PR doesn't have the scope so it should fail in the future, is it? 😝

@zewa666 zewa666 changed the title Husky commit msg feat: introduce husky commit-msg hook and pr title linter Feb 12, 2024
@zewa666
Copy link
Contributor Author

zewa666 commented Feb 12, 2024

@ghiscoding hehe I'm just playing with it right now to get everything setup properly

@ghiscoding
Copy link
Owner

ok let me know when it's ready to be merged, looks good from my point of view... also note that I pushed my new release already during lunch time and will do the other repos later tonight after work :)

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 12, 2024

yep seems to work even on the current one.
My first title was wrong on purpose and it found it. Changing it afterwards picked up the change event re-ran and it seems to work now.

I'm not sure if you're following standard conventional-changelog so I kept the defaults as are. If you'd like to tweak things lemme know. Aside from that I'm done with this one and its ready for merge

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 12, 2024

I'm not sure if you're following standard conventional-changelog so I kept the defaults as are. If you'd like to tweak things lemme know. Aside from that I'm done with this one and its ready for merge

Yeah that is indeed my preference, I use Lerna-Lite with the --conventional-commits enabled, so it does follow the conventional changelog. I've been using conventional changelog for years but never took the time to setup Husky, this is great, we'll have to add them to other repos as well (Angular-Slickgrid, Aurelia-Slickgrid, ...)

"version": {
"conventionalCommits": true,
"createRelease": "github",
"changelogIncludeCommitsClientLogin": " - by @%l",
"changelogHeaderMessage": "## All-in-One SlickGrid framework agnostic wrapper, visit [Slickgrid-Universal](https://github.com/ghiscoding/slickgrid-universal) 📦🚀",
"message": "chore(release): publish new version %v",
"syncWorkspaceLock": true
}

BTW, I guess your PR title in this case should be a chore: since it doesn't bring any feature to the user of this lib.

@zewa666 zewa666 changed the title feat: introduce husky commit-msg hook and pr title linter chore: introduce husky commit-msg hook and pr title linter Feb 12, 2024
@zewa666
Copy link
Contributor Author

zewa666 commented Feb 12, 2024

I was exactly thinking the same ... it's merely build tooling but ultimately wasn't sure. Good to have the re-confirmation.

yeah Husky is a great little tool full of helpful bits and pieces. Used it for tons of things like license checks, coverage mins etc.

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 12, 2024

Awesome, it's always nice to have contributions by power users like you :) Thanks again!

@ghiscoding ghiscoding merged commit b2f51bf into ghiscoding:master Feb 12, 2024
7 of 8 checks passed
@ghiscoding
Copy link
Owner

@zewa666 I had to adjust the commitlint rules because it failed on me because the body content went over 100 chars but I don't really care about the body, I do care about the title not being more than 72 chars though. So anyway, I adjusted as below, but I also would like to know if this can be moved to maybe an .mjs file? I'm trying to get away from CJS as much as possible and that should include config files too. I've never extended a config other than below so I'm not even sure if it's possible?

module.exports = {
extends: ['@commitlint/config-conventional'],
rules: {
'body-max-line-length': [1, 'always', 72],
'footer-max-line-length': [0, 'always'] // Make sure there is never a max-line-length by disabling the rule
},
};

Also another side note, I upgraded to the new @typescript-eslint v7 that got released today and the PR linter failed because it expect correct peer dependencies installed and that is no longer true with v7 (probably because not all dependencies are at v7 yet), so I switched to npm install --force to get rid of the error. However I also see that you fixed @commitlint/config-conventional at v17 even though I have v18 in the root package.json

@ghiscoding
Copy link
Owner

ahh so after searching on commitlint GitHub, I found that this works

I changed module.exports = to export default and it worked for me.

however, pull-request-name-linter-action doesn't support it yet :(

How to work with commitlint.config.ts (TypeScript config file)

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 13, 2024

@zewa666 ahh gosh I got a much bigger problem, it looks like Husky screwed up my release. I'm completely missing the typical git push. I think Husky blocked it for some reason but didn't exit. The release was created just before you added the PR linter.

I typically have a commit of this shape chore(release): publish new version 4.3.1 (here's the previous commit one from 2 weeks ago), it didn't create one for today's release for v4.4.0 and I don't see much in the CI but I do see Husky being triggered. I think it should be completely disabled when pushing a new release. I thought only my changelog weren't updated so I changed them by hand but then I realized the entire release commit is missing... So Lerna did create the release partially, it did create the GitHub Release and also created & pushed the tag but the entire commit is missing... This is bad... ideally I would like to disable it when releasing

(here's the link for the v4.4.0 release workflow which is shown below)

image
chore(release): publish new version 4.3.1`

Comparing today's release workflow with the previous release 2 weeks ago and I can see that there seem to be a race condition with Husky and Lerna

EDIT

I believe I found a solution, I can disable Husky in CI as per their docs CI server and Docker, it is very important that I disable Husky while pushing a new release. I'll have to update all packages versions by hand since that is completely missing... ah actually just running Lerna version in dry-run mode was enough to get commit changes (good thing I added this new dry-run mode in Lerna-Lite, else I would really have to do it by hand)

Well there's a lot of good lessons learned here lol. PR #1390 should hopefully resolve the issue

EDIT 2

Wait that seem to have caused other problems or side effect from butched release, some of my commits/PRs got removed completely. For example PR #1386 is gone from the git history for some reason, what the heck?? It looks like I'm missing all the PRs between 1385 and 1389 (including this PR in here too which is weird since I'm commenting in it)... wow that really screwed up a lot of things. I'm not sure how to reapply orphan PRs, at least we still have links to these PRs, they are just missing from git (possibly because of the Lerna release that got butched), I'll have to reapply these PRs manually tomorrow and possibly release another version, it should be ok now that I found how to disable Husky... wow that is a big story this Husky 😅

EDIT 3

I was able to restore the branch which include all of these PRs and created a new PR #1393, that should resolve the problem. I think it might have been caused by me actually now that I think of it, I tried to do a git force push and it didn't work and I might have caused all these issues without realizing it...nonetheless, disabling Husky when releasing is a must

image

and here's the list of latest PRs, I highlighted the missing PRs no longer showing in Git

image

ghiscoding added a commit that referenced this pull request Feb 13, 2024
* chore(release): publish new version 4.4.0

* docs: add missing commit to changelog

* chore: introduce husky commit-msg hook and pr title linter (#1385)

* chore: setup husky commit-msg hook with commitlint

* chore(deps): upgrade to major `@typescript-eslint` v7

* chore: use pnpm to install commitlint dep

* chore: add pnpm install to PR linter workflow

* chore: add pnpm install to PR linter workflow

* chore: pnpm install commitlint globally

* chore: pnpm install commitlint in workspace

* chore: revert to using npm install but with --legacy-peer-deps

* chore: revert to using npm install but with --force

---------

Co-authored-by: ghiscoding <[email protected]>
Co-authored-by: Vildan Softic <[email protected]>
Co-authored-by: ghiscoding <[email protected]>
ghiscoding added a commit that referenced this pull request Feb 13, 2024
* chore(release): publish new version 4.4.0

* docs: add missing commit to changelog

* chore: introduce husky commit-msg hook and pr title linter (#1385)

* chore: setup husky commit-msg hook with commitlint

* chore(deps): upgrade to major `@typescript-eslint` v7 (#1386)

* chore(deps): upgrade to major `@typescript-eslint` v7

* chore: migrate from node-archiver to fflate to zip standalone SF bundle (#1387)

* chore: replace bundle zip creation from `node-archiver` to `fflate`

* chore(deps): update to latest pnpm action v3
- latest pnpm action is using Node 20 and drops Node 16 which is EOL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants