-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
etcdbackend: support version auto discovery #2299
Conversation
Hi, i do not think the CI failure is relevant to this pr. |
Is rule #2 because you're concerned about breakages with existing data if the API level changes? |
Also, the failure is not related to this. I made a copypasta screwup when fixing tests because of a sad interaction between Go bugs and |
Yes, exactly. |
return newEtcd3Backend(conf, logger) | ||
default: | ||
return nil, EtcdVersionUnknow | ||
} | ||
} | ||
|
||
func getEtcdAPIVersion(c client.Client) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on this function as to how the version is computed might be helpful.
if haEnabled == "" { | ||
haEnabled = conf["ha_enabled"] | ||
} | ||
haEnabledBool, _ := strconv.ParseBool(haEnabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we catch the error here?
Agreed on Vishal's request to catch the ParseBool error (people do put strange things in config file) but after that we'll merge it. |
@jefferai I will clean up today or tomorrow, and ping you guys. Thanks a lot for reviewing it. |
@jefferai @vishalnayak fixed. PTAL! |
👍 |
Rule:
when api version is not defined by user:
when api version is provided by user, always trust user.
Manually tested.