-
Notifications
You must be signed in to change notification settings - Fork 22
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
GH-30 initial integration of golangci-lint #44
GH-30 initial integration of golangci-lint #44
Conversation
Hello @hectorgabucio, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
Codecov Report
@@ Coverage Diff @@
## master #44 +/- ##
=========================================
- Coverage 5.82% 5.80% -0.03%
=========================================
Files 6 6
Lines 738 741 +3
=========================================
Hits 43 43
- Misses 677 680 +3
Partials 18 18
Continue to review full report at Codecov.
|
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 the contribution 👍
Plugins use p.API.Log*
instead of log
. See https://developers.mattermost.com/extend/plugins/server/reference/#API.LogWarn for the docs. Would you please change that?
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 nit pick, the rest looks good 👍
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.
Great work 🎉
@hectorgabucio Could you please resolve the merge conflict? |
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.
Awesome work @hectorgabucio!
@hanzei the linting failed https://app.circleci.com/pipelines/github/mattermost/mattermost-plugin-jenkins/75/workflows/be714750-99de-48fe-927c-f340a226f6ca/jobs/246 maybe we use a newer golang ci version in CI? |
@hanzei @mickmister conflicts resolved and CI passed |
Awesome, thanks @hectorgabucio 👍 @DHaussermann There are no function changes in this PR. I've successfully build and deployed the plugin locally. Any additional tests you want to make? |
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.
Tested and passed
- Built from this branch - no lint issues with full
make
- Enabled the plugin and tested that slash commands seem functional
- I will this during end to end testing before the next release for a thorough regression test
LGTM!
Thanks @hectorgabucio for this enhancement!
Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour. |
cc @mickmister @hanzei please help with merging if |
Trying to auto merge this PR. |
Pull Request successfully merged |
Summary
Integrates GolangCI-Lint in mattermost jenkins plugin.
Ticket Link
Fixes #30