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 golangci-lint to CI #154

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add golangci-lint to CI #154

wants to merge 6 commits into from

Conversation

jackatbancast
Copy link
Contributor

This change introduces the GolangCI linter into the CI, alongside
some sensible defaults for enabled linters.
This should help ensure that we are producing high quality code,
whilst raising potential issues

It is important to note that these configurations are not set in stone,
and are likely to change to suit the workflow of the project, enabling or
disabling behaviour as seen fit.

@jackatbancast jackatbancast marked this pull request as ready for review April 20, 2020 09:12
Copy link
Contributor

@benwh benwh left a comment

Choose a reason for hiding this comment

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

Other than the 1 comment it LGTM.

I do find it slightly hard, when reading the config file, to tell which options we've changed from the defaults. But on the other hand it is quite useful to have all of the config fields already populated, so I think it's fine.

.golangci.yml Outdated
new: false

# Show only new issues created after git revision `origin/master`
new-from-rev: origin/master
Copy link
Contributor

Choose a reason for hiding this comment

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

I already use golangci-lint locally and would prefer for this to not be set locally. Can we use to the --new-from-rev CLI flag instead?

It'd be really nice if we could target the base branch rather than just origin/master too, but apparently CircleCI don't give you an env var for that, there's various hacky solutions which people have come up with though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this too. I decided to specify it in the config as this would unify the local run with what is in CI, but happy to specify this as a flag in the step instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me personally it's confusing to not be seeing all linting errors across the project while working in my editor, and feels like it's less likely we'd ever go and clean the project up if this was the case. Happy to get another opinion though.

This change introduces the GolangCI linter into the CI, alongside
some sensible defaults for enabled linters.
This should help ensure that we are producing high quality code,
whilst raising potential issues

It is important to note that these configurations are not set in stone,
and are likely to change to suit the workflow of the project, enabling or
disabling behaviour as seen fit.
This commit neglects to handle or acknowledge the error potentially
raised by the ``logger.Log(...)`` call, and so should fail CI.
This change moves the specification of the diff to check against to
the invocation of the command, allowing local runs to run against the
entire project.

Thanks @benwh.
This change is a test to determine if CI builds are failing due to OOM
errors.
This should also increase the performance of the linting as the
concurrency will match the number of available CPU cores.
Copy link
Contributor

@benwh benwh left a comment

Choose a reason for hiding this comment

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

LGTM but it appears to flake due to excessive memory usage :(

We've agreed to take another look after the Kubebuilder upgrade

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants