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

Integrate GolangCI-Lint #658

Closed
hanzei opened this issue Sep 15, 2020 · 7 comments · Fixed by #712
Closed

Integrate GolangCI-Lint #658

hanzei opened this issue Sep 15, 2020 · 7 comments · Fixed by #712
Labels
Difficulty/3:Hard Hard ticket Help Wanted Community help wanted Tech/Go Type/Task A general task

Comments

@hanzei
Copy link
Collaborator

hanzei commented Sep 15, 2020

Mattermost is currently in the process of using golangci-lint as the default linter for all go projects. mattermost-community/mattermost-plugin-autolink#108 serves as an example of how the migration should look like:

  1. Copy .golangci.yml from https://github.com/mattermost/mattermost-plugin-autolink/ into this repository.
  2. Copy Makefile from https://github.com/mattermost/mattermost-plugin-autolink/ into this repository.
  3. Copy build/ from https://github.com/mattermost/mattermost-plugin-autolink/ into this repository.
  4. Run go mod tidy and make dist to ensure the build system works fine.
  5. Update linters-settings.goimports.local-prefixes to the go module path i.e. to github.com/mattermost/mattermost-plugin-aws-SNS.
  6. Run make golangci-lint and fix all issues that arise.

Fixing an linter issue is sometimes trivial but can also be quite complicated. There might be cases, where ignoring an issue using issues.exclude-rules is needed. If you don't know how to resolve an error, please reach out to @hanzei on the Mattermost Community Server or ask the question here.

@hanzei hanzei added Help Wanted Community help wanted Up For Grabs Ready for help from the community. Removed when someone volunteers Difficulty/3:Hard Hard ticket Tech/Go Type/Task A general task labels Sep 15, 2020
@carantunes
Copy link
Contributor

Hi again! Then can I do this one?

@hanzei
Copy link
Collaborator Author

hanzei commented Sep 30, 2020

All yours 👍

@hanzei hanzei removed the Up For Grabs Ready for help from the community. Removed when someone volunteers label Sep 30, 2020
@hanzei
Copy link
Collaborator Author

hanzei commented Oct 27, 2020

Hey @carantunes, I just want to check how you are doing with the ticket. Do you have any questions?

@hanzei hanzei added the Up For Grabs Ready for help from the community. Removed when someone volunteers label Nov 3, 2020
@hanzei
Copy link
Collaborator Author

hanzei commented Nov 3, 2020

Making this available for the public agian

@carantunes
Copy link
Contributor

I'm sorry for my absence, I've been quite busy with work.

I did however start a PR (just pushed it), there's are quite a lot of fixes to be done still. I'm working on it, and will ask you some question about the more tricky ones.

@carantunes
Copy link
Contributor

There are a lot of gosec warning which I believe will be the more tricky. Should I just #nosec?

A sample:

server/expvar/endpoint_test.go:67:7: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
        r := rand.New(rand.NewSource(time.Now().UnixNano()))
             ^
server/utils/kvstore/hashed_key.go:7:2: G501: Blocklisted import crypto/md5: weak cryptographic primitive (gosec)
        "crypto/md5"
        ^
server/utils/kvstore/hashed_key.go:76:7: G401: Use of weak cryptographic primitive (gosec)
        h := md5.New()
             ^
server/http.go:47:2: G101: Potential hardcoded credentials (gosec)
        routeACUserRedirectWithToken                = "/ac/user_redirect.html"
        ^
server/kv.go:7:2: G501: Blocklisted import crypto/md5: weak cryptographic primitive (gosec)
        "crypto/md5"
        ^
server/kv.go:89:7: G401: Use of weak cryptographic primitive (gosec)
        h := md5.New()
             ^
server/kv.go:95:7: G401: Use of weak cryptographic primitive (gosec)
        h := md5.New()
             ^
server/kv.go:370:20: G403: RSA keys should be at least 2048 bits (gosec)
                newRSAKey, err = rsa.GenerateKey(rand.Reader, 1024)
                                 ^
server/stats.go:4:2: G501: Blocklisted import crypto/md5: weak cryptographic primitive (gosec)
        "crypto/md5"
        ^
server/stats.go:96:8: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
                r := rand.New(rand.NewSource(time.Now().UnixNano()))
                     ^
server/command_test.go:299:22: G403: RSA keys should be at least 2048 bits (gosec)
                                conf.rsaKey, _ = rsa.GenerateKey(rand.Reader, 1024)
                                                 ^
server/http_test.go:104:18: G601: Implicit memory aliasing in for loop. (gosec)
                                                foundSub = &savedSub
                                                           ^
server/jira_test_util_test.go:29:19: G601: Implicit memory aliasing in for loop. (gosec)
                ret.Channel.add(&sub)
                                ^


@hanzei hanzei removed the Up For Grabs Ready for help from the community. Removed when someone volunteers label Nov 9, 2020
@hanzei
Copy link
Collaborator Author

hanzei commented Nov 9, 2020

@carantunes Awesome, thanks for pushing that PR 👍

Yes, the md5 related warning can be ignored using nosec. I'm not sure about the G601. Could you double check if they are valid?

hanzei added a commit that referenced this issue Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty/3:Hard Hard ticket Help Wanted Community help wanted Tech/Go Type/Task A general task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants