Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: fix anti-pattern name in NewEtcdProcessCluster #14825

Merged

Conversation

clement2026
Copy link
Contributor

Context #14711

Rename anti-pattern-naming fields(DisableStrictReconfigCheck and NoCN) and move them to DefaultConfig.

Signed-off-by: Clark <fwyongxing@gmail.com>
@clement2026 clement2026 marked this pull request as ready for review November 23, 2022 06:12
ForceNewCluster bool
InitialToken string
QuotaBackendBytes int64
DisableStrictReconfigCheck bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sure we should change it.

Both Disable or No seems to enable it by default. If we switch to CN, I think we should change all the places to DefaultXXXConfig, like DefaultConfig. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like DefaultConfig was updated. As this is just an internal test framework I don't see any negative impacts of changing the defaults.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. The WithConfigXYZ always call DefaultConfig. For sure, it looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to review, I should have explained it earlier😬

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM(non-binding)

@serathius serathius merged commit 51c0b4d into etcd-io:main Nov 23, 2022
@clement2026 clement2026 deleted the EtcdProcessClusterConfig-antipattern-name branch September 24, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants