Adding lint commit and husky to root configuration#4311
Conversation
|
|
|
Welcome @dibyanshu-pal-kushwaha! |
|
Hey hey. please check the contribution commit guidelines. There are some examples of commit messages that are good and bad in there. We do not use fix and chore for example. |
|
cool, thanks. Please check the failed checks to see what the errors are? Can you please rebase and squash your commits down to just the ones to be merged? Probably there should be one or two commits. |
There was a problem hiding this comment.
Pull request overview
This PR adds automated commit message validation by integrating Husky git hooks and commitlint at the root level. The implementation aims to enforce the project's commit guidelines that require an <area>: <description> format for all commit messages.
Key Changes
- Moved Husky from frontend to root configuration with upgraded version (v4.3.8 → v9.1.7)
- Added
lint:commitscript to validate commit messages against the main branch - Created custom commitlint configuration with a
headlamp-formatrule to validate commit message structure - Set up git hooks for commit-msg validation and pre-commit linting
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added husky to root devDependencies, created prepare and lint:commit scripts, upgraded concurrently version |
| package-lock.json | Added extensive new dependencies for eslint, prettier, lint-staged, and related tooling |
| frontend/package.json | Removed husky dependency and husky.hooks configuration, but contains critical JSON syntax errors with duplicate lint-staged configurations |
| commitlint.config.js | Created custom headlamp-format rule to validate commit message format with regex pattern |
| .husky/commit-msg | Added hook to run commitlint on commit message creation |
| .husky/pre-commit | Added hook to run lint-staged on pre-commit |
Comments suppressed due to low confidence (1)
frontend/package.json:233
- There are three duplicate lint-staged configurations in this file (lines 126-157, 163-194, and 202-233). This is redundant and will cause confusion about which configuration is actually used. Only one lint-staged configuration should be present at the top level of package.json.
"lint-staged": {
"src/**/*.{js,jsx,ts,tsx}": [
"eslint -c package.json --fix"
],
"src/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../app/**/*.{ts,tsx}": [
"eslint -c package.json --fix"
],
"../app/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../plugins/headlamp-plugin/{bin,lib}/**/*.{ts,tsx}": [
"eslint -c package.json --fix"
],
"../plugins/headlamp-plugin/{bin,lib}/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../plugins/examples/**/*.{ts,tsx}": [
"eslint -c package.json --fix --resolve-plugins-relative-to ."
],
"../plugins/examples/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../e2e-tests/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../e2e-tests/**/*.{ts,tsx}": [
"eslint -c package.json --fix --resolve-plugins-relative-to ."
]
},
"browserslist": {
"production": [
">0.2%",
"not dead",
"not op_mini all"
],"lint-staged": {
"src/**/*.{js,jsx,ts,tsx}": [
"eslint -c package.json --fix"
],
"src/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../app/**/*.{ts,tsx}": [
"eslint -c package.json --fix"
],
"../app/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../plugins/headlamp-plugin/{bin,lib}/**/*.{ts,tsx}": [
"eslint -c package.json --fix"
],
"../plugins/headlamp-plugin/{bin,lib}/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../plugins/examples/**/*.{ts,tsx}": [
"eslint -c package.json --fix --resolve-plugins-relative-to ."
],
"../plugins/examples/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../e2e-tests/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../e2e-tests/**/*.{ts,tsx}": [
"eslint -c package.json --fix --resolve-plugins-relative-to ."
]
},
"development": [
"last 1 chrome version",
"last 1 firefox version",
"last 1 safari version"
]
},
"lint-staged": {
"src/**/*.{js,jsx,ts,tsx}": [
"eslint -c package.json --fix"
],
"src/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../app/**/*.{ts,tsx}": [
"eslint -c package.json --fix"
],
"../app/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../plugins/headlamp-plugin/{bin,lib}/**/*.{ts,tsx}": [
"eslint -c package.json --fix"
],
"../plugins/headlamp-plugin/{bin,lib}/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../plugins/examples/**/*.{ts,tsx}": [
"eslint -c package.json --fix --resolve-plugins-relative-to ."
],
"../plugins/examples/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../e2e-tests/**/*.{js,jsx,ts,tsx,json,css,scss,md}": [
"prettier --config package.json --write"
],
"../e2e-tests/**/*.{ts,tsx}": [
"eslint -c package.json --fix --resolve-plugins-relative-to ."
]
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
88c3e35 to
60e40bf
Compare
|
@dibyanshu-pal-kushwaha Can you please check the comments by the copilot? Comment for ones that you have addressed “I have done this”, or write if it does not make sense “this does not make sense because of these reasons…” |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Can you please address the open items?
I think also “frontend: Add nnn” should be disallowed.
Please add the areas you are changing, not just frontend. For example “frontend: resourceMap: Fix performance issue with clicking on items”. This is because many changes are in backend, so it does not give enough context really.
Same with “backend: Add feature” should be disallowed.
Please add the areas you are changing, not just “backend:”. For example “backend: auth: Add new test for logging in”. This is because many changes are in backend, so it does not give enough context really.
This is because many changes are in frontend or backend, so it does not give context really.
illume
left a comment
There was a problem hiding this comment.
Thanks for all of that.
I resolved all those open conversations you had finished, and will look into it in more detail later.
Probably the commits could be squashed into one again.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8ad3773 to
5d1d93a
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks. I marked some more comments resolved.
Check out your commit messages… there should be a capitalized verb, and also you have a space before the : which should not be there.
illume
left a comment
There was a problem hiding this comment.
Can you please rebase against main to get rid of the merge commits?
044bd4b to
7458fc0
Compare
6aa27bc to
3b300fc
Compare
5fe2c9e to
42c58c9
Compare
|
@illume , Please review it , I have rebased it and now it is merge conflict free. |
|
Thanks for that change. Can you please ask in the headlamp kubernetes slack channel for someone else to review and test this? |
42c58c9 to
aac9f67
Compare
|
@dibyanshu-pal-kushwaha can anything be done about the failing tests? or are they due to some other change (not related to this PR)? |
|
I am working over it , trying to solve. |
|
@M-DEV-1 I fixed the changes, run the frontend test locally , and other tests, too, it worked. Please have a look and help me if anything , I missed. |
|
The frontend/src/lib/k8s/api/v2/multiplexer.ts change should be removed from here into to a separate PR. Please always check the commits locally (no merge commits), you can you please rebase against main to get rid of the merge commits? Also, please always check the other things locally first ( |
illume
left a comment
There was a problem hiding this comment.
Left some notes of things to tidy up.
7a63132 to
0fb4ff1
Compare
fsfsdf Restoring the changes
0fb4ff1 to
35de70e
Compare
|
Cool, looks like it's passing the Github checks now. Thanks for tidying up those things. I will test it out in the coming days. |
Summary
This PR fixes the lack of automated commit message validation and integrates Husky to ensure contribution guidelines are followed.
Related Issue
Fixes #4303
Changes
Steps to Test
2.Try to create any commit with wrong format (for example : git commit -m "test commit")