Makefile: add faillint to lint target - Issue 2142 #2143

kadern0 · 2020-02-17T03:59:54Z

Fixes #2142

I added CHANGELOG entry for this change.
(I've included a line at the bottom of the Added section)
Change is not relevant to the end user.

Changes

Added failint package with the paths specified within the issue description to 'make lint'

Verification

Manual execution of the command and output verification.

kadern0 · 2020-02-17T04:05:22Z

This is the output of the command:

make lint
>> verifying packages being imported
/home/kaderno/go/src/github.com/improbable-eng/thanos/pkg/block/fetcher.go:9:12: package "errors" shouldn't be imported
/home/kaderno/go/src/github.com/improbable-eng/thanos/pkg/receive/hashring.go:8:2: package "errors" shouldn't be imported
/home/kaderno/go/src/github.com/improbable-eng/thanos/pkg/cacheutil/memcached_client_test.go:8:2: package "errors" shouldn't be imported
/home/kaderno/go/src/github.com/improbable-eng/thanos/pkg/query/api/v1_test.go:22:2: package "errors" shouldn't be imported
/home/kaderno/go/src/github.com/improbable-eng/thanos/pkg/runutil/example_test.go:8:2: package "errors" shouldn't be imported
/home/kaderno/go/src/github.com/improbable-eng/thanos/pkg/store/cache/memcached_test.go:8:2: package "errors" shouldn't be imported
/home/kaderno/go/src/github.com/improbable-eng/thanos/pkg/store/storepb/custom_test.go:7:2: package "errors" shouldn't be imported

All these packages import "errors", so they should be updated before merging this PR or builds will fail.

bwplotka

Thanks!

Some suggestions.

Makefile

bwplotka · 2020-02-17T08:15:16Z

Makefile

@@ -283,14 +285,18 @@ web: web-pre-process $(HUGO)
 	# TODO(bwplotka): Make it --gc
 	@cd $(WEB_DIR) && HUGO_ENV=production $(HUGO) --config hugo.yaml --minify -v -b $(WEBSITE_BASE_URL)

+.PHONY: failint
+failint:


Please let's add this to lint command. We already have too many commands ;p

I I know you added this as lint dep, but I think it might be more explicit when inlined in lint command.

It makes sense moving it inside of the lint command although then we would have to wait for the whole linting to complete whenever we want to test (not a huge deal).

I think that's ok (:

You can put it in front of it if you think it's fast.

I think I've fixed this, please let me know if you were thinking something different when you said to put it in front.

Makefile

bwplotka · 2020-02-17T08:16:51Z

Makefile

+.PHONY: failint
+failint:
+	@echo ">> verifying packages being imported"
+	@faillint -paths $(IMPORTS_TO_AVOID) ./...


Also let's install failint as we do for other Go binaries.

I've followed the "Dependency management" section under the contributing guide. I've realized that if I run make deps it removes the faillint entries from go.sum and go.mod

And that's ok - check the binaries like $(MINIO_SERVER) and how it install it and version. (: Can we mimic that?

Done (but check it, please)!

go.mod

daixiang0 · 2020-02-18T07:40:20Z

Would it be better to change pr title as "Makefile: add faillint to lint target"?

kadern0 · 2020-02-18T07:46:54Z

Would it be better to change pr title as "Makefile: add faillint to lint target"?

Yeah, I like it better. Should I keep the reference to the issue No.?

bwplotka

Overall good, thanks for this! Some comments.

bwplotka · 2020-02-18T07:58:29Z

CHANGELOG.md

@@ -678,3 +678,4 @@ Initial version to have a stable reference before [gossip protocol removal](/doc
 - Bucket commands.
 - Downsampling support for UI.
 - Grafana dashboards for Thanos components.
+- Failint for imported packages verification.


I think we should not touch 0.1.0 release ;p

Also this is a tooling, not really relevant for Thanos users, so we don't need changelog entry. (:

I was following the guide :) Removed!

About not touching the 0.1.0 version, should I change the PR to a different branch instead of master?

No. Master is fine. Just you put the changelog entry at the bottom, but actually releases goes from newest to oldest.

Anyway, there should be no entry.

bwplotka

Hm, this should work I believe. But CI and DCO fails. You can click on those to learn why.

Thanks!

Curious, have you tested it? e.g put some not allowed imports and run it?

kadern0 · 2020-02-19T07:33:42Z

One of the tests was failing because of incorrect signing off of commits. The other test fails because of what I've mentioned in my second comment on this PR. There are 7 packages importing errors, thus, the linting stage fails. Now that the "setup" is working, it might be appropriate, fixing these files, what do you think?

Also, yes, I've tested the correct behavior of the code (with different modules and also placing them under a directory called vendor) and it seems to be working as expected.

kadern0 · 2020-02-19T07:38:24Z

If I update this line:
MODULES_TO_AVOID ?=errors,prometheus/tsdb,prometheus/prometheus/pkg/testutils
to:
MODULES_TO_AVOID ?=errors=pkg/errors,prometheus/tsdb,prometheus/prometheus/pkg/testutils

Then, in the output we get a suggestion of the correct module to import:
improbable-eng/thanos/pkg/block/fetcher.go:9:12: package "errors" shouldn't be imported, suggested: "pkg/errors"

Should we add the recommended modules for each of the ones to avoid?

bwplotka · 2020-02-19T08:44:40Z

That would be amazing! Nice idea @kadern0!

But please let's use the full path for imports if we can?

Looks like we detected errors!

 /go/src/github.com/thanos-io/thanos/pkg/block/fetcher.go:9:12: package "errors" shouldn't be imported
/go/src/github.com/thanos-io/thanos/pkg/receive/hashring.go:8:2: package "errors" shouldn't be imported
/go/src/github.com/thanos-io/thanos/pkg/cacheutil/memcached_client_test.go:8:2: package "errors" shouldn't be imported
/go/src/github.com/thanos-io/thanos/pkg/query/api/v1_test.go:22:2: package "errors" shouldn't be imported
/go/src/github.com/thanos-io/thanos/pkg/runutil/example_test.go:8:2: package "errors" shouldn't be imported
/go/src/github.com/thanos-io/thanos/pkg/store/cache/memcached_test.go:8:2: package "errors" shouldn't be imported
/go/src/github.com/thanos-io/thanos/pkg/store/storepb/custom_test.go:7:2: package "errors" shouldn't be imported

Do you mind changing those?

Good job, can't wait to merge it! (:

kadern0 · 2020-02-19T12:16:44Z

I've changed all the imports but now circleci is failing for some reason I don't understand. Could you please have a look?

Makefile

Signed-off-by: Pablo Caderno <[email protected]>

Signed-off-by: kaderno <[email protected]> Signed-off-by: Pablo Caderno <[email protected]>

Signed-off-by: Pablo Caderno <[email protected]>

yeya24

Thanks for the great work! @kadern0
CI failed but unrelated.
LGTM

bwplotka

Thanks!

LGTM! Let's merge on green!

go.mod

pkg/block/fetcher.go

* Added faillint Signed-off-by: Pablo Caderno <[email protected]> * Added faillint Signed-off-by: kaderno <[email protected]> Signed-off-by: Pablo Caderno <[email protected]> * Added faillint Signed-off-by: Pablo Caderno <[email protected]>

bwplotka requested changes Feb 17, 2020

View reviewed changes

bwplotka reviewed Feb 18, 2020

View reviewed changes

kadern0 changed the title ~~Issue 2142: Added failint to Makefile - make lint~~ Makefile: add faillint to lint target - Issue 2142 Feb 18, 2020

bwplotka reviewed Feb 18, 2020

View reviewed changes

kadern0 force-pushed the issue-2142 branch from a1e08dc to 14492b3 Compare February 19, 2020 07:27

kadern0 force-pushed the issue-2142 branch 4 times, most recently from 309c4d1 to 81b27b7 Compare February 19, 2020 11:21

kadern0 force-pushed the issue-2142 branch from e330f89 to 0173759 Compare February 19, 2020 20:32

yeya24 reviewed Feb 19, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

kadern0 force-pushed the issue-2142 branch from eb54ebe to 97bdb1a Compare February 20, 2020 00:07

pcaderno added 2 commits February 20, 2020 11:07

Added faillint

9807b8e

Signed-off-by: Pablo Caderno <[email protected]>

Added faillint

a7321f3

Signed-off-by: kaderno <[email protected]> Signed-off-by: Pablo Caderno <[email protected]>

kadern0 force-pushed the issue-2142 branch from 97bdb1a to a7321f3 Compare February 20, 2020 00:08

Added faillint

4091b50

Signed-off-by: Pablo Caderno <[email protected]>

kadern0 force-pushed the issue-2142 branch from 830bb18 to 4091b50 Compare February 20, 2020 00:18

yeya24 approved these changes Feb 20, 2020

View reviewed changes

bwplotka approved these changes Feb 20, 2020

View reviewed changes

go.mod Show resolved Hide resolved

pkg/block/fetcher.go Show resolved Hide resolved

bwplotka merged commit f278950 into thanos-io:master Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile: add faillint to lint target - Issue 2142 #2143

Makefile: add faillint to lint target - Issue 2142 #2143

kadern0 commented Feb 17, 2020

kadern0 commented Feb 17, 2020

bwplotka left a comment

bwplotka Feb 17, 2020

bwplotka Feb 17, 2020

kadern0 Feb 17, 2020 •

edited

Loading

bwplotka Feb 18, 2020

bwplotka Feb 18, 2020

kadern0 Feb 18, 2020

bwplotka Feb 17, 2020

kadern0 Feb 17, 2020

bwplotka Feb 18, 2020

kadern0 Feb 18, 2020

daixiang0 commented Feb 18, 2020

kadern0 commented Feb 18, 2020

bwplotka left a comment

bwplotka Feb 18, 2020

kadern0 Feb 18, 2020

kadern0 Feb 18, 2020

bwplotka Feb 18, 2020

bwplotka left a comment

kadern0 commented Feb 19, 2020

kadern0 commented Feb 19, 2020

bwplotka commented Feb 19, 2020

kadern0 commented Feb 19, 2020

yeya24 left a comment

bwplotka left a comment

Makefile: add faillint to lint target - Issue 2142 #2143

Makefile: add faillint to lint target - Issue 2142 #2143

Conversation

kadern0 commented Feb 17, 2020

Changes

Verification

kadern0 commented Feb 17, 2020

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadern0 Feb 17, 2020 • 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

Choose a reason for hiding this comment

daixiang0 commented Feb 18, 2020

kadern0 commented Feb 18, 2020

bwplotka left a 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

bwplotka left a comment

Choose a reason for hiding this comment

kadern0 commented Feb 19, 2020

kadern0 commented Feb 19, 2020

bwplotka commented Feb 19, 2020

kadern0 commented Feb 19, 2020

yeya24 left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

kadern0 Feb 17, 2020 •

edited

Loading