Use `errors.Is` for error equality checks

ianlancetaylor commented 1 week ago

Bug report criteria

[X] This bug report is not security related, security issues should be disclosed privately via etcd maintainers.
[X] This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
[X] You have read the etcd bug reporting guidelines.
[X] Existing open issues along with etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

When using the current Go HEAD which will be the future Go 1.24 release, in the directory etcd-io/etcd/server, running go test ./embed fails. The test error messages are:

--- FAIL: TestLogRotation (0.00s)
    --- FAIL: TestLogRotation/invalid_logger_config (0.00s)
        config_test.go:736: test "invalid logger config", expected error: invalid log rotation config: json: cannot unmarshal bool into Go struct field logRotationConfig.maxsize of type int, got: invalid log rotation config: json: cannot unmarshal bool into Go struct field logRotationConfig.Logger.maxsize of type int

What did you expect to happen?

I expected the tests to pass.

How can we reproduce it (as minimally and precisely as possible)?

Build Go from HEAD as outlined at https://go.dev/doc/install/source. Use the newly built Go to run the etcd-io/server tests.

Anything else we need to know?

In general we recommend against testing for exact error strings. The Go standard library reserves the right to change error strings. It's better to simply test whether an error occurred or not. If it's necessary to check for a particular error, it's better to use errors.Is or errors.As to check the error. If that is infeasible, it's better to use strings.Contains to check that the error message contains some identifying string. Thanks.

Etcd version (please run commands below)

```console $ etcd --version # paste output here $ etcdctl version # paste output here ```

Etcd configuration (command line flags or environment variables)

linux/amd64

Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

```console $ etcdctl member list -w table # paste output here $ etcdctl --endpoints= endpoint status -w table # paste output here ```

Relevant log output

No response

ivanvc commented 6 days ago

Hi @ianlancetaylor, Thanks for raising this issue. We aim to have the main branch working with the latest Go version released. When a new minor is released. We work to adopt the new version (refer to issues like #18443 and #18548) and plan/log the work in a version bump issue.

Thanks for raising awareness about comparing errors with errors.Is(); we currently have #18510 and #18551 open to track this change.

I think the best action item for this issue is to close it for now, as we'll work on the Go 1.24.0 update when it gets released.

Thanks again, Ian.

ivanvc commented 6 days ago

We discussed this issue and https://github.com/etcd-io/etcd/pull/18510 in our fortnightly etcd triage meeting. We'll keep this issue open and use it as an umbrella to track the adoption of errors.Is.

ivanvc commented 6 days ago

/retitle Use errors.Is for error equality checks

redwrasse commented 6 days ago

/assign

redwrasse commented 6 hours ago

Will try to get time tomorrow to submit PRs for this issue.

etcd-io / etcd