Consume returned values implicitly, remove some nolint comments #410

tranngoclam · 2021-11-16T15:08:35Z

No description provided.

codefromthecrypt

I made a comment about adding _, _ = moreos.Fprintf everywhere. please revert that.. I think you misunderstood me. I meant to change the signature of moreos.Fprintf to not return any values since we always ignore them anyway. After you revert this, I'll take another look!

codefromthecrypt · 2021-11-16T23:04:01Z

e2e/admin_test.go

 	if err != nil {
 		return nil, err
 	}
 	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
 		return nil, err
 	}
-	defer resp.Body.Close() //nolint:errcheck
+	defer resp.Body.Close()


ack, this neither trips golint nor goland!

codefromthecrypt · 2021-11-16T23:09:04Z

e2e/main_test.go

@@ -94,13 +94,13 @@ func mockEnvoyVersionsServer() (*httptest.Server, error) {
 			h := r.Header.Get(k)
 			if h != v {
 				w.WriteHeader(500)
-				w.Write([]byte(moreos.Sprintf("invalid %q: %s != %s\n", k, h, v))) //nolint
+				_, _ = w.Write([]byte(moreos.Sprintf("invalid %q: %s != %s\n", k, h, v)))


please revert ones like _, _ = w as they are worse than the trailing //nolint comment as they clutter the file. The real problem is the IDE not honoring //nolint.

Adding pressure to https://youtrack.jetbrains.com/issue/GO-6502 is the better way than adding debt everywhere IMHO. Please upvote that if you don't mind! Sometimes issues go stale and have resolved in JetBrains. That this isn't a "standard" is invalid because de-facto standards end up in IDE config often enough.

PS I know not everyone shares this opinion, but I do think if we weren't slaves to IDE defects, we'd agree this is worse to read. cc @llinder and @johnlanda who I think decided to be slaves in another project 😎 I just really don't want to set a precedent here where I end up intentionally making code harder to read over this.

Since https://youtrack.jetbrains.com/issue/GO-6502 is locked I opened the next best xxpxxxxp/intellij-plugin-golangci-lint#92, as at least there it is possible to upvote!

codefromthecrypt

some notes and I think some of the //nolint will have to come back later... maybe after rebase.

I think there's a saying "when in a hole, stop digging" so once this is contained to only the thing discussed in the other PR (changing the sig of moreos.Fprintf), let's stop so we can conserve our time for more deserving things

codefromthecrypt · 2021-11-17T07:44:49Z

internal/envoy/install.go

@@ -97,7 +98,7 @@ func untarEnvoy(ctx context.Context, dst string, src version.TarballURL, // dst,
 	if err != nil {
 		return err
 	}
-	defer res.Body.Close() //nolint
+	defer func() { _ = res.Body.Close() }()


this is one of those which is worse "fixed" imho :D

codefromthecrypt · 2021-11-17T07:45:21Z

internal/envoy/run.go

@@ -101,7 +101,7 @@ func awaitAdminAddress(sigCtx context.Context, r *Runtime) {
 	for i := 0; i < 10 && sigCtx.Err() == nil; i++ {
 		adminAddress, adminErr := r.GetAdminAddress()
 		if adminErr == nil {
-			moreos.Fprintf(r.Out, "discovered admin address: %s\n", adminAddress) //nolint


odd this passes lint (via make check).. maybe it won't after rebase 😎

codefromthecrypt · 2021-11-17T07:46:09Z

internal/envoy/shutdown/node.go

-			return err
-		}
+		moreos.Fprintf(w, "%v\t%v\t%v\t%v\t%v\t%v\t%v\t%.2f\t%.2f\t%v\n",
+			p.pid, p.username, status, p.rss, p.vms, p.minflt, p.majflt, p.pCPU, p.pMem, p.cmd)


cool this is easier to read and chance of error writing stdout isn't high

codefromthecrypt · 2021-11-17T07:46:51Z

internal/moreos/moreos.go

 	if runtime.GOOS != OSWindows {
-		return fmt.Fprintf(w, format, a...)
+		_, _ = fmt.Fprintf(w, format, a...)


fair enough to do this here vs //nolint as it is contained and highly reused.

codefromthecrypt · 2021-11-17T07:47:41Z

internal/moreos/moreos_test.go


 	expected := "foo\n\nbar\n"
 	if runtime.GOOS == OSWindows {
 		expected = "foo\r\n\r\nbar\r\n"
 	}

 	require.Equal(t, expected, stdout.String())
-	require.Equal(t, len(expected), count)


yep, this is no longer possible to check, but wasn't used either!

codefromthecrypt · 2021-11-17T07:48:11Z

internal/tar/tar.go

@@ -151,11 +152,11 @@ func TarGz(dst, src string) error { //nolint dst, src order like io.Copy
 	if err != nil {
 		return err
 	}
-	defer file.Close() //nolint
+	defer func() { _ = file.Close() }()


please revert all these

codefromthecrypt · 2021-11-17T07:48:25Z

internal/test/fakebinary/testdata/fake_envoy.go

@@ -28,7 +28,7 @@ func main() {
 	signal.Notify(c, os.Interrupt, syscall.SIGTERM)

 	// Echo the same first line Envoy would. This also lets code scraping output know signals are trapped
-	os.Stderr.Write([]byte("initializing epoch 0" + lf)) //nolint
+	_, _ = os.Stderr.Write([]byte("initializing epoch 0" + lf))


some more reverts left like this

codefromthecrypt · 2021-11-17T07:49:46Z

internal/test/server.go

 	require.NoError(t, err)
-	defer f.Close() // nolint
+	t.Cleanup(func() { require.NoError(t, f.Close()) })


this one is worse than before. RequireFakeEnvoyTarGz isn't a test method, it is a utility, so cleanup on exit is valid and also less crufty.

codefromthecrypt · 2021-11-17T07:51:21Z

main_test.go

@@ -29,7 +29,7 @@ func TestRunErrors(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(200)
 	}))
-	defer server.Close()
+	t.Cleanup(func() { server.Close() })


let's not cruft-sandwich things. everywhere I'll ask to not func() wrap functions

Suggested change

t.Cleanup(func() { server.Close() })

t.Cleanup(server.Close)

Signed-off-by: Lam Tran <[email protected]>

tranngoclam · 2021-11-17T08:38:06Z

some notes and I think some of the //nolint will have to come back later... maybe after rebase.

I think there's a saying "when in a hole, stop digging" so once this is contained to only the thing discussed in the other PR (changing the sig of moreos.Fprintf), let's stop so we can conserve our time for more deserving things

Got it, thanks for your time and also the recommendation, let's decide this PR's status 😎 .

codefromthecrypt · 2021-11-17T09:24:05Z

internal/tar/tar.go

@@ -82,7 +83,7 @@ func Untar(dst string, src io.Reader) error { // dst, src order like io.Copy
 	tr := tar.NewReader(zSrc)
 	for {
 		header, err := tr.Next()
-		if err == io.EOF {
+		if errors.Is(err, io.EOF) {


is something wrapping the io.EOF now or are you just expecting it to?

Not really, my point is keeping the consistency when checking error at the present and also future.
IMHO, we should use errors.Is for all the cases, what do you think ?

Thanks for letting me know your input, but I don't think we should call a more expensive function when we don't need it. errors.Is handles unwrapping, and there are two places where you changed to use that here where this isn't needed.

First case you changed was in a unit test on code we wrote. We are testing an expected error. To reduce precision in a test is an anti-goal.. ex I would want to know if our code started wrapping errors. Making the assertion more flimsy allows that test to pass even if someone accidentally wrapped the error. Since we control the code we should know what we are doing and be able to tell if we made a mistake.

The second place you changed the expression was unnecessary for a different reason. The reader api documents the result of EOF as a known behavior. We don't need to expect expect tar to suddenly violate that contract and expect if it did, it would be doing that via wrapping.

Even if func-e isn't particularly concerned about code size, memory size or perf, it is important to know what you are doing. In other libraries, we'd want to avoid bloat and performance loss for no reason. For example, you can see that using errors.Is generates larger code than using the simpler and valid form https://godbolt.org/z/fzabhba45

There are cases of style consistency where there could be subtle differences and choosing one over another is sensible. We totally do that. However, this is a behavior change and the choice adds surprise.. For example, I would suggest the otherwise should be true. If you use errors.Is, there should be a good reason to do that. Trigger the exception the opposite way, use == unless something is special. This may not be the same optimization choice for layered apps or apps that use a lot of unknown deps, but that's the rationale here.

Hope this makes sense. One point that remains is you have added more scope to this PR after a request to reduce scope of it. This causes a day round trip and time to go over pros and cons. Changing the code policy of a project is something better to do when actively developing it vs a "drive by". For example, through fixing other issues, things that are priority to fix, the same time is spent and more value brought through discussions and whatnot. I would suggest to be somewhat empathetic to the project teams when doing style changes and not doing more things on each rev, as we end up in discussions like this which consume both of our time even if there is benefit personally, it starves other change.

@codefromthecrypt
I totally appreciate these points, also reverted the changes.

codefromthecrypt · 2021-11-17T09:24:36Z

internal/moreos/moreos_func-e_test.go

@@ -176,7 +177,7 @@ func requireFindProcessError(t *testing.T, proc *os.Process, expectedErr error)
 	// Wait until the operating system removes or adds the scheduled process.
 	require.Eventually(t, func() bool {
 		_, err := process.NewProcess(int32(proc.Pid)) // because os.FindProcess is no-op in Linux!
-		return err == expectedErr
+		return errors.Is(err, expectedErr)


similar... is something wrapping this error now?

codefromthecrypt · 2021-11-18T00:29:25Z

opened a separate issue about the func-wrapping as there are some surprising results there #414

Signed-off-by: Lam Tran <[email protected]>

codefromthecrypt · 2021-11-18T02:22:53Z

internal/envoy/install.go

@@ -32,6 +32,7 @@ import (
 var binEnvoy = filepath.Join("bin", "envoy"+moreos.Exe)

 // InstallIfNeeded downloads an Envoy binary corresponding to globals.GlobalOpts and returns a path to it or an error.
+//nolint


we don't want function-scoped //nolint in main code even if it could be ok in tests. Probably better to not use this scope in general as it subverts the linter for all code enclosed.

for example, golint adds linters on almost all releases, so in the future this may prevent us from knowing a problem there's no linter now for.

codefromthecrypt · 2021-11-18T02:25:04Z

internal/tar/tar.go

@@ -142,7 +142,8 @@ func extractFile(dst string, src io.Reader, perm os.FileMode) error {
 // Ex If "src" includes "/tmp/envoy/bin" and "/tmp/build/bin". If "src" is "/tmp/envoy", "dst" includes "envoy/bin".
 //
 // This is used to compress the working directory of Envoy after it is stopped.
-func TarGz(dst, src string) error { //nolint dst, src order like io.Copy
+//nolint:revive


codefromthecrypt · 2021-11-18T02:25:56Z

internal/test/server.go

@@ -110,6 +110,7 @@ func (s *server) funcEVersions() []byte {
 }

 // RequireFakeEnvoyTarGz makes a fake envoy.tar.gz
+//nolint:gosec


this is test-only, so ok

Signed-off-by: Lam Tran <[email protected]>

tranngoclam · 2021-11-18T03:50:08Z

@codefromthecrypt looks like a network issue when running the latest commit on windows machine, could you please help to check it ?

codefromthecrypt · 2021-11-18T04:51:02Z

it appears that the directory t.TempDir() deletion may be attempted prior to t.Cleanup(xxx) which would lead to a fail on windows vs earlier when the tests used defer to invoke stop the server (which would happen before t.TempDir() deletion trigger.

codefromthecrypt · 2021-11-18T05:00:15Z

actually the order is correct (t.TempDir() pushes a delete command onto t.Cleanup and later the http server Close is added, meaning it should invoke prior.

Probably, there is some small race that we always won using defer before, but is losing now, or at least lost this last test run. As test flakiness can lose hours of productivity due to amplification across PRs and people, we should fix this next, either by reverting the former commit or adding logic to wait for the filesystem to sync, or something else.

Since this change is unrelated to those things, if clicking rebuild passes, we can merge it.

codefromthecrypt · 2021-11-18T05:06:21Z

Thanks for the help @tranngoclam let's get this in!

codefromthecrypt · 2021-11-18T05:21:02Z

I'm unconvinced server.Close has anything to do with the flake now.. I looked at the code and it doesn't reference the directory created by t.TempDir(). Let's see if it happens again and debug as necessary.

codefromthecrypt reviewed Nov 16, 2021

View reviewed changes

codefromthecrypt reviewed Nov 17, 2021

View reviewed changes

tranngoclam added 4 commits November 17, 2021 15:13

Consume returned values implicitly, remove some nolint comments

4745782

Signed-off-by: Lam Tran <[email protected]>

Revert golangci_lint version

f707deb

Signed-off-by: Lam Tran <[email protected]>

Revert moreos.Fprintf

09cefde

Signed-off-by: Lam Tran <[email protected]>

Add nolint comment to ignore

290befe

Signed-off-by: Lam Tran <[email protected]>

tranngoclam force-pushed the lint branch from ece1df4 to 290befe Compare November 17, 2021 08:30

Revert the version of golangci-lint

a834b85

Signed-off-by: Lam Tran <[email protected]>

codefromthecrypt reviewed Nov 17, 2021

View reviewed changes

Revert err checking

7ba6a23

Signed-off-by: Lam Tran <[email protected]>

codefromthecrypt reviewed Nov 18, 2021

View reviewed changes

Add //nolint

a022c00

Signed-off-by: Lam Tran <[email protected]>

codefromthecrypt merged commit 1ec8c92 into tetratelabs:master Nov 18, 2021

tranngoclam deleted the lint branch November 18, 2021 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consume returned values implicitly, remove some nolint comments #410

Consume returned values implicitly, remove some nolint comments #410

tranngoclam commented Nov 16, 2021 •

edited

Loading

codefromthecrypt left a comment

codefromthecrypt Nov 16, 2021

codefromthecrypt Nov 16, 2021

codefromthecrypt Nov 16, 2021

codefromthecrypt left a comment

codefromthecrypt Nov 17, 2021

codefromthecrypt Nov 17, 2021

codefromthecrypt Nov 17, 2021

codefromthecrypt Nov 17, 2021

codefromthecrypt Nov 17, 2021

codefromthecrypt Nov 17, 2021

codefromthecrypt Nov 17, 2021

codefromthecrypt Nov 17, 2021

codefromthecrypt Nov 17, 2021

tranngoclam commented Nov 17, 2021

codefromthecrypt Nov 17, 2021

tranngoclam Nov 17, 2021

codefromthecrypt Nov 18, 2021

tranngoclam Nov 18, 2021

codefromthecrypt Nov 17, 2021

codefromthecrypt commented Nov 18, 2021

codefromthecrypt Nov 18, 2021

codefromthecrypt Nov 18, 2021

codefromthecrypt Nov 18, 2021

codefromthecrypt Nov 18, 2021

tranngoclam commented Nov 18, 2021

codefromthecrypt commented Nov 18, 2021

codefromthecrypt commented Nov 18, 2021

codefromthecrypt commented Nov 18, 2021

codefromthecrypt commented Nov 18, 2021

Consume returned values implicitly, remove some nolint comments #410

Consume returned values implicitly, remove some nolint comments #410

Conversation

tranngoclam commented Nov 16, 2021 • edited Loading

codefromthecrypt 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

codefromthecrypt 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

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

tranngoclam commented Nov 17, 2021

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

codefromthecrypt commented Nov 18, 2021

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

tranngoclam commented Nov 18, 2021

codefromthecrypt commented Nov 18, 2021

codefromthecrypt commented Nov 18, 2021

codefromthecrypt commented Nov 18, 2021

codefromthecrypt commented Nov 18, 2021

tranngoclam commented Nov 16, 2021 •

edited

Loading