server/embed: address golangci var-naming issues

[ivanvc]

Addresses issues with config.{TLSMinVersion,TLSMaxVersion,WALDir,MaxWALFiles}.

This is part 1 of 2 from server/embed, as it's the package with the most changes.

Part of #17578.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

[ivanvc]

/retest

[ahrtr]

LGTM

Thanks @ivanvc

[ahrtr]

cc @fuweid @jmhbnz @serathius

[jmhbnz]

LGTM - Thanks @ivanvc

[serathius]

I don't think it's worth to be pedantic to complicate the config struct so much. If this is a blocker for enabling var-naming on whole repo, we should just exclude this file.

[ahrtr]

I don't think it's worth to be pedantic to complicate the config struct so much. If this is a blocker for enabling var-naming on whole repo, we should just exclude this file.

• Overall it isn't that complicated; this PR follows the same pattern to handle the 4 fields in Config struct. It is super clear and easy to understand.
• It's also backward compatible change.
• I won't insist on merging this PR if majority maintainers &members are against it, but I still think we should welcome & open to such change which improves readability, although it's minor.

[serathius]

but I still think we should welcome & open to such change which improves readability, although it's minor.

For me using linters like var-naming is not about readability, it's about code style consistency. Don't have strong preference about WALDir vs WalDir. However, duplicating fields definitely reduces readability.

[ahrtr]

For me using linters like var-naming is not about readability, it's about code style consistency.

Making code style consistent is part of readability.

However, duplicating fields definitely reduces readability.

It's a temporary solution to make the change backward compatible. We will remove one of them in 3.7. Also @ivanvc has been already following the same pattern in previous PRs.

Again, overall not a big problem, doesn't worth too much getting stuck on this. defer to majority maintainers & member's opinions.

[ivanvc]

As the author of these Pull Requests, I left these changes as the last ones, as I felt they were the most controversial (and the decision agreed in this PR will impact follow-up PRs on other config fields with inconsistent naming). This time, getting to a position in which the change is backward compatibility was the most intricate. I initially suggested doing a linter ignore for these. Still, I think that if we don't deprecate the variable names now, it's improbable that we'll rename them later in another release without prior deprecation.

I think applying the linter rule helps with code style consistency and gives internal (and external) developers a better experience. For example, with the current state of the code (and inconsistencies with naming Wal and WAL), this could be potentially an end-user code that has an embedded etcd:

if cfg.MaxWalFiles != embed.DefaultMaxWALFiles {
  ...
}

Developers with an IDE may not have an issue with the naming, but it may not be very pleasant in other scenarios.

I'm advocating for enabling var-naming to have a more mature and formal naming for internal and external developers. It'd be good to hear from others and @fuweid, who worked to enable linter rules in the past.

[fuweid]

+1 for this change. We don't break command line interface and I believe it's good timing to do clean up. Yes, it needs some effort to maintain two fields. We can consider to add UT to cover that.

NOTE: When I re-enable lint, I just want to focus on existing lint and keep the change in small.

[ivanvc]

@ahrtr, do we want any other team members to chime on this? Please cc who you think should have an opinion on this. Thanks.

[ahrtr]

@serathius can we merge this PR?

[serathius]

I don't like the change as it's the most controversial from all static analysis changes. It duplicates fields on user facing config which complicates the code and user interface. I wanted to discuss it further make sure we are not overzealous and just apply linters blindly. Making change now means we are committing to a new API, that we won't be able to clean up anytime soon. And for what benefit? Just because we didn't want to put //nolint:var-naming comment before a variable name. v3.7 will not come anytime soon.

Nonetheless, the change is don't I will block the change if community supports it. @ahrtr feel free to merge if you think it's benefitial.

[ahrtr]

v3.7 will not come anytime soon.

Actually I am not insist on merging this PR. Due to bad maintenance history, each etcd minor release is actually a LTS version. If we have the same release cadence as K8s, e.g. a minor release each 3 ~ 4 months, then I will merge this PR. But the reality is we may release 3.7 at least 3+ years later based on the etcd release history.

@ivanvc So let's disable the linter for the fields in Config. Thanks for your work and sorry for the confusion.

[ivanvc]

This is a struct exclusively used in this test case. It tests that marshaling the JSON fields works as expected, feeding it as a YAML config file. So, it should be fine to rename this field

[serathius]

Right

[ivanvc]

@serathius, @ahrtr PTAL 🙏

[ahrtr]

LGTM

Thank you !