-
Notifications
You must be signed in to change notification settings - Fork 25
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
Added source code linting to Groovy code and Jenkinsfile #325
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #325 +/- ##
=========================================
Coverage 87.43% 87.43%
Complexity 29 29
=========================================
Files 81 81
Lines 207 207
Branches 11 11
=========================================
Hits 181 181
Misses 19 19
Partials 7 7 |
Signed-off-by: chawinphat <[email protected]>
Hi @chawinphat Thank you for the contribution. We also have (most of the) groovy code living in |
Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing this feature @chawinphat .
Couple of questions mentioned below. Overall looks good.
@@ -1,5 +1,5 @@ | |||
{ | |||
"extends": "recommended", | |||
"extends": "recommended-jenkinsfile", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between recommended and recommended-jenkinsfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the linter's recommended format which lints jenkinsfiles too. When using just 'recommended' the linter catches errors that are not actually errors and breaks the program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some examples what failures are you encountering? I am doubtful what to use as well. On high level looks like recommended
is best path for groovy. recommended-jenkinsfile
seems to be applicable for jenkinsFiles specifically? Please correct me if I am wrong.
Adding @prudhvigodithi @peterzhuamazon @dblock to get some input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One error I saw was that the linter complained that 'variable x was not used in the file' even when x was referenced inside a string in that file.
Running the linter's automatic fixing caused tests to fail if I used the 'jenkins' setting while it was able to build and test fine with 'jenkins-file'
Description
Describe what this change achieves.
Created a pre-commit hook to run a linter for groovy and jenkinsfiles.
I used npm-groovy-lint and the pre-commit framework. The pre-commit hook will run after calling
git commit
and will run commands specified on the.pre-commit-config.yaml file.
. In this case, we will runnpm-groovy-lint
on all directories specified in the 'entry' field.Screenshot below shows the result on linting tests/jenkins, src/jenkins, and /vars.
I also fixed each error in tests/jenkins, vars, and src/jenkins.
Issues Resolved
#320
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.