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

Initial adoption of golangci-lint for continuous integration #278

Merged
merged 42 commits into from
Aug 10, 2020
Merged

Initial adoption of golangci-lint for continuous integration #278

merged 42 commits into from
Aug 10, 2020

Conversation

vzamanillo
Copy link
Contributor

@vzamanillo vzamanillo commented Jul 23, 2020

Implements #277

Needs revision, it should be set as a draft for further improvement before confirming the merge.

There are several linters that I have disabled because for now it is a lot of work to correct the errors, for example

  • funlen: Detection of long functions.
  • gocyclo: Calculates cyclomatic complexities.
  • gosec: Inspects source code for security problems.
  • lll: Line length linter, used to enforce line length in files.

and I have added some directives to skip the checks with (nolint) because they need a small refactor but I think it's fine for now, we can improve it in the future.

Changes need in-depth review to make sure nothing is broken and that they are appropriate.

If you want to test this locally with https://github.com/nektos/act you'll have to add an additional step before "Set up Go" due to a dependencies error of the nektos/act default docker image.

    steps:
      - name: Install dependencies
        run: |
          apt update
          apt install gcc build-essential --assume-yes --quiet --no-install-suggests --no-install-recommends

      - name: Set up Go
        uses: actions/setup-go@v2
        with:
          go-version: 1.13

Note I added a new step golangci-lint after Build step instead of adding a new workflow file as https://github.com/golangci/golangci-lint-action#how-to-use recommends, but in the future we could move it to its own file to have better control over the actions of the GitHub workflow.

I think the configuration is not very strict but it ensures a minimum quality of the code, if you consider it appropriate we can add other linters to make it more strict, but we have to think about it in order to facilitate the work of the collaborators in new pull requests.

We have an open discussion here.

I have do my best, I apologize in advance for any possible problems that may arise.

use strings.ReplaceAll method instead off strings.Replace (gocritic)
Should use `time.Since` instead of `time.Now().Sub`
Fixed running 'gofmt -s -w file'
len(matchTokens) <= 0 can be len(matchTokens) == 0
We passet huge parameters by pointer
Can re-write `<a href="([A-Za-z0-9\/.]+)"><b>` as `<a href="([A-Za-z0-9/.]+)"><b>`
@Ice3man543
Copy link
Member

@vzamanillo wow, this is amazing work! It'll take some more work to make things proper however this is a good start.

@vzamanillo
Copy link
Contributor Author

@vzamanillo wow, this is amazing work! It'll take some more work to make things proper however this is a good start.

Thank you very much @Ice3man543! as you said, it is a good start.

@Ice3man543
Copy link
Member

@vzamanillo As it's a very big PR, give me some time to look over the changes. I'll work on this PR and merge this ASAP!

@vzamanillo
Copy link
Contributor Author

@vzamanillo As it's a very big PR, give me some time to look over the changes. I'll work on this PR and merge this ASAP!

Don't worry, we are not hurry, it is better to make sure nothing is broken before merge. Tell me if you need help with anything.

We separate the logic to upload to chaos to preserve the SRP away from utils.
@vzamanillo
Copy link
Contributor Author

Build fails because we are running go get before running this action: fetched $GOPATH/pkg/mod conflict. Ref: golangci/golangci-lint-action#23
@vzamanillo
Copy link
Contributor Author

Hehehe, it works...

linting

@vzamanillo
Copy link
Contributor Author

Can we merge this?, I have a lot of cool things on my mind but I am waiting to this PR has been released.

Do you need help to review it?

@ehsandeep ehsandeep marked this pull request as ready for review August 9, 2020 20:23
@ehsandeep
Copy link
Member

Hi @vzamanillo,

Thank you for confirming and letting us know, this PR is ready for the review and merge, we will this merge this in a day or two, thanks a lot for putting all the work on this.

@ehsandeep ehsandeep added the Status: Review Needed The issue has a PR attached to it which needs to be reviewed label Aug 9, 2020
@ehsandeep ehsandeep requested a review from Mzack9999 August 9, 2020 20:27
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Thanks a lot for cleaning up the code!

@vzamanillo
Copy link
Contributor Author

Awesome work! Thanks a lot for cleaning up the code!

Thanks!

@ehsandeep ehsandeep merged commit 2c3b5cc into projectdiscovery:master Aug 10, 2020
@vzamanillo vzamanillo deleted the golangci-lint branch August 10, 2020 06:59
@ehsandeep ehsandeep added Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Completed Nothing further to be done with this issue. Awaiting to be closed. and removed Status: Review Needed The issue has a PR attached to it which needs to be reviewed labels Aug 10, 2020
@ehsandeep ehsandeep linked an issue Aug 10, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Completed Nothing further to be done with this issue. Awaiting to be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add golangci-lint-action to the existing GitHub workflow
4 participants