This change is 

tftr!

bors r=ajwerner

Build succeeded:

• GitHub CI (Cockroach)

@rafiss just a headsup that this seems to have caused #72371 failures. The reason is that https://github.com/cockroachdb/cockroach/pull/72351/files#diff-6b2c8ccac0d3ee5f528d6661fcc9b37a032f7ac928cbfaebc72a05db34e47b4dR179 will now return nil if the err it is wrapping is nil. We do in fact want to return an error if we find a record, even if getRecord itself didn't return an error, because we are waiting for the record to get reconciled.

I'll send out a fix for this, but might be something to keep in mind when running the new linter! I (along with @miretskiy and @stevendanna) also feel that wrapping a nil error should be disallowed. While it saves a few lines of code it doesn't seem like a very desirable side effect? This might be a discussion for another issue.

thanks for explaining @adityamaru !

I can add a thing to my linter to try to detect nil wrapped errors.

to be fully robust to the problem, a nil-check could be added to the Wrap function itself. maybe @knz has thoughts on that. (it would be backwards incompatible, but perhaps a good idea)

a nil-check could be added to the Wrap function itself.

huh, no - it's a feature:

result, err := somefunc()
return result, errors.Wrap(err, ...)

I can add a thing to my linter to try to detect nil wrapped errors.

Nice. The nillness linter catches the "definitely nil" cases, but doesn't catch the "maybe nil" cases.

huh, no - it's a feature:

I think that the argument is that while it is a feature, the cost isn't worth the benefit. That said, changing it would put us out-of-line with other similar functions in the Go ecosystem.

That said, changing it would put us out-of-line with other similar functions in the Go ecosystem.

:that: