-
Notifications
You must be signed in to change notification settings - Fork 8.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
Settings should use snake_case #7444
Comments
kibana.yml
kibana.yml
I think this is a good idea. Consistency is great, but relying on capitalization in general for configuration is less than ideal. We'd need to deprecate the existing configurations so that they continue to work and just log a warning when used. |
++ And once implemented, we should change the documentation to use the new forms and in 6.0 we can remove support for camel casing. |
is someone already working on this ? if not i would give it a try ... |
Nope, if no one's assigned, it's a safe bet that no one is working on it. |
There are existing setting deprecation logic, both outright deprecation as well as a concept called "legacy" which we used when there was a 1:1 upgrade of config properties between versions. Take a look at those and see if they help. |
Is this still actively under consideration? |
kibana.yml
kibana.yml
Updated with the recommendation to keep all config keys camelCase for consistency until we change all config keys in a single major refactor. |
@joshdover told me you were planning to do this for 7.6? Just wanted to confirm as your roadmap seems to be planning this for 8.0.0-alpha1? First, questions:
Second point, If that's actually planned for 7.6: I just wanted to express my concern that this doesn't feel imho doable in such a short amount of time because of the following reasons: 1. The refactor seems to include some namespace renaming
The NP configuration works with namespace/prefixes. Currently, all server related properties are under the 2. Switching all properties to snake_caseI have no objection per say if that's a consensus (imho I think it's a good idea), however I just want to point out that currently, all our TS configuration types are 'generated' from the configuration schemas const schema = schema.object(
{
autoListen: schema.boolean({ defaultValue: true }),
basePath: schema.maybe(
schema.string({
validate: match(validBasePathRegex, "must start with a slash, don't end with one"),
})
)
}
)
export type ConfigType = TypeOf<typeof config.schema>; Changing the schema declarations to match the new snake_case properties means either:
Being a JS developer, I personally dislike the idea of having to use snake_case in actual types, as it goes against the language conventions/consensus on the subject, so I'm leaning toward option two, but that's only an opinion. However what is certain is that we need to decide which option we want to take, and to keep in mind that both have heavy impacts on the core codebase. 3. Configurations schemas are split between legacy and new platformWe are in a migration period. Not all properties have been migrated from legacy to the Kibana Platform. Performing these changes now means impacts on both 'worlds'. Also legacy is mostly written in javascript, meaning that we will not have the TS compiler to back us up during the configuration refactor, meaning that it will probably take more time than if we were doing this after complete migration. |
Thanks for context on the platform changes. I'm up for further discussion on the second issue - Generally I'm not a fan of added complexity over convention, mostly due to things getting lost in translation. e.g. looking up a kibana.yml key and having to trace it through several abstractions to find the source. I do need to look at the new config service though, I suspect I don't have a full understanding of what it is involved. I thought it was closer to the old service with getters and setters for some reason. We have camel case settings as a blocker and may need to punt on non camel case renames if necessary. The blocker urgency on the ops side is to remove our docker whitelist of environment variables that get converted to settings - we want this done automatically. |
kibana.yml
This is not something which we are targeting for 7.6. We see this as a breaking change which we're looking at for 8.0. |
Kibana uses a service name as a prefix for a config key, while elasticsearch doesn't. Are we concerned with such a tiny level of inconsistency?
If yes, we should decide on the naming convention across the whole stack. |
Lets do this transformation to snake case when reading the We will need to identify any instances/plugins which have configuration that use snake case already to keep it consistent. @elastic/kibana-platform, can you weigh in here? This is also starting to turn into more of a platform related change. |
You mean converting snake_cased properties to camelCase when reading the config right? Or is it the other way around? Either way, that doesn't solve the point 2. of #7444 (comment) I think? |
We should weigh up the pro's and con's for this specific area, but we have a similar snake case vs camelcase problem with API's. In the case of API's we decided the overhead of trying to convert between the standards is not worth it #52284 (comment). It also makes it much harder to track down a variable in the code base as it might not be obvious that there's a translation layer converting it. |
align cors settings naming schema in both Kibana and Elasticsearch:
server.cors.allow-credentials: boolean;
server.cors.allow-origin: string[];
server.cors.allowCredentials: boolean;
server.cors.allowOrigin: string[]; |
`v91.3.1`⏩`v92.0.0-backport.0` --- ## [`v92.0.0-backport.0`](https://github.com/elastic/eui/releases/v92.0.0-backport.0) **This is a backport release only intended for use by Kibana.** **Bug fixes** - Fixed an `EuiTreeView` JSX Typescript error ([#7452](elastic/eui#7452)) - Fixed a color console warning being generated by disabled `EuiStep`s ([#7454](elastic/eui#7454)) ## [`v92.0.0`](https://github.com/elastic/eui/releases/v92.0.0) - Updated generic types of `EuiBasicTable`, `EuiInMemoryTable` and `EuiSearchBar.Query.execute` to add `extends object` constraint ([#7340](elastic/eui#7340)) - This change should have no impact on your applications since the updated types only affect properties that exclusively accept object values. - Added a new `EuiFlyoutResizable` component ([#7439](elastic/eui#7439)) - Updated `EuiTextArea` to accept `isClearable` and `icon` as props ([#7449](elastic/eui#7449)) **Bug fixes** - `EuiRange`/`EuiDualRange`'s track ticks & highlights now update their positions on resize ([#7442](elastic/eui#7442)) **Deprecations** - Updated `EuiFilterButton` to remove the second `.euiFilterButton__textShift` span wrapper. Target `.euiFilterButton__text` instead ([#7444](elastic/eui#7444)) **Breaking changes** - Removed deprecated `EuiNotificationEvent`. We recommend copying the component to your application if necessary ([#7434](elastic/eui#7434)) - Removed deprecated `EuiControlBar`. We recommend using `EuiBottomBar` instead ([#7435](elastic/eui#7435))
`v91.3.1`⏩`v92.0.0-backport.0` --- ## [`v92.0.0-backport.0`](https://github.com/elastic/eui/releases/v92.0.0-backport.0) **This is a backport release only intended for use by Kibana.** **Bug fixes** - Fixed an `EuiTreeView` JSX Typescript error ([elastic#7452](elastic/eui#7452)) - Fixed a color console warning being generated by disabled `EuiStep`s ([elastic#7454](elastic/eui#7454)) ## [`v92.0.0`](https://github.com/elastic/eui/releases/v92.0.0) - Updated generic types of `EuiBasicTable`, `EuiInMemoryTable` and `EuiSearchBar.Query.execute` to add `extends object` constraint ([elastic#7340](elastic/eui#7340)) - This change should have no impact on your applications since the updated types only affect properties that exclusively accept object values. - Added a new `EuiFlyoutResizable` component ([elastic#7439](elastic/eui#7439)) - Updated `EuiTextArea` to accept `isClearable` and `icon` as props ([elastic#7449](elastic/eui#7449)) **Bug fixes** - `EuiRange`/`EuiDualRange`'s track ticks & highlights now update their positions on resize ([elastic#7442](elastic/eui#7442)) **Deprecations** - Updated `EuiFilterButton` to remove the second `.euiFilterButton__textShift` span wrapper. Target `.euiFilterButton__text` instead ([elastic#7444](elastic/eui#7444)) **Breaking changes** - Removed deprecated `EuiNotificationEvent`. We recommend copying the component to your application if necessary ([elastic#7434](elastic/eui#7434)) - Removed deprecated `EuiControlBar`. We recommend using `EuiBottomBar` instead ([elastic#7435](elastic/eui#7435))
We should align with ES, Beats & LS here... use lower case with
_
as a separator.For example, instead of
kibana.defaultAppId
usekibana.default_app_id
(actually, the in this specific example, I'd even remove the_id
part...kibana.default_app
is enough)/cc @epixa
Update 2019-12-05:
Until we've refactored all config keys in a single big refactor, all existing and new config keys should keep using camelCase for consistency.
The text was updated successfully, but these errors were encountered: