Remove dependencies on unused tools, install tools explicitly instead of via go.mod #3355

rbroggi · 2021-10-29T18:28:56Z

Short description of the changes

golint has been deprecated and is no longer used in this repository. Eliminating the useless import in the tools.go.

Signed-off-by: rbroggi [email protected]

codecov · 2021-10-29T18:36:40Z

Codecov Report

Merging #3355 (996d1e0) into master (b1254bd) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3355      +/-   ##
==========================================
+ Coverage   96.45%   96.47%   +0.01%     
==========================================
  Files         259      259              
  Lines       15166    15166              
==========================================
+ Hits        14629    14631       +2     
+ Misses        453      451       -2     
  Partials       84       84

Impacted Files	Coverage Δ
pkg/config/tlscfg/cert_watcher.go	`94.73% <0.00%> (+2.10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1254bd...996d1e0. Read the comment docs.

yurishkuro · 2021-10-29T18:36:59Z

tools.go

@@ -25,5 +25,4 @@ import (
 	_ "github.com/securego/gosec/cmd/gosec"
 	_ "github.com/vektra/mockery/cmd/mockery"
 	_ "github.com/wadey/gocovmerge"
-	_ "golang.org/x/lint/golint"


actually, we need to replace it with import of github.com/golangci/golangci-lint, because it is being explicitly installed by version in install-tools Makefile target instead of using go.mod as we do with other tools.

what about simply removing this file? It seems build tag 'tool' is nowhere called...

github.com/golangci/golangci-lint is not a valid package (it's a binary)

This file is needed so that the respective packages are added to go.mod and when we do go install we use the versions pinned in go.mod, not the random/latest ones.

I am not sure what you mean that github.com/golangci/golangci-lint is not a valid package - it's the source code of the tool , we currently install it as

go install github.com/golangci/golangci-lint/cmd/[email protected]

but once we have it in go.mod with the version we should just install it with go install github.com/golangci/golangci-lint/cmd/golangci-lint

Ok, I have tried to replicate your suggestion to the best of my understanding. Please notice that:

having golangci-lint in our go.mod is bringing a lot of indirect dependencies as well (see the go.mod file). This is a consequence of the nature of golangci-lint - it is a linter aggregator and therefore it declares all the supported linters as its dependencies. In Jaeger case this might be a big price to pay given that we are only using 3 out of the very long list of supported linters.

Since we do not use gosec directly but only indirectly through golangci-lint I have removed the explicit dependency from the tools.go file.

I have removed altogether the import of honnef.co/go/tools/cmd/staticcheck as we are not using it directly or indirectly. (correct me if I'm wrong)

I took the liberty to merge install-mockery into install-tools target because it felt tidier IMO. Please let me know if you prefer to keep them separate (and if there is a rational for that I would be interested to know as well).

Hm, I think it was a mistake that staticcheck was removed https://github.com/jaegertracing/jaeger/pull/3237/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L169 -- cc @albertteoh

The staticcheck linter is included by default: https://golangci-lint.run/usage/linters/#enabled-by-default-linters

Confirmed with:

$ make lint ... INFO [lintersdb] Active 11 linters: [deadcode gofmt gosec gosimple govet ineffassign staticcheck structcheck typecheck unused varcheck]

I wanted to complement with some info:

As a matter of fact in the official golangci-lint documentation the go install github.com/golangci/golangci-lint/cmd/golangci-lint@<version> is the advised best approach.

Some things I think we should tackle nevertheless:

Having the tools versioning in tools.go or in make install-tools sounds good to me, but I think that it would be simpler/clearer if we pick one instead of using a hybrid approach (some tools versioned in go.mod and others explicitly specified in the install-tools). This is why earlier I suggested deleting the tools.go. In the context of this PR I suggest we only do a cleanup and follow up on actions 3 and 4 below. To be completely honest I have a preference to handle the tool dependencies outside of go.mod because the fact that we are using only go tools is incidental and I think it makes more sense to centralize the tooling dependency in a language-agnostic place (like the make install-tools target).

Even if you choose to keep the tools.go we do not need to declare gosec and staticcheck as those are indirect dependencies of golangci-lint and will be taken care of by install-tools target.

(to be done in separate PR) https://github.com/wadey/gocovmerge last commit was in 2016 - I would consider either:

vendoring it

copying it to this repository

since the script is very simple, create our own version and maintain it in the scope of this repo

(to be done in separate PR) considering to upgrade mockery to v2.

I thought we don't need gocovmerge anymore since we no longer test pkg by pkg

I think you are absolutely right :)

I have updated the PR.

yurishkuro · 2021-10-29T18:37:51Z

pkg/es/wrapper/wrapper_nolint.go

@@ -16,9 +16,6 @@ package eswrapper

 import "github.com/jaegertracing/jaeger/pkg/es"

-// Some of the functions of elastic.BulkIndexRequest violate golint rules,
-// e.g. Id() should be ID() and BodyJson() should be BodyJSON().
-


this feels like the relevant comment that explains the need for this file.

please restore the comment

I did it already

it still shows as deleted in the latest version of PR

my bad. Updated now

rbroggi · 2021-10-30T10:24:28Z

tools.go

@@ -19,11 +19,8 @@ package jaeger

 // https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module
 import (
-	_ "honnef.co/go/tools/cmd/staticcheck"


not used in our current configuration of golangci-lint or in any other form

rbroggi · 2021-10-30T10:25:30Z

tools.go

 	_ "github.com/mjibson/esc"
-	_ "github.com/securego/gosec/cmd/gosec"


indirect dependency of golangci-lint will be naturally included in go.mod as indirect dependency.

rbroggi · 2021-10-30T10:26:30Z

Makefile

@@ -409,12 +410,8 @@ generate-zipkin-swagger: init-submodules
 	$(SWAGGER) generate server -f ./idl/swagger/zipkin2-api.yaml -t $(SWAGGER_GEN_DIR) -O PostSpans --exclude-main
 	rm $(SWAGGER_GEN_DIR)/restapi/operations/post_spans_urlbuilder.go $(SWAGGER_GEN_DIR)/restapi/server.go $(SWAGGER_GEN_DIR)/restapi/configure_zipkin.go $(SWAGGER_GEN_DIR)/models/trace.go $(SWAGGER_GEN_DIR)/models/list_of_traces.go $(SWAGGER_GEN_DIR)/models/dependency_link.go

-.PHONY: install-mockery


I took the liberty to merge all the tools installation into a single make target. makes sense?

yurishkuro · 2021-10-30T21:36:15Z

having golangci-lint in our go.mod is bringing a lot of indirect dependencies

I tend to agree, this went into the wrong direction, we don't want to be tracking so many dependencies. I think we should go back to installing golang-ci via explicit version and not list it in the tools.go

Question, however: when we install golang-ci directly, not via go.mod, (a) does it auto-install all other required tools, and (b) does it manage their versions or always gets the latest?

albertteoh · 2021-10-30T22:36:03Z

(a) does it auto-install all other required tools, and

Yes, that's my understanding. To test it, I removed by $GOPATH/pkg/mod dir and $GOPATH/bin/* and ran:

$  make lint                   
golangci-lint -v run
make: golangci-lint: No such file or directory
make: *** [lint] Error 1

$ make install-tools
...

$ make lint
golangci-lint -v run
...
INFO Execution took 10.59982082s                  
./scripts/updateLicenses.sh > .fmt.log
./scripts/import-order-cleanup.sh stdout > .import.log

(b) does it manage their versions or always gets the latest?

My understanding is that it manages their versions:

$ make install-tools
...
go: downloading github.com/polyfloyd/go-errorlint v0.0.0-20210510181950-ab96adb96fea
...

is consistent with golangci-lint v1.42.0's (the tag version we're installing in Makefile's install-tools target) go-errorlint dependency: https://github.com/golangci/golangci-lint/blob/v1.42.0/go.mod#L61.

Switching to golangci-lint's master branch, we see this dependency version has been upgraded:

	github.com/polyfloyd/go-errorlint v0.0.0-20210722154253-910bb7978349

golint has been deprecated and is no longer used in this repository. Eliminating the useless import in the tools.go. Signed-off-by: rbroggi <[email protected]>

* Uniform the tool dependency handling by delegating it to the `go.mod` file * Using `tools.go` to import all the *direct* tools dependencies * Centralizing the installation of dependencies in a single make target * Eliminating explicit dependency from `gosec` as it's a transitive dependency of `golangci-lint` Signed-off-by: rbroggi <[email protected]>

Signed-off-by: rbroggi <[email protected]>

rbroggi requested a review from a team as a code owner October 29, 2021 18:28

rbroggi requested a review from pavolloffay October 29, 2021 18:29

yurishkuro reviewed Oct 29, 2021

View reviewed changes

rbroggi force-pushed the linter branch from 519d5bb to 5d5b674 Compare October 29, 2021 18:42

rbroggi commented Oct 30, 2021

View reviewed changes

rbroggi requested a review from yurishkuro October 30, 2021 18:38

rbroggi added 3 commits October 31, 2021 07:58

Eliminating useless golint import

6135dd0

golint has been deprecated and is no longer used in this repository. Eliminating the useless import in the tools.go. Signed-off-by: rbroggi <[email protected]>

Aligning tool versioning

039cb5c

Signed-off-by: rbroggi <[email protected]>

rbroggi force-pushed the linter branch from f03bd0a to 039cb5c Compare October 31, 2021 07:20

rbroggi requested a review from albertteoh October 31, 2021 07:48

rbroggi added 3 commits October 31, 2021 14:51

gcovmerge no longer needed

6a45a3a

Signed-off-by: rbroggi <[email protected]>

merging mockery v2 PR

080c714

Signed-off-by: rbroggi <[email protected]>

reestablish valid comment

996d1e0

Signed-off-by: rbroggi <[email protected]>

yurishkuro approved these changes Oct 31, 2021

View reviewed changes

yurishkuro changed the title ~~Eliminating useless golint import~~ Remove dependencies on unused tools, install tools explicitly instead of via go.mod Oct 31, 2021

yurishkuro merged commit e6b1c8f into jaegertracing:master Oct 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependencies on unused tools, install tools explicitly instead of via go.mod #3355

Remove dependencies on unused tools, install tools explicitly instead of via go.mod #3355

rbroggi commented Oct 29, 2021

codecov bot commented Oct 29, 2021 •

edited

Loading

yurishkuro Oct 29, 2021

rbroggi Oct 29, 2021

yurishkuro Oct 29, 2021

rbroggi Oct 30, 2021

yurishkuro Oct 30, 2021

albertteoh Oct 30, 2021

rbroggi Oct 31, 2021 •

edited

Loading

yurishkuro Oct 31, 2021

rbroggi Oct 31, 2021

yurishkuro Oct 29, 2021

yurishkuro Oct 30, 2021 •

edited

Loading

rbroggi Oct 31, 2021

yurishkuro Oct 31, 2021

rbroggi Oct 31, 2021

rbroggi Oct 30, 2021

rbroggi Oct 30, 2021

rbroggi Oct 30, 2021

yurishkuro commented Oct 30, 2021

albertteoh commented Oct 30, 2021 •

edited

Loading

		_ "github.com/mjibson/esc"
		_ "github.com/securego/gosec/cmd/gosec"

Remove dependencies on unused tools, install tools explicitly instead of via go.mod #3355

Remove dependencies on unused tools, install tools explicitly instead of via go.mod #3355

Conversation

rbroggi commented Oct 29, 2021

Short description of the changes

codecov bot commented Oct 29, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbroggi Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Oct 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Oct 30, 2021

albertteoh commented Oct 30, 2021 • edited Loading

codecov bot commented Oct 29, 2021 •

edited

Loading

rbroggi Oct 31, 2021 •

edited

Loading

yurishkuro Oct 30, 2021 •

edited

Loading

albertteoh commented Oct 30, 2021 •

edited

Loading