Move ensureDefaultIndexPattern into data plugin #63100

sulemanof · 2020-04-09T11:43:03Z

Summary

Since we started moving legacy plugins into NP,
this comment #59895 (comment) became more than relevant.
While importing ensureDefaultIndexPattern statically, it gets into different plugins bundles, so such variables as bannerId & timeoutId, which are tracked for banner appeared, are not in sync between apps and eventually ends up with duplicated banners (currently the dashboard is in NP, but discover & visualize are about to be moved, so there would be 3 copies of the same banner):

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 IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

lizozom · 2020-04-12T15:07:11Z

src/plugins/data/public/plugin.ts

@@ -182,6 +183,7 @@ export class DataPublicPlugin implements Plugin<DataPublicPluginSetup, DataPubli
      ui: {
        IndexPatternSelect: createIndexPatternSelect(core.savedObjects.client),
        SearchBar,
+        ensureDefaultIndexPattern: createEnsureDefaultIndexPattern(core, indexPatterns),


I think this should not be exported from here.
It's not really a UI component, and the naming makes it extra unclear.
It looks more like it should be a useDefaultIndexPattern hook, that is exported from kibana_react.

This would be the best way to implement this (except the thing about showing the warning banner, it would be disappeared right after a plugin is destroyed),
but unfortunately, right now it's only used across angular driven plugins and I guess there is no way to integrate this pattern well there

@lizozom What do you think about exporting this from IndexPatternsService?

It looks more like it should be a useDefaultIndexPattern hook, that is exported from kibana_react

I agree the ergonomics of this would be better. The only problem is that the way this helper is currently written (with bannerId and timeoutId being stateful), means we really have no choice but to put it somewhere in the runtime contract of the data plugin, as kibana_utils and kibana_react are only imported statically.

I don't have strong feelings on whether it goes in ui or IndexPatternsService, but since it contains state that needs to be shared across apps, it seems those are our only options outside of refactoring the function altogether and storing that state elsewhere (e.g. sessionStorage).

So what would be the final decision?
Should it stay in ui or moved into IndexPatternsService, or even postponed in respect of #57401 ?

If @lizozom doesn't like the idea of putting it in ui, then IMHO it's fine to put it on IndexPatternsService. She should be available tomorrow and can hopefully chime in if she has any other ideas.

or even postponed in respect of #57401 ?

I don't think we should wait on that issue to fix this bug. That discussion is still in early stages & I don't know when the actual implementation will be prioritized.

@sulemanof

I think moving it to the index pattern service is better.
It's not an actual UI component.

Thanks!

sulemanof · 2020-04-13T07:27:10Z

@elasticmachine merge upstream

…dex_pattern # Conflicts: # src/legacy/core_plugins/kibana/public/visualize/np_ready/legacy_app.js

…ibana into np/move_ensure_index_pattern

sulemanof · 2020-04-21T15:20:24Z

This was eventually moved into IndexPatternsService.
Ready for review. cc @lukeelmers @flash1293

sulemanof · 2020-04-22T07:41:42Z

@elasticmachine merge upstream

flash1293

Tested in Chrome and works as expected. Just wondering why documentation changed for the search service...

flash1293 · 2020-04-22T15:09:49Z

docs/development/plugins/data/public/kibana-plugin-plugins-data-public.search.md

        isStringType: (agg: import("./search").AggConfig) => boolean;
-        isType: (type: string) => (agg: import("./search").AggConfig) => boolean;
+        isType: (...types: string[]) => (agg: import("./search").AggConfig) => boolean;


Where is this change coming from?

Seems all of the docs changes were not directly related to changes in this PR,
because after the last merge with the master, all of them are just gone (means the same changes have been already merged -> https://github.com/elastic/kibana/blob/master/docs/development/plugins/data/public/kibana-plugin-plugins-data-public.search.md#search-variable)

Weird, I thought we had a check for that in the build.

CI checks that the latest api.json file is up to date and committed but doesn't verify that docs (based on this file) are up to date. so if you --accept api changes but don't commit the new docs this could happen.

…dex_pattern # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.createsearchsource.md # src/plugins/data/public/public.api.md

…ibana into np/move_ensure_index_pattern

lukeelmers

Code changes here LGTM.

My only nit would be that it'd be nice to add ensureDefaultIndexPattern to our src/data/public/mocks.ts for completeness. The mocks aren't complete for index patterns ATM anyway, but even a simple jest.fn() would be a useful starting point and save us from needing to add this later.

…dex_pattern

sulemanof · 2020-04-24T11:34:51Z

My only nit would be that it'd be nice to add ensureDefaultIndexPattern to our src/data/public/mocks.ts for completeness. The mocks aren't complete for index patterns ATM anyway, but even a simple jest.fn() would be a useful starting point and save us from needing to add this later.

@lukeelmers I'd just deleted the mock in this commit to prevent adding the platform team into this thread, because it makes to update couple of snapshots in management, since it has already lasted too long 😄
Returned back, snapshots updated. Hope this will end up eventually 🙂

rudolf

👍 For platform changes

kibanamachine · 2020-04-24T13:19:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 14cb75a

History

💚 Build #42668 succeeded a7d2a20
💚 Build #42347 succeeded 162d8d7
💔 Build #42150 failed e1a0cf7
💚 Build #40342 succeeded 9519da7
💚 Build #39894 succeeded 3bfa875

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Move ensure_default_index_pattern into data plugin * Update docs to include ensureDefaultIndexPattern * Fix translations * Move helper into index_patterns service * Update docs * Remove mock * Add mock Co-authored-by: Elastic Machine <[email protected]>

* Move ensure_default_index_pattern into data plugin * Update docs to include ensureDefaultIndexPattern * Fix translations * Move helper into index_patterns service * Update docs * Remove mock * Add mock Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

…bana into ingest-node-pipeline/open-flyout-create-edit * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (116 commits) [Ingest Node Pipelines] More lenient treatment of on-failure value (elastic#64411) Report Deletion via UI- functional test (elastic#64031) Bump handlebars dependency from 4.5.3 to 4.7.6 (elastic#64402) [Uptime] Update TLS settings (elastic#64111) [alerting] removes usage of any throughout Alerting Services code (elastic#64161) [CANVAS] Moves notify to a canvas service (elastic#63268) [Canvas] Misc NP Stuff (elastic#63703) update apm index pattern (elastic#64232) Task/hostlist pagination (elastic#63722) [NP] Vega migration (elastic#63849) Move ensureDefaultIndexPattern into data plugin (elastic#63100) [Fleet] Fix agent status count to not include unenrolled agents (elastic#64106) Migrate graph_workspace saved object registration to Kibana platform (elastic#64157) Index pattern management UI -> TypeScript and New Platform Ready (edit_index_pattern) (elastic#64184) [ML] EuiDataGrid ml/transform components. (elastic#63447) [ML] Moving to kibana capabilities (elastic#64057) Move input_control_vis into NP (elastic#63333) remove reference to local application service in graph (elastic#64288) KQL removes leading zero and breaks query (elastic#62748) [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193) ...

sulemanof added 2 commits April 9, 2020 16:24

Move ensure_default_index_pattern into data plugin

2716e33

Update docs to include ensureDefaultIndexPattern

6d0a4e0

sulemanof force-pushed the np/move_ensure_index_pattern branch from 1345cb5 to 6d0a4e0 Compare April 9, 2020 13:33

Fix translations

3bfa875

sulemanof requested review from lukeelmers and flash1293 April 9, 2020 14:57

sulemanof marked this pull request as ready for review April 9, 2020 14:57

sulemanof requested a review from a team April 9, 2020 14:57

sulemanof requested a review from a team as a code owner April 9, 2020 14:57

sulemanof added release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0 labels Apr 9, 2020

lizozom reviewed Apr 12, 2020

View reviewed changes

Merge branch 'master' into np/move_ensure_index_pattern

9519da7

lukeelmers mentioned this pull request Apr 13, 2020

Redesign and better call to action when users have no index pattern and navigating in Kibana #57401

Closed

sulemanof added 5 commits April 21, 2020 13:35

Merge remote-tracking branch 'upstream/master' into np/move_ensure_in…

78f4c97

…dex_pattern # Conflicts: # src/legacy/core_plugins/kibana/public/visualize/np_ready/legacy_app.js

Move helper into index_patterns service

d0532d7

Update docs

1df31de

Merge branch 'np/move_ensure_index_pattern' of github.com:sulemanof/k…

e1a0cf7

…ibana into np/move_ensure_index_pattern

Remove mock

64ba52f

Merge branch 'master' into np/move_ensure_index_pattern

162d8d7

flash1293 approved these changes Apr 22, 2020

View reviewed changes

sulemanof added 2 commits April 23, 2020 10:33

Merge remote-tracking branch 'upstream/master' into np/move_ensure_in…

095edcf

…dex_pattern # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.createsearchsource.md # src/plugins/data/public/public.api.md

Merge branch 'np/move_ensure_index_pattern' of github.com:sulemanof/k…

a7d2a20

…ibana into np/move_ensure_index_pattern

lukeelmers approved these changes Apr 23, 2020

View reviewed changes

sulemanof added 2 commits April 24, 2020 14:24

Add mock

16ad08a

Merge remote-tracking branch 'upstream/master' into np/move_ensure_in…

14cb75a

…dex_pattern

sulemanof requested a review from a team as a code owner April 24, 2020 11:28

rudolf approved these changes Apr 24, 2020

View reviewed changes

sulemanof merged commit 9bda6bd into elastic:master Apr 24, 2020

sulemanof deleted the np/move_ensure_index_pattern branch April 24, 2020 13:20

sulemanof mentioned this pull request Apr 24, 2020

[7.x] Move ensureDefaultIndexPattern into data plugin (#63100) #64425

Merged

sulemanof added the backported label Apr 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ensureDefaultIndexPattern into data plugin #63100

Move ensureDefaultIndexPattern into data plugin #63100

sulemanof commented Apr 9, 2020 •

edited

Loading

lizozom Apr 12, 2020

sulemanof Apr 13, 2020

flash1293 Apr 13, 2020

lukeelmers Apr 13, 2020

sulemanof Apr 15, 2020

lukeelmers Apr 15, 2020

lizozom Apr 20, 2020

sulemanof commented Apr 13, 2020

sulemanof commented Apr 21, 2020

sulemanof commented Apr 22, 2020

flash1293 left a comment

flash1293 Apr 22, 2020

sulemanof Apr 23, 2020

flash1293 Apr 23, 2020

rudolf Apr 24, 2020

lukeelmers left a comment

sulemanof commented Apr 24, 2020

rudolf left a comment

kibanamachine commented Apr 24, 2020

Move ensureDefaultIndexPattern into data plugin #63100

Move ensureDefaultIndexPattern into data plugin #63100

Conversation

sulemanof commented Apr 9, 2020 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sulemanof commented Apr 13, 2020

sulemanof commented Apr 21, 2020

sulemanof commented Apr 22, 2020

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

sulemanof commented Apr 24, 2020

rudolf left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 24, 2020

💚 Build Succeeded

History

sulemanof commented Apr 9, 2020 •

edited

Loading