Move timezone settings into autoload file#22623
Conversation
timroes
added
Feature:Visualizations
timroes
requested review from
azasypkin,
chrisronline,
markov00,
ppisljar,
spalger and
walterra
markov00
left a comment
markov00
left a comment
There was a problem hiding this comment.
Code LGTM. Good to merge after CI pass
💔 Build Failed |
|
Retest |
walterra
left a comment
walterra
left a comment
There was a problem hiding this comment.
LGTM - I tested the code change regarding ML and the timezone gets still picked up correctly!
💔 Build Failed |
|
retest |
ppisljar
left a comment
ppisljar
left a comment
There was a problem hiding this comment.
code LGTM after CI goes green
💚 Build Succeeded |
azasypkin
left a comment
azasypkin
left a comment
There was a problem hiding this comment.
LGTM, but, if possible, please add a couple of jest tests for the new autoload/settings.js to make sure we correctly set initial and changed values.
During testing I've also noticed one weird thing: by default uiSettings.get('dateFormat:tz') returns Browser string that we pass directly to moment.tz.setDefault(...) that doesn't feel right even though it may work as expected right now, once we change timezone to some non-default value and then reset back to Browser, subscribers will be notified with null value instead of Browser string. Just observation, not sure if it's something we should take care of right now 🤷♂️
chrisronline
left a comment
chrisronline
left a comment
There was a problem hiding this comment.
LGTM! I messed around with absolute times in monitoring UI and didn't see any regressions
💔 Build Failed |
|
Jenkins, test this - And ES build failure once again |
💔 Build Failed |
|
retest |
spalger
left a comment
spalger
left a comment
There was a problem hiding this comment.
LGTM, created #22668 to deal with the weirdness spotted by @azasypkin upstream.
💚 Build Succeeded |
* Move timezone settings into autoload file * Remove applying setting from timelion * Remove manual set from ML * Remove manual set from monitoring * Remove now obsolete code from embedding test plugin
8 participants
Until now we loaded the
dateFormat:tzanddateFormat:dowsetting via theKibanaRootController, which will be applied usingchrome.setRootController. That has the huge disadvantage, that every app plugin won't have that setting applied and would need to manually apply it tomoment.I moved that behavior to a new
autoload/settingfile, which is also included intoautoload/all. That way every app plugin that includesui/autoload/settingorui/autoload/allwill automatically have those settings.I found this issue, while writing functional plugin tests for the visualize loader API (see #22595), and seeing that no timezone is applied when embedding visualizations. I think using an
autoloadfile is the better solution that requiring every app plugin to do exactly the same by itself (or like the dashboard only plugin, even import controllers from thekibanaplugin).For QA: This PR should not change any behavior in Kibana, all time zone settings should still work as beforehand.
CC:
Dev Docs
Time zone in application plugins
If you are using
import 'ui/autoload/all';in your application plugin (orimport 'ui/autoload/settings';directly) the timezone and start of the week configured in Kibana will now correctly be set tomomentand thus be used by Kibana core services, like the visualize loader API to embed visualizations.