Forbid timezones not working in Elasticsearch#70780
Conversation
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
|
This PR left me thinking about where the time zones in Elasticsearch come from. Seems like they are bundled with the date library (Joda) in some cases the java native time database (elastic/elasticsearch#56659) - it should come down to the time zones from this database: https://www.iana.org/time-zones Can we somehow make sure they stay in sync? Most likely out of scope for this PR though. |
|
@elasticmachine merge upstream |
|
@flash1293 I think this is more worth a general new discuss issue (since moment pulls it from the same database afaik, so it's most likely more a versioning problem and we might need to expose them via an API... which then means our advanced settings need to be async). But please feel free to open one, if you think it's an issue worth discussing. |
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
lukeelmers
left a comment
There was a problem hiding this comment.
Code here LGTM & the direction makes sense in the absence of a proper solution for keeping moment & joda in sync.
I think this is more worth a general new discuss issue (since moment pulls it from the same database afaik, so it's most likely more a versioning problem and we might need to expose them via an API... which then means our advanced settings need to be async
++ I think this is worth discussing at least, even if there isn't a great solution at the moment... especially as they're both using the same data source. It'd be good to pull the platform team into this discussion as I believe the plan is for them to assume ownership of this setting in the future (once they remove the legacy platform).
| await PageObjects.settings.navigateTo(); | ||
| await PageObjects.settings.clickKibanaSettings(); | ||
| await PageObjects.settings.setAdvancedSettingsSelect('dateFormat:tz', 'EST'); | ||
| await PageObjects.settings.setAdvancedSettingsSelect('dateFormat:tz', 'Etc/GMT+5'); |
There was a problem hiding this comment.
ℹ️ This test did obviously not test what it wanted to test, since it used a timezone, that would not actually allow requests. I assume the test did pass, because it most likely did never refresh the page properly. I want to look into the test separately from this PR, and make it work properly, but for this PR I simply replaced the removed timezone by an equivalent still available time zone.
* Permit timezones not working in Elasticsearch * Fix functional tests * Use timezone without summer time for test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: Rename legacy ES mock accessors (elastic#70432) [APM] Adds 'Anomaly detection' settings page to create ML jobs per environment (elastic#70560) Forbid timezones not working in Elasticsearch (elastic#70780)
…rbac * alerting/consumer-based-rbac: Rename legacy ES mock accessors (elastic#70432) [APM] Adds 'Anomaly detection' settings page to create ML jobs per environment (elastic#70560) Forbid timezones not working in Elasticsearch (elastic#70780) [ML] Adding peak_model_bytes to model size stats type (elastic#70825)
* Permit timezones not working in Elasticsearch * Fix functional tests * Use timezone without summer time for test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Fixes #70352
This removes the timezones from the allowed selection, that moment.js knows about, but Elasticsearch doesn't understand.
I've ran a quick script with all timezones moment knows about against ES, and it seems on master (and the latest release) the following timezones are not understood by Elasticsearch:
This PR removes them from the choices in advanced settings. While this removes options a user could have potentially configured, I don't treat this as a breaking change, since if a user would have any of them configured, none of their requests in Kibana would have worked, so I think the chance anyone has this configuration is rather low. If so, they'd now need to go to advanced settings and change it to something else (but they anyway would need to do so, to have any query working in Kibana).
Checklist
Delete any items that are not applicable to this PR.
[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibility[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser[ ] This was checked for cross-browser compatibility, including a check against IE11For maintainers
[ ] This was checked for breaking API changes and was labeled appropriately