-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Allow float settings to be configured with other settings as default #126751
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
Allow float settings to be configured with other settings as default #126751
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @nicktindall, I've created a changelog YAML for you. |
ldematte
left a comment
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.
LGTM
| ); | ||
| assertThat( | ||
| floatSetting.get( | ||
| Settings.builder().put("float_val.default", configuredDefaultValue).put("float_val", configuredSettingValue).build() |
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.
Nit: this would have been maybe more useful if it was something else, e.g.
assertThat(
floatSetting.get(
Settings.builder().put("float_val.default", someOtherDefaultValue).put("float_val", configuredSettingValue).build()),
equalTo(configuredSettingValue)
);
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.
I think it is, but my naming is not great.
i.e. defaultSetting (the default for float_val.default) != configuredDefault (the configured value for float_val.default != configuredSetting (the configured value for float_val). I'll fix the naming up to make that clearer.
Broken out of #126091