search

k8snetworkplumbingwg / ovs-cni

Open vSwitch CNI plugin
Apache License 2.0
224 stars 71 forks source link

fix golint warnings #140

Closed pperiyasamy closed 3 years ago

pperiyasamy commented 4 years ago

This fixes golint code style warnings across go modules in the project. It is makes golint a mandatory prerequisite for CI to pass.

Signed-off-by: Periyasamy Palanisamy periyasamy.palanisamy@est.tech

pperiyasamy commented 4 years ago

/release-note-none

phoracek commented 4 years ago

Thanks @pperiyasamy. I'm sure we'd break this again unless we introduce a CI check. Would you please add a new make target with this sanity check and called it from https://github.com/kubevirt/ovs-cni/blob/master/automation/check-patch.e2e.sh#L19?

pperiyasamy commented 4 years ago

/cc @JanScheurich

kubevirt-bot commented 4 years ago

@pperiyasamy: GitHub didn't allow me to request PR reviews from the following users: JanScheurich.

Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/kubevirt/ovs-cni/pull/140#issuecomment-727961396): >/cc @JanScheurich Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
phoracek commented 4 years ago

CI failed on flag provided but not defined: -junit-output. It seems unrelated. @RamLavi could it be related to recent Go changes?

RamLavi commented 3 years ago

CI failed on flag provided but not defined: -junit-output. It seems unrelated. @RamLavi could it be related to recent Go changes?

it passes on master

pperiyasamy commented 3 years ago

CI failed on flag provided but not defined: -junit-output. It seems unrelated. @RamLavi could it be related to recent Go changes?

it passes on master

Yes, this is what so strange to me, i could see junit-output is passed like this --junit-output=$ARTIFACTS/junit.functest.xml, where is ARTIFACTS variable set ?

pperiyasamy commented 3 years ago

CI failed on flag provided but not defined: -junit-output. It seems unrelated. @RamLavi could it be related to recent Go changes?

it passes on master

Yes, this is what so strange to me, i could see junit-output is passed like this --junit-output=$ARTIFACTS/junit.functest.xml, where is ARTIFACTS variable set ?

I think we may have to parse junit-output arg at here to solve this issue.

phoracek commented 3 years ago

The issue was introduced by the second commit in this PR. I'd recommend you split the second PR even further since it mixes makefile, comment and code changes. Then we should be in a better place figuring out which change broke the junit parsing.

pperiyasamy commented 3 years ago

I'd recommend you split the second PR even further since it mixes makefile, comment and code changes. Then we should be in a better place figuring out which change broke the junit parsing.

@phoracek thanks, that's a good idea, let me split into smaller commits to see which change causes this issue.

pperiyasamy commented 3 years ago

@phoracek we now at least know what is the root cause of the problem, the go modules inside the tests/clusterand test/node throws golint warnings like below.

/tmp/ovs-cni/go/path/src/github.com/kubevirt/ovs-cni/.gopath/src/github.com/kubevirt/ovs-cni/tests/cluster/cluster.go:42:2: should not use dot imports
/tmp/ovs-cni/go/path/src/github.com/kubevirt/ovs-cni/.gopath/src/github.com/kubevirt/ovs-cni/tests/cluster/cluster.go:43:2: should not use dot imports
/tmp/ovs-cni/go/path/src/github.com/kubevirt/ovs-cni/.gopath/src/github.com/kubevirt/ovs-cni/tests/cluster/cluster.go:47:6: type name will be used as cluster.ClusterAPI by other packages, and that stutters; consider calling this API
/tmp/ovs-cni/go/path/src/github.com/kubevirt/ovs-cni/.gopath/src/github.com/kubevirt/ovs-cni/tests/node/node.go:27:2: should not use dot imports
/tmp/ovs-cni/go/path/src/github.com/kubevirt/ovs-cni/.gopath/src/github.com/kubevirt/ovs-cni/tests/node/node.go:28:2: should not use dot imports
Makefile:40: recipe for target 'lint' failed
make: *** [lint] Error 1
+ EXIT_VALUE=2
+ set +o xtrace

To fix this, I've renamed the files into *_test.go, after renaming, each test modules needed flag parsing (actually this was causing build breakage in earlier CI runs with unknow flag error). After parsing the flags, now build is failing as there is no tests in these modules.

pperiyasamy commented 3 years ago

@phoracek i've made golint check to be skipped for tests/cluster and tests/node packages and build is passing now. i think we're okay with that.

phoracek commented 3 years ago

Sounds good. Thanks for the patience, Peri.

kubevirt-bot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phoracek, pperiyasamy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubevirt/ovs-cni/blob/master/OWNERS)~~ [phoracek] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
phoracek commented 3 years ago

/hold

One second, why is this assert dropped: https://github.com/kubevirt/ovs-cni/pull/140/files#diff-547c2ca2c8f324d2f8cda404f94afc1d9da30e5292aa2703f68f16683365a136L122-L124

pperiyasamy commented 3 years ago

why is this assert dropped:

@phoracek oops, i guess git rebase didn't happen properly, Thanks for figuring this out. now restored this change.