tests LGTM but you forgot a few cases.

Reviewed 16 of 16 files at r1, 13 of 13 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @rafiss)

pkg/server/drain_test.go, line 90 at r2 (raw file):

			return nil
		}
		return errors.Newf("server not yet refusing RPC, got %v", err /* nolint:errwrap */)

Can you explain in a comment why the linter exception is needed here?

pkg/testutils/lint/passes/errwrap/errwrap.go, line 104 at r3 (raw file):

		// Check that none of the arguments are err.Error()
		if (pkg.Name() == "fmt" && fn.Name() == "Errorf") ||

Can you implement this via a map? This would be more efficient.

Also is there a way to share this data with the map in ../fmtsafe/functions.go?
(This also reveals you're missing a few here - unimplemented, roachpb, sqlerrors etc)

small request, can you please make the appropriate change for bazel too?

diff --git a/BUILD.bazel b/BUILD.bazel
index f1b5c20697..4e322ae951 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -238,6 +238,7 @@ nogo(
         "//pkg/testutils/lint/passes/descriptormarshal",
         "//pkg/testutils/lint/passes/errcheck",
         "//pkg/testutils/lint/passes/errcmp",
+        "//pkg/testutils/lint/passes/errwrap",
         "//pkg/testutils/lint/passes/fmtsafe",
         "//pkg/testutils/lint/passes/grpcclientconnclose",
         "//pkg/testutils/lint/passes/grpcstatuswithdetails",

updated bazel build config

i'm giving this another run now that more error functions are being linted

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @otan)

pkg/server/drain_test.go, line 90 at r2 (raw file):

yes added; it would be incorrect to wrap the error here since this function should return an error if err is nil, and wrapping a nil error results in another nil error.

pkg/testutils/lint/passes/errwrap/errwrap.go, line 104 at r3 (raw file):

i made the maps backed by the same thing

mod the one testing nit.

Reviewed 1 of 12 files at r4, 10 of 17 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz, @otan, @rafiss, and @rickystewart)

pkg/testutils/lint/passes/errwrap/testdata/src/a/a_test.go, line 63 at r5 (raw file):

	_ = errors.NewAssertionErrorWithWrappedErrf(anotherErr, "format %v", wrappedErr) // want `non-wrapped error is passed to errors.NewAssertionErrorWithWrappedErrf; use pgerror.Wrap/errors.Wrap/errors.CombineErrors/errors.WithSecondaryError/errors.NewAssertionErrorWithWrappedErrf instead`

	_ = fmt.Errorf("got %s", wrappedErr /* nolint:errwrap */)  // linting is skipped.

can you test the // style comment too? It should work.

Nice, a few nits and this is good to go.

Reviewed 8 of 38 files at r6, 40 of 40 files at r7, 9 of 9 files at r8, 17 of 17 files at r9, 17 of 17 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @ajwerner, @otan, @rafiss, and @rickystewart)

pkg/ccl/importccl/read_import_mysql.go, line 767 at r10 (raw file):

					// error.
					// nolint:errwrap
					return nil, errors.Wrapf(

💯

pkg/cli/clierrorplus/decorate_error.go, line 90 at r10 (raw file):

		connInsecureHint := func() error {
			const format = "cannot establish secure connection to insecure server.\n" +

hmm 🤔
is this a legitimate change? This suggests either it would benefit from errors.Wrap, or from a linter exception.

ditto below

pkg/cloud/amazon/s3_storage.go, line 437 at r10 (raw file):

			case s3.ErrCodeNoSuchBucket, s3.ErrCodeNoSuchKey:
				// nolint:errwrap
				return nil, errors.WithMessage(

maybe errors.Wrapf(errors.Wrap(...), "%v", err) to ensure err gets embarked as a secondary error.

pkg/cloud/azure/azure_storage.go, line 160 at r10 (raw file):

			case azblob.ServiceCodeBlobNotFound, azblob.ServiceCodeResourceNotFound:
				// nolint:errwrap
				return nil, 0, errors.WithMessage(

ditto previous, errors.Wrapf

pkg/cloud/gcp/gcs_storage.go, line 200 at r10 (raw file):

			// return our internal ErrFileDoesNotExist.
			// nolint:errwrap
			err = errors.WithMessage(

ditto previous, errors.Wrapf

pkg/cloud/httpsink/http_storage.go, line 270 at r10 (raw file):

		if err != nil && resp.StatusCode == 404 {
			// nolint:errwrap
			err = errors.WithMessage(

ditto previous, errors.Wrapf

pkg/cloud/nodelocal/nodelocal_storage.go, line 146 at r10 (raw file):

		// The local store returns a golang native ErrNotFound, whereas the remote
		// store returns a gRPC native NotFound error.
		// nolint:errwrap

I don't think this linter exception is on the right source code line

pkg/cloud/nodelocal/nodelocal_storage.go, line 148 at r10 (raw file):

		// nolint:errwrap
		if oserror.IsNotExist(err) || status.Code(err) == codes.NotFound {
			return nil, 0, errors.WithMessage(

ditto previous, errors.Wrapf

pkg/cmd/roachtest/tests/encryption.go, line 56 at r10 (raw file):

			// Generate encryption store key through `./cockroach gen encryption-key -s=size aes-size.key`.
			if err := c.RunE(ctx, c.Node(nodes), fmt.Sprintf("./cockroach gen encryption-key -s=%[1]d aes-%[1]d.key", size)); err != nil {
				return errors.Wrapf(err, "failed to generate aes key with size %d through CLI", size)

nit: aes -> AES

pkg/kv/kvserver/client_lease_test.go, line 669 at r10 (raw file):

				wait(timetoDie.Nanoseconds())
			}
			// nolint:errwrap

nit: you can add the same comment // NB: errors.Wrapf(nil, ...) returns nil. that you added elsewhere.

pkg/kv/kvserver/replica_rangefeed_test.go, line 675 at r10 (raw file):

			}
			err = repl.AdminTransferLease(ctx, roachpb.StoreID(1))
			return errors.Wrapf(err, "not raft follower: %+v, transferred lease", raftStatus)

beware, err can be nil here. I think you want the other thing instead.

pkg/security/securitytest/securitytest.go, line 80 at r10 (raw file):

			if _, dirErr := AssetDir(joined); dirErr != nil {
				errString := dirErr.Error()
				return nil, errors.Wrapf(err, "missing directory (%s)", errString)

you could take the dirErr as secondary error here:

return nil, errors.Wrapf(err, "missing directory (%v)", dirErr /* nolint */)

pkg/util/httputil/http.go, line 116 at r10 (raw file):

	if contentType := resp.Header.Get(ContentTypeHeader); !(resp.StatusCode == http.StatusOK && contentType == JSONContentType) {
		b, err := ioutil.ReadAll(resp.Body)
		// nolint:errwrap

nit: you can add the comment that explains what happens with nil errors here too

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @ajwerner, @knz, @otan, @rafiss, and @rickystewart)

pkg/cli/clierrorplus/decorate_error.go, line 90 at r10 (raw file):

i made this change in order to match more closely to how the other error decorating functions worked in this file. (e.g. see connSecurityHint above.) i didn't use errors.Wrap here since it seemed like these messages were carefully formatted by hand, and Wrap makes it less obvious what the resulting string will look like. however, if your preference is for Wrap also, then i'd definitely agree with that change

pkg/cloud/amazon/s3_storage.go, line 437 at r10 (raw file):

done

pkg/cloud/azure/azure_storage.go, line 160 at r10 (raw file):

done

pkg/cloud/gcp/gcs_storage.go, line 200 at r10 (raw file):

done

pkg/cloud/httpsink/http_storage.go, line 270 at r10 (raw file):

done

pkg/cloud/nodelocal/nodelocal_storage.go, line 146 at r10 (raw file):

fixed

pkg/cloud/nodelocal/nodelocal_storage.go, line 148 at r10 (raw file):

done

pkg/cmd/roachtest/tests/encryption.go, line 56 at r10 (raw file):

fixed

pkg/kv/kvserver/client_lease_test.go, line 669 at r10 (raw file):

done

pkg/kv/kvserver/replica_rangefeed_test.go, line 675 at r10 (raw file):

fixed; very nice catch

pkg/security/securitytest/securitytest.go, line 80 at r10 (raw file):

done

pkg/testutils/lint/passes/errwrap/testdata/src/a/a_test.go, line 63 at r5 (raw file):

done

pkg/util/httputil/http.go, line 116 at r10 (raw file):

done

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @ajwerner, @knz, @otan, @rafiss, and @rickystewart)

pkg/cli/clierrorplus/decorate_error.go, line 90 at r10 (raw file):

I don't have a strong opinion. If you keep your choice, you can explain it in a comment.