Golangci lint #731

bege13mot · 2019-10-17T14:57:24Z

rhatdan · 2019-10-21T13:11:37Z

@bege13mot You need sign all of your PRs
Also needs a rebase.

bege13mot · 2019-10-21T15:17:26Z

@bege13mot You need sign all of your PRs
Also needs a rebase.

Thank you, I've signed all commits, but unfortunately, I'm not sure about rebasing. Could you please help a little bit with it?

vrothberg · 2019-10-21T15:33:16Z

You need to rebase on the most recent commit on master. There are many ways to do that. I usually keep the upstream projects as a remote:

$ git remote add upstream [email protected]:containers/image.git
$ git remote update
# now can rebase our branch on top of upstream/master
$ git pull --rebase upstream master

bege13mot · 2019-10-21T15:35:24Z

Thank you

bege13mot · 2019-10-22T12:21:08Z

Unfortunately, still can't pass the test:

cmd/skopeo/layers.go:145:28: not enough arguments in call to dest.PutManifest
	have (context.Context, []byte)
	want (context.Context, []byte, *digest.Digest)
cmd/skopeo/layers.go:149:20: not enough arguments in call to dest.Commit
	have (context.Context)
	want (context.Context, types.UnparsedImage)

vrothberg · 2019-10-23T08:27:36Z

I restarted Travis. We had to update Skopeo to unblock the CI here.

vrothberg · 2019-10-23T08:28:21Z

Commit eae61fa must be dropped from this PR.

vrothberg

Please use more descriptive commit messages. "Fix ABC" is not enough to describe what's going on. "Fix" is a very generic (and overused) term. "Check all errors in ABC" would be more descriptive. Good commit messages are especially important for maintenance.

A more detailed review follows in a bit.

vrothberg

Thanks for opening the PR, @bege13mot!

As mentioned before, the commit messages must be more descriptive and also explain/justify why certain changes are okay (e.g., dead-code removal). I dropped comments where I would like some further changes (e.g., log errors instead of ignoring). Also, I found a couple of changes that seem unrelated to golangci-lint. Please make sure that those are not part of this PR. We can address them in a separate PR.

copy/copy.go

copy/copy_test.go

directory/directory_test.go

signature/policy_reference_match_test.go

signature/signature.go

storage/storage_reference.go

storage/storage_test.go

bege13mot · 2019-10-23T11:36:20Z

Hi @vrothberg ,
Thank you very much for your review.

openshift/openshift-copies.go

TomSweeneyRedHat · 2019-10-23T12:41:33Z

CI tests are not at all happy.

bege13mot · 2019-10-23T14:33:28Z

CI tests are not at all happy.

Hi, it looks like an issue with containers/skopeo

/tmp/tmp.hP4WQEvbic/src/github.com/containers/skopeo/hack/make/validate-lint: line 14: golint: command not found
2055Makefile:175: recipe for target 'validate-local' failed

copy/copy.go

docker/archive/transport_test.go

docker/daemon/daemon_dest.go

docker/tarfile/src.go

docker/tarfile/dest.go

storage/storage_test.go

types/types.go

Makefile

mtrmac · 2019-10-23T19:13:19Z

openshift/openshift-copies.go

-			defer clientCertFile.Close()
+			defer func() {
+				err = clientCertFile.Close()
+				logrus.Debugf("error closing client cert CA: %v", err)


The two copy&pasted errors here and below are incorrect.

Sorry, I'm not clear about these copy&pasted errors

The text of the 3 Debugf messages is the same (“closing client CA”), but only one of them actually closes a client CA; the other two should be updated to match what actually failed (client certificate/key file).

mtrmac · 2019-10-23T19:21:20Z

Hi, it looks like an issue with containers/skopeo

/tmp/tmp.hP4WQEvbic/src/github.com/containers/skopeo/hack/make/validate-lint: line 14: golint: command not found
Makefile:175: recipe for target 'validate-local' failed

At a first glance, this looks like an issue in how this PR changes the environment for make test-skopeo to me.

vrothberg · 2019-11-25T14:49:04Z

@bege13mot, can you rebase? We had some CI flakes that are fixed now.

One thing I just noticed now is that all commits are not signed with your real name but with the GitHub user which is in conflict with the contributing policies (https://github.com/containers/image/blob/master/CONTRIBUTING.md). My apologies for noticing that late.

mtrmac · 2019-11-26T03:38:54Z

This is now very big, and likely enough to break if any other PR is merged. Maybe it would be more practical to split it into smaller PRs? (Definitely no need to split it artificially just because I said so now — but, if the rebase happens one commit at a time, maybe separate 10 unrelated commits into a separate PR.)

Ideally, of course, a rebased version would pass tests, be quickly reviewed, and immediately merged; splitting this is distinctly second best. Still, a possible option.

vrothberg · 2019-11-26T08:21:51Z

I think something went wrong while rebasing since there are many older commits unrelated to this PR.

vrothberg · 2019-11-26T12:18:35Z

Failures look legit:

package github.com/golangci/golangci-lint: no Go files in /gopath/src/github.com/golangci/golangci-lint

Makefile:58: recipe for target 'tools.timestamp' failed

Signed-off-by: Ivan Voronchihin <[email protected]>

rhatdan · 2019-11-27T10:50:16Z

@vrothberg @mtrmac Tests pass, is this ready to merge?
@bege13mot thanks for all of the work on this?

vrothberg · 2019-11-27T10:54:00Z

@vrothberg @mtrmac Tests pass, is this ready to merge?
@bege13mot thanks for all of the work on this?

I'll do one final review now 👍

vrothberg

Just very minor nits left, and we're ready to merge. Thanks for the nice contribution and the long breath, @bege13mot !

vrothberg · 2019-11-27T10:54:39Z

Makefile


 tools.timestamp: Makefile
 	@GO111MODULE="off" go get -u $(BUILDFLAGS) golang.org/x/lint/golint
+	curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sudo sh -s -- -b $(go env GOPATH)/bin v1.21.0


Please drop sudo. It's not needed. We should also first check if golangci-lint isn't already installed and only install if needed. An example how to do that is in libpod: https://github.com/containers/libpod/blob/master/Makefile#L483

Unfortunately, it isn't help.
`install: cannot create regular file '//golangci-lint': Permission denied``

⚠️

“help” or not, this project absolutely must not create files in the root directory of the user’s filesystem!

If using sudo here really has that effect, it must be removed, pretty much at any cost, up to and including not running golang-ci, if there were really no alternative.

I'll fix that. $(go env GOPATH) didn't evaluate here for some odd reason.

vrothberg · 2019-11-27T10:57:19Z

pkg/sysregistriesv2/system_registries_v2.go

 	// deterministic behavior which is not guaranteed by maps.
 	for _, reg := range config.Registries {
-		others, _ := regMap[reg.Location]
+		others := regMap[reg.Location]


I agree with @mtrmac. We should address it.

vrothberg · 2019-11-27T10:59:23Z

storage/storage_image.go

 	layers, err := s.imageRef.transport.store.LayersByUncompressedDigest(info.Digest)
+	if err != nil {
+		return nil, -1, "", err
+	}


Let's ignore the error here. It's really okay if there's an error here as we might need to lookup the compresssed digest. The log here could cause confusing noise.

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

vrothberg

LGTM, thanks!

@rhatdan PTAL. Once this is merged, I'll tackle the install targets to make them consistent with Buildah, Podman etc.

rhatdan · 2019-11-27T14:25:16Z

LGTM

sockets.DialerFromEnvironment seems to implement an all_proxy environment variable. Either way, since containers#731 the return value was ignored, making that code completely ineffective, and noone has complained for three years. So let's just drop it. Signed-off-by: Miloslav Trmač <[email protected]>

containers#731 introduced a WaitGroup which is waited but never signaled, and a nonsensical error reporting mechanism (where errors are pushed but don't terminate future operations). Instead, rely on PipeWriter.CloseWithError() to propagate failures from the goroutine. And move all of the body of the goroutine into a separate function, so that we can use defer() to ensure CloseWithError() is _always_ called. Signed-off-by: Miloslav Trmač <[email protected]>

vrothberg reviewed Oct 23, 2019

View reviewed changes

vrothberg requested changes Oct 23, 2019

View reviewed changes

TomSweeneyRedHat reviewed Oct 23, 2019

View reviewed changes

openshift/openshift-copies.go Outdated Show resolved Hide resolved

mtrmac reviewed Oct 23, 2019

View reviewed changes

bege13mot added 11 commits November 26, 2019 17:57

tr.Dial is deprecated: Use DialContext instead

e64803b

Signed-off-by: Ivan Voronchihin <[email protected]>

Fix SA2002: the goroutine calls T.Fatalf

2f81372

Signed-off-by: Ivan Voronchihin <[email protected]>

_imgs, _ref, _transport, _imgd is unused

f50c0e4

Signed-off-by: Ivan Voronchihin <[email protected]>

Fix S1020: when ok is true, err can't be nil

cb1d752

Signed-off-by: Ivan Voronchihin <[email protected]>

Delete unused

e099400

Signed-off-by: Ivan Voronchihin <[email protected]>

Add helper for error checking

26bf709

Signed-off-by: Ivan Voronchihin <[email protected]>

Handle checking error in dirImageMockWithRef

620d71d

Signed-off-by: Ivan Voronchihin <[email protected]>

Fix SA1019: os.SEEK_SET is deprecated

42e5bd8

Signed-off-by: Ivan Voronchihin <[email protected]>

Fix golangci-lint advises

21320ca

Signed-off-by: Ivan Voronchihin <[email protected]>

Fix this value is never used

4d60795

Signed-off-by: Ivan Voronchihin <[email protected]>

Fix Error return value is not checked

33ec055

Signed-off-by: Ivan Voronchihin <[email protected]>

vrothberg reviewed Nov 27, 2019

View reviewed changes

bege13mot added 4 commits November 27, 2019 15:04

Change golint to golangci-lint

34750ac

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

Remove error handling

6463b77

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

Add internal error handling

94bbdce

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

Return sudo

83bef13

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

vrothberg reviewed Nov 27, 2019

View reviewed changes

rhatdan merged commit d13a90f into containers:master Nov 27, 2019

vrothberg mentioned this pull request Dec 2, 2019

migrate to golangci-lint #730

Closed

Golangci lint #731

Golangci lint #731

Uh oh!

Conversation

bege13mot commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented Oct 21, 2019

Uh oh!

bege13mot commented Oct 21, 2019

Uh oh!

vrothberg commented Oct 21, 2019

Uh oh!

bege13mot commented Oct 21, 2019

Uh oh!

bege13mot commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrothberg commented Oct 23, 2019

Uh oh!

vrothberg commented Oct 23, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bege13mot commented Oct 23, 2019

Uh oh!

Uh oh!

TomSweeneyRedHat commented Oct 23, 2019

Uh oh!

bege13mot commented Oct 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Oct 23, 2019

Uh oh!

vrothberg commented Nov 25, 2019

Uh oh!

mtrmac commented Nov 26, 2019

Uh oh!

vrothberg commented Nov 26, 2019

Uh oh!

vrothberg commented Nov 26, 2019

Uh oh!

rhatdan commented Nov 27, 2019

Uh oh!

vrothberg commented Nov 27, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bege13mot commented Oct 17, 2019 •

edited

Loading

bege13mot commented Oct 22, 2019 •

edited

Loading