Integrate GolangCI-Lint #100
Conversation
hanzei
added
2: Dev Review
| @@ -73,10 +73,10 @@ func TestOnConfigurationChange(t *testing.T) { | |||
| "same configuration": { | |||
| SetupAPI: func() *plugintest.API { | |||
| api := &plugintest.API{} | |||
| api.On("LoadPluginConfiguration", mock.Anything).Return(nil) | |||
| api.On("GetTeams").Return([]*model.Team{&model.Team{Id: teamId}}, nil) | |||
| api.On("LoadPluginConfiguration", mock.AnythingOfType("*main.configuration")).Return(nil) | |||
There was a problem hiding this comment.
Using mock.AnythingOfType("*main.configuration") is a bit more specific then mock.Anything
| return fmt.Errorf("Failed to upload plugin bundle: %s", resp.Error.Error()) | ||
| return errors.Wrap(resp.Error, "failed to upload plugin bundle") | ||
| } | ||
|
|
||
| log.Print("Enabling plugin.") | ||
| _, resp = client.EnablePlugin(pluginID) | ||
| if resp.Error != nil { | ||
| return fmt.Errorf("Failed to enable plugin: %s", resp.Error.Error()) | ||
| return errors.Wrap(resp.Error, "Failed to enable plugin") |
There was a problem hiding this comment.
Are we enforcing errors.Wrap instead of fmt.Errorf?
There was a problem hiding this comment.
Not, we are not enforcing this, but you are bringing up another great points.
The complain from golangci-lint for the original message is:
error strings should not be capitalized or end with punctuation or a newline (golint), which is actually not fixed with the change. golangci-lint just doesn't find the capitalized message.
But given that build is just a copy from https://github.com/mattermost/mattermost-plugin-starter-template I leaning towards not fixing it in this PR.
| @@ -47,6 +47,7 @@ func TestServeHTTP(t *testing.T) { | |||
|
|
|||
| result := w.Result() | |||
| require.NotNil(t, result) | |||
| defer result.Body.Close() | |||
|
@hanzei, I just ran
|
|
@jfrerich What does |
|
I've got version 1.25.1. It was released yesterday https://github.com/golangci/golangci-lint/releases |
|
@jfrerich I'm fairly confident that the failure you are reporting was introduced with golangci/golangci-lint@dfb2278, which is part of |
|
@hanzei, I agree that ignoring is ok if fine. I've added the |
|
@jfrerich Would you please report the gosec issue on https://github.com/mattermost/mattermost-plugin-starter-template? |
|
@hanzei. Done. |
|
@jfrerich Is there anything that needs to be done in this PR before you approve it? |
|
@DHaussermann This is the last PR that should go into |
|
/update-branch |
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
==========================================
- Coverage 17.10% 17.07% -0.03%
==========================================
Files 14 14
Lines 1175 1177 +2
==========================================
Hits 201 201
- Misses 934 935 +1
- Partials 40 41 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I agree with @hanzei
Tested and passed.
- Ensured there where no build errors
- Enabled plugin and briefly tested core functionality
- We can rely on end to end testing pre-release for more thorough test coverage
LGTM!
DHaussermann
added
4: Reviews Complete
Summary
Downstream the
golangci-lintconfig from mattermost/mattermost-plugin-starter-template#90 and syncbuild.Most changes are straightforward and just style wise. I did comment on the more tricky ones.
Ticket Link
Follow up on mattermost-community/mattermost-plugin-autolink#108