search

gardener / machine-controller-manager

Declarative way of managing machines for Kubernetes cluster
Apache License 2.0
256 stars 117 forks source link

Fix linting issues in `test`, `handlers` and `features` packages #795

Closed afritzler closed 1 year ago

afritzler commented 1 year ago

What this PR does / why we need it: Fix linting issues in test, handlers and features packages.

Release note:

NONE
gardener-robot commented 1 year ago

@afritzler Thank you for your contribution.

afritzler commented 1 year ago

What is the reasoning behind forbidding dot imports?

./pkg/test/integration/common/framework.go:18:2: should not use dot imports
./pkg/test/integration/common/framework.go:19:2: should not use dot imports
./pkg/test/integration/common/framework.go:20:2: should not use dot imports

In the context of gomega and ginkgo dot imports IMO are pretty handy.

unmarshall commented 1 year ago

In the context of gomega and ginkgo dot imports IMO are pretty handy.

I agree, it should not be removed in tests. It should however not be used in non-test code.

himanshu-kun commented 1 year ago

should not use dot imports

For now I don't find any way to add it as an exception to golint . We will switch to golangci-lint where we could configure for this particular error. For the time being lets avoid dot imports @afritzler . Kindly update the PR , I'll merge it quickly.

afritzler commented 1 year ago

Regarding the dot-imports. I can revert this part. Didn't mean to break things on your side.

unmarshall commented 1 year ago

kindly also remove the dot imports for now, as we don't have a way to ignore those in golint

Just to add to this point. We are not against usage of dot-imports in tests as it is widely used across several gardener projects and is also listed as an exception to the general rule for using dot-imports. See https://go.dev/doc/go1compat 

Dot imports. If a program imports a standard package using import . "path", additional names defined in the imported package in future releases may conflict with other names defined in the program. We do not recommend the use of import . outside of tests, and using it may cause a program to fail to compile in future releases.

https://github.com/golang/lint is deprecated and once we move to golangci-lint then we can explicitly add a rule to allow dot-imports in tests. Till then just have a shorter alias for ginkgo and gomega.

himanshu-kun commented 1 year ago

Regarding the dot-imports. I can revert this part. Didn't mean to break things on your side. @afritzler waiting for your changes. We are planning a release , and would like to get this change in.

afritzler commented 1 year ago

Give me a sec here.

afritzler commented 1 year ago

Dot imports have been removed and the branch has been rebased to master.

afritzler commented 1 year ago

@himanshu-kun I am not sure I like this change as it now includes the gardener/gardener dependency which results in a huge PR. change.

himanshu-kun commented 1 year ago

@himanshu-kun I am not sure I like this change as it now includes the gardener/gardener dependency which results in a huge PR. change.

Yes definitely, we shouldn't add g/g as dependency. My proposal was more towards copying the relevant code from g/g in some util file, so that we could use the matcher

afritzler commented 1 year ago

Alright, I copied the matches over into test/utils/matchers.

himanshu-kun commented 1 year ago

/milestone v0.49