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 linter for Go #710

Merged
merged 15 commits into from
Oct 11, 2024
Merged

Add linter for Go #710

merged 15 commits into from
Oct 11, 2024

Conversation

maci3jka
Copy link
Contributor

@maci3jka maci3jka commented Oct 2, 2024

This PR improves the development experience. It adds to go checks the linter golangci-lint, aggregating the number of liners and adding annotations on the PR with found issues. Additionally, it introduces go.lint target to makefile which allows running the same set of linters locally.

@maci3jka
Copy link
Contributor Author

maci3jka commented Oct 8, 2024

example of working https://github.com/canonical/k8s-snap/pull/722/files

image

@maci3jka maci3jka marked this pull request as ready for review October 9, 2024 06:44
@maci3jka maci3jka requested a review from a team as a code owner October 9, 2024 06:44
Copy link
Contributor

@HomayoonAlimohammadi HomayoonAlimohammadi left a comment

Choose a reason for hiding this comment

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

Thanks a lot @maci3jka, Almost LGTM. Just minor comments and questions.

golangci.yml Outdated Show resolved Hide resolved
.github/workflows/go.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

Going in the right direction.

Please enable all, see how much issues we get and post that here. Then we can create a PR that branches from this PR and fix all of those issues (if they are straightforward).

.github/workflows/go.yaml Outdated Show resolved Hide resolved
golangci.yml Outdated Show resolved Hide resolved
.github/workflows/go.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@HomayoonAlimohammadi HomayoonAlimohammadi left a comment

Choose a reason for hiding this comment

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

Thanks a lot. This is wonderful. Now we have a shit ton of linting problems to fix. Merging this would cause the k8s-snap pipeline to get stuck in a failing state until everything is fixed which is not good. We can do 2 things:

  1. Keep the PR open and gradually fix the issues
  2. Change the base from main to a custom branch.

I would go with option 1 because when we're done with the feature branch, rebasing it on main will be a huge pain and is super error prone.
Options 1 is itself challenging tho. We will still need to make sure we constantly rebase on main to prevent a huge and problematic/conflicting rebase later on.
WDYT @bschimke95?

Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

LGTM

@bschimke95
Copy link
Contributor

I added a list of all linters and disabled them. Now I will gradually enable all of the linters and fix the relevant code pieces as separate PRs.

@bschimke95 bschimke95 merged commit c618f09 into main Oct 11, 2024
18 of 19 checks passed
@bschimke95 bschimke95 deleted the KU-1165/add-golangci-lint branch October 11, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants