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

Add Golang linting in CI and remove unreachable code from the repo #184

Open
akshay196 opened this issue Mar 24, 2023 · 22 comments
Open

Add Golang linting in CI and remove unreachable code from the repo #184

akshay196 opened this issue Mar 24, 2023 · 22 comments
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@akshay196
Copy link
Member

Describe the issue you're facing

  • There are few unused funcs/packages that need to be removed. Run Golang lint to check other lint errors.
  • Add Golang lint job to CI, use https://golangci-lint.run/ or similar
@akshay196 akshay196 added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. good first issue Good for newcomers labels Mar 24, 2023
@bhavyastar
Copy link

Hey @akshay196 I would like to work on this issue.

@akshay196
Copy link
Member Author

@bhavyastar Thanks for showing interest.
There are quite a few lint errors that we need to fix as part of this issue. At the end we need to enable go lint checker in CI.
With the default linters enabled golangci-lint reports 184 errors.

$ golangci-lint run --out-format json | jq '.Issues | length'
184

Feel free to fix all at once or take a sequential approach. Let us know if you need any help.
Meantime I would like to take a look at different golangci-lint linters (from disabled list) that we can enable for us.

@akshay196 akshay196 added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 2, 2023
@theluckiesthuman
Copy link

@akshay196 Is the issue fixed, or can I work on this?

@akshay196
Copy link
Member Author

akshay196 commented May 30, 2023

@bhavyastar Are you still working on this issue?

@akshay196
Copy link
Member Author

@theluckiesthuman If you're interested, you can get started on this. Let us know if you need anything.

@theluckiesthuman
Copy link

@akshay196 Please assign me this.

@karthikmurali60
Copy link

@akshay196 can i work on this ?

@akshay196
Copy link
Member Author

@theluckiesthuman Are you still working on this?

@akshay196
Copy link
Member Author

@karthikmurali60 Feel free to take it up and raise PR once done.

@Omkar0114
Copy link

Hi @akshay196 I want to work on this issue!

@Omkar0114
Copy link

  • After running golangci-lint run --fix showing lint errors

image

  • But after this command golangci-lint run --enable-all --fix resolving lint errors automatically? Or we have to do it manually?

image

@akshay196
Copy link
Member Author

@Omkar0114 You can use automatic fix option of golangci-lint.
We also need to add the golang lint checker in CI (GitHub Actions) so that new changes/PRs are verified for linting errors.

Hope this is clear. Feel free to raise PR.

@Omkar0114
Copy link

@akshay196 Sure. I'll raise a PR soon. Also, I want to ask if we create the lint.yaml file separately under .github/workflows, or in go.yaml we can add the lint job?

@akshay196
Copy link
Member Author

akshay196 commented Sep 14, 2023

@Omkar0114 let's add lint check in go.yml itself.

@Omkar0114
Copy link

Cool. Thanks @akshay196

@akshay196
Copy link
Member Author

This issue is open for anyone interested. Partial PR for a reference #253

@nirav-rafay nirav-rafay added this to the v0.2.8 milestone Jan 19, 2024
@jaydee029
Copy link
Contributor

Hi, @akshay196 I would like to work on this.

@akshay196
Copy link
Member Author

Hey @jaydee029 Feel free to start working. No one actively working on it right now.
Let us know if you need any help.

@jaydee029
Copy link
Contributor

@akshay196 I'm assuming we resolve all the linting errors not just the unused func/types error? right?

@jaydee029
Copy link
Contributor

jaydee029 commented Apr 11, 2024

@akshay196 I'm assuming we resolve all the linting errors not just the unused func/types error? right?

If this is true, we would need to make certain structural changes such as error handling, since lot of the errors involved are missing error checks, as well as the way in which for loops are structured etc.

@akshay196
Copy link
Member Author

@jaydee029 sure, lets try to resolve all possible linting errors.
If it is going to be a big change then take iterative approach, fix few errors at first and then others.
Also we need to add CI check for linting so we avoid adding new errors in future.

@akshay196
Copy link
Member Author

@jaydee029 Thanks for your contribution. We have now added golangci-lint to GitHub workflows.
Feel free to start raising PRs to fix lint issues in code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants