Move ui/index_patterns code to data plugin. by lukeelmers · Pull Request #42994 · elastic/kibana

lukeelmers · 2019-08-08T23:19:20Z

Summary

Initial pass at moving the contents of ui/index_patterns into the data plugin, while re-exporting the contracts from ui/index_patterns to preserve backwards compatibility.

This also introduces mocks for the data plugin, which are used to provide a mock for the entire legacy contract of ui/index_patterns. This means tests relying on ui/index_patterns can just use jest.mock('ui/index_patterns') without worrying about the fact that it's talking to the data plugin under the hood.

There should be no breaking changes as a result of this PR.

There are a lot of follow-up tasks that will need to happen to prepare index patterns for the new platform, including finishing TS conversion and cleaning out legacy dependencies. This PR is solely focused on reshaping the code and moving into the data plugin with a proper setup lifecycle contract, which will make further changes easier to manage.

Dev Docs

No functional or breaking changes were introduced here, however, the file structure of index patterns was changed somewhat from how it was structured in the original module:

src/legacy/core_plugins/data/public/index_patterns/
├── components
│   ├── index.ts
│   └── index_pattern_select.tsx
├── errors.ts
├── fields
│   ├── field.ts
│   ├── field_list.ts
│   └── index.ts
├── index.ts
├── index_patterns
│   ├── __tests__
│   │   └── _get_computed_fields.js
│   ├── _fields_fetcher.ts
│   ├── _pattern_cache.ts
│   ├── flatten_hit.js
│   ├── format_hit.ts
│   ├── index.ts
│   ├── index_pattern.test.js
│   ├── index_pattern.ts
│   ├── index_patterns.test.js
│   ├── index_patterns.ts
│   ├── index_patterns_api_client.test.js
│   ├── index_patterns_api_client.test.mock.js
│   └── index_patterns_api_client.ts
├── index_patterns_service.mock.ts
├── index_patterns_service.ts
├── utils.test.ts
└── utils.ts

elasticmachine · 2019-08-08T23:19:22Z

Pinging @elastic/kibana-app-arch

src/legacy/core_plugins/data/public/index_patterns/index_patterns_service.mock.ts

src/legacy/core_plugins/data/public/plugin.ts

lizozom

Looks good.
Just a few questions and LGTM.

...management/sections/index_patterns/edit_index_pattern/create_edit_field/create_edit_field.js

src/legacy/ui/public/courier/courier.js

lukeelmers · 2019-08-13T19:51:19Z

@elastic/kibana-platform FYI, the codeowners review here is for a small change to a test in src/legacy/ui/public/saved_objects/__tests__/saved_object.js

lukeelmers · 2019-08-13T19:53:52Z

src/test_utils/public/stub_index_pattern.js

This was no longer consistent with the actual IndexPattern class, and is only used directly in a few places, so I updated the args

lukeelmers · 2019-08-13T19:57:10Z

src/legacy/ui/public/index_patterns/__mocks__/index.ts

Since code was moved into the shimmed data plugin, a bunch of tests failed due to stuff that needed to be mocked now that the whole data plugin was getting kicked off.

So the approach here was to mimic jest.mock('ui/new_platform')... basically if your plugin is still importing from the legacy location in ui/index_patterns, you can use the new platform mock along with jest.mock('ui/index_patterns'), and everything will be taken care of for you; no need to worry about what downstream dependencies the data plugin has.

rudolf

ui/public/saved_objects changes LGTM

lizozom

Added two minor questions but otherwise LGTM

lizozom · 2019-08-18T14:17:37Z

src/legacy/core_plugins/data/public/index_patterns/index_patterns_service.ts

Shouldn't these two guys be in __LEGACY?

lizozom · 2019-08-18T14:21:45Z

src/legacy/core_plugins/data/public/index_patterns/components/index_pattern_select.tsx

I see you added the stateful initializer! So cool. :-)
I guess we're going to have one in many components.
Do you have an idea for a common name? (not necessarily for this PR!)

Do you have an idea for a common name? (not necessarily for this PR!)

Yeah I think that's a good question. One possibility (which is the approach I took here for the time being) would be the use the same name in both places. If it's in the lifecycle, you can assume it is "pre-wired", and if it is exported statically, assume it isn't.

There are a number of reasons this isn't ideal, however (could lead to confusion when suddenly a component with the same name has a different contract when imported statically). I don't have any great ideas on that right now, but worth thinking about.

I'm more interested in how many people will prefer to have a static import which requires more configuration, vs a stateful one which doesn't. I think that remains to be seen still.

streamich

Code LGTM

streamich · 2019-08-19T09:26:15Z

src/legacy/core_plugins/data/public/index_patterns/index_patterns_service.mock.ts

Does this mock have to be here? It might be unexpected if user wants to themselves mock ui/chrome, but it is already mocked.

Yeah I debated whether to mock ui/chrome in each of the individual tests that were using index patterns, vs mocking it in one place here.

The reason I settled on this implementation was because I felt we should minimize the amount of stuff people need to know about dependencies of their dependencies. Each of the tests knows it is using index patterns, so it is reasonable to ask them to use jest.mock('ui/index_patterns'). And they know they are (eventually) moving over to new platform, so it's reasonable to ask them to jest.mock('ui/new_platform').

But they don't necessarily know that index patterns have moved to the data plugin, which starts up some legacy stuff, which relies on chrome. It felt unfair to ask them to figure this out, which is why I moved the mock here instead.

There are a few scenarios where this has downsides though:

Case you mentioned: Users want to provide custom mocks for chrome, or require the "real" chrome. In this case they'd need to unmock or requireActual.

Users writing tests with index patterns have other code that needs chrome to be mocked as well. Their tests are "magically" working, but may break sometime in the future when we remove chrome (and the mock) from index patterns.

Overall I think scenario 1 will be relatively uncommon for most users. Scenario 2 could be more common, but is still something we can fix in the PR where we remove chrome from index patterns.

…rnSelect components.

lukeelmers · 2019-08-19T18:42:25Z

I tested this firefox smoke test failure locally, and I was seeing it on master too -- so it may not be related. Going to push and let CI run again to see how it goes

       └-: rollup job
         └-> "before all" hook
         └-: rollup job
           └-> "before all" hook
           └-> "before all" hook
             └- ✖ fail: "rollup job rollup job "before all" hook"
             │      [invalid_alias_name_exception] Invalid alias name [.kibana], an index exists with the same name as the alias, with { index_uuid="EFwODD3MTNCVbLI6CwauAA" & index=".kibana" }
             │   :: {"path":"/.kibana_1","query":{"include_type_name":false},"body":"{\"settings\":{\"index\":{\"number_of_replicas\":\"1\",\"number_of_shards\":\"1\"}},\"mappings\":{\"dynamic\":\"strict\",\"properties\":{\"canvas-workpad\":{\"dynamic\":\"false\",\"properties\":{\"@created\":{\"type\":\"date\"},\"@timestamp\":{\"type\":\"date\"},\"id\":{\"index\":false,\"type\":\"text\"},\"name\":{\"fields\":{\"keyword\":{\"type\":\"keyword\"}},\"type\":\"text\"}}},\"config\":{\"dynamic\":\"true\",\"properties\":{\"buildNum\":{\"type\":\"keyword\"}}},\"dashboard\":{\"properties\":{\"description\":{\"type\":\"text\"},\"hits\":{\"type\":\"integer\"},\"kibanaSavedObjectMeta\":{\"properties\":{\"searchSourceJSON\":{\"type\":\"text\"}}},\"optionsJSON\":{\"type\":\"text\"},\"panelsJSON\":{\"type\":\"text\"},\"refreshInterval\":{\"properties\":{\"display\":{\"type\":\"keyword\"},\"pause\":{\"type\":\"boolean\"},\"section\":{\"type\":\"integer\"},\"value\":{\"type\":\"integer\"}}},\"timeFrom\":{\"type\":\"keyword\"},\"timeRestore\":{\"type\":\"boolean\"},\"timeTo\":{\"type\":\"keyword\"},\"title\":{\"type\":\"text\"},\"uiStateJSON\":{\"type\":\"text\"},\"version\":{\"type\":\"integer\"}}},\"graph-workspace\":{\"properties\":{\"description\":{\"type\":\"text\"},\"kibanaSavedObjectMeta\":{\"properties\":{\"searchSourceJSON\":{\"type\":\"text\"}}},\"numLinks\":{\"type\":\"integer\"},\"numVertices\":{\"type\":\"integer\"},\"title\":{\"type\":\"text\"},\"version\":{\"type\":\"integer\"},\"wsState\":{\"type\":\"text\"}}},\"index-pattern\":{\"properties\":{\"fieldFormatMap\":{\"type\":\"text\"},\"fields\":{\"type\":\"text\"},\"intervalName\":{\"type\":\"keyword\"},\"notExpandable\":{\"type\":\"boolean\"},\"sourceFilters\":{\"type\":\"text\"},\"timeFieldName\":{\"type\":\"keyword\"},\"title\":{\"type\":\"text\"},\"type\":{\"type\":\"keyword\"},\"typeMeta\":{\"type\":\"keyword\"}}},\"kql-telemetry\":{\"properties\":{\"optInCount\":{\"type\":\"long\"},\"optOutCount\":{\"type\":\"long\"}}},\"migrationVersion\":{\"dynamic\":\"true\",\"type\":\"object\"},\"namespace\":{\"type\":\"keyword\"},\"search\":{\"properties\":{\"columns\":{\"type\":\"keyword\"},\"description\":{\"type\":\"text\"},\"hits\":{\"type\":\"integer\"},\"kibanaSavedObjectMeta\":{\"properties\":{\"searchSourceJSON\":{\"type\":\"text\"}}},\"sort\":{\"type\":\"keyword\"},\"title\":{\"type\":\"text\"},\"version\":{\"type\":\"integer\"}}},\"server\":{\"properties\":{\"uuid\":{\"type\":\"keyword\"}}},\"space\":{\"properties\":{\"_reserved\":{\"type\":\"boolean\"},\"color\":{\"type\":\"keyword\"},\"description\":{\"type\":\"text\"},\"initials\":{\"type\":\"keyword\"},\"disabledFeatures\":{\"type\":\"keyword\"},\"name\":{\"fields\":{\"keyword\":{\"ignore_above\":2048,\"type\":\"keyword\"}},\"type\":\"text\"}}},\"telemetry\":{\"properties\":{\"enabled\":{\"type\":\"boolean\"}}},\"timelion-sheet\":{\"properties\":{\"description\":{\"type\":\"text\"},\"hits\":{\"type\":\"integer\"},\"kibanaSavedObjectMeta\":{\"properties\":{\"searchSourceJSON\":{\"type\":\"text\"}}},\"timelion_chart_height\":{\"type\":\"integer\"},\"timelion_columns\":{\"type\":\"integer\"},\"timelion_interval\":{\"type\":\"keyword\"},\"timelion_other_interval\":{\"type\":\"keyword\"},\"timelion_rows\":{\"type\":\"integer\"},\"timelion_sheet\":{\"type\":\"text\"},\"title\":{\"type\":\"text\"},\"version\":{\"type\":\"integer\"}}},\"type\":{\"type\":\"keyword\"},\"updated_at\":{\"type\":\"date\"},\"url\":{\"properties\":{\"accessCount\":{\"type\":\"long\"},\"accessDate\":{\"type\":\"date\"},\"createDate\":{\"type\":\"date\"},\"url\":{\"fields\":{\"keyword\":{\"ignore_above\":2048,\"type\":\"keyword\"}},\"type\":\"text\"}}},\"visualization\":{\"properties\":{\"description\":{\"type\":\"text\"},\"kibanaSavedObjectMeta\":{\"properties\":{\"searchSourceJSON\":{\"type\":\"text\"}}},\"savedSearchId\":{\"type\":\"keyword\"},\"title\":{\"type\":\"text\"},\"uiStateJSON\":{\"type\":\"text\"},\"version\":{\"type\":\"integer\"},\"visState\":{\"type\":\"text\"}}}}},\"aliases\":{\".kibana\":{}}}","statusCode":400,"response":"{\"error\":{\"root_cause\":[{\"type\":\"invalid_alias_name_exception\",\"reason\":\"Invalid alias name [.kibana], an index exists with the same name as the alias\",\"index_uuid\":\"EFwODD3MTNCVbLI6CwauAA\",\"index\":\".kibana\"}],\"type\":\"invalid_alias_name_exception\",\"reason\":\"Invalid alias name [.kibana], an index exists with the same name as the alias\",\"index_uuid\":\"EFwODD3MTNCVbLI6CwauAA\",\"index\":\".kibana\"},\"status\":400}"}
             │       at respond (/var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-firefoxSmoke/node/linux-immutable/kibana/node_modules/elasticsearch/src/lib/transport.js:315:15)
             │       at checkRespForFailure (/var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-firefoxSmoke/node/linux-immutable/kibana/node_modules/elasticsearch/src/lib/transport.js:274:7)
             │       at HttpConnector.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-firefoxSmoke/node/linux-immutable/kibana/node_modules/elasticsearch/src/lib/connectors/http.js:166:7)
             │       at IncomingMessage.wrapper (/var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-firefoxSmoke/node/linux-immutable/kibana/node_modules/elasticsearch/node_modules/lodash/lodash.js:4929:19)
             │       at endReadableNT (_stream_readable.js:1103:12)
             │       at process._tickCallback (internal/process/next_tick.js:63:19)
             │ 
             │ 
             │undefined
             │46 passing (16.0m)
             │7 pending
             │1 failing
             │undefined

elasticmachine · 2019-08-19T19:47:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers added review Feature:Data Views Data Views code and UI - index patterns before 8.0 v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.4.0 Feature:NP Migration labels Aug 8, 2019

lukeelmers self-assigned this Aug 8, 2019

This comment has been minimized.

Sign in to view

lukeelmers force-pushed the feat/index-patterns-move branch from 40037bc to 2a5fa46 Compare August 13, 2019 04:56

This comment has been minimized.

Sign in to view

lizozom reviewed Aug 13, 2019

View reviewed changes

src/legacy/core_plugins/data/public/index_patterns/index_patterns_service.mock.ts Outdated Show resolved Hide resolved

lizozom reviewed Aug 13, 2019

View reviewed changes

src/legacy/core_plugins/data/public/plugin.ts Outdated Show resolved Hide resolved

lizozom reviewed Aug 13, 2019

View reviewed changes

...management/sections/index_patterns/edit_index_pattern/create_edit_field/create_edit_field.js Outdated Show resolved Hide resolved

src/legacy/ui/public/courier/courier.js Outdated Show resolved Hide resolved

lukeelmers force-pushed the feat/index-patterns-move branch from 2a5fa46 to 5d01797 Compare August 13, 2019 19:47

lukeelmers marked this pull request as ready for review August 13, 2019 19:48

lukeelmers requested a review from a team as a code owner August 13, 2019 19:48

lukeelmers requested a review from ppisljar August 13, 2019 19:50

lukeelmers commented Aug 13, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

lukeelmers force-pushed the feat/index-patterns-move branch from 5d01797 to f7b7d9e Compare August 15, 2019 22:49

This comment has been minimized.

Sign in to view

rudolf approved these changes Aug 16, 2019

View reviewed changes

lukeelmers force-pushed the feat/index-patterns-move branch from f7b7d9e to 599327c Compare August 16, 2019 17:09

lukeelmers requested a review from a team August 16, 2019 17:09

ogupte approved these changes Aug 16, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

lizozom approved these changes Aug 18, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

streamich approved these changes Aug 19, 2019

View reviewed changes

lukeelmers added 14 commits August 19, 2019 10:57

Move ui/index_patterns files to data plugin.

5bee0d4

Export updated service contract & re-export from ui/public.

9b14ec8

Fix broken import.

8244018

Fix TS errors & create data service mocks.

2572ca5

Fix broken CCR & Rollup tests.

14063c5

Update translation IDs.

b7807b7

Update stubbed index pattern.

dc14f4f

Revert changes to imports in courier.

dcb1c55

Move Provider to static export, add both static & prewired IndexPatte…

dd42901

…rnSelect components.

Update ui/index_patterns mocks and fix broken jest tests.

66f8beb

Clean up index pattern jest test.

9ee5a9c

Fix remaining jest tests.

5108a4b

Move fixtures from .mock file to utils and export as static.

ea39d1e

Fix tslint failure.

75cf732

lukeelmers force-pushed the feat/index-patterns-move branch from a49c490 to 75cf732 Compare August 19, 2019 18:42

lukeelmers merged commit 1c0813e into elastic:master Aug 19, 2019

lukeelmers deleted the feat/index-patterns-move branch August 19, 2019 20:12

lukeelmers mentioned this pull request Aug 19, 2019

[7.x] Move ui/index_patterns code to data plugin. (#42994) #43546

Merged

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Aug 19, 2019

Move ui/index_patterns code to data plugin. (elastic#42994)

e438ad1

lukeelmers added a commit that referenced this pull request Aug 20, 2019

[7.x] Move ui/index_patterns code to data plugin. (#42994) (#43546)

7a8c345

lukeelmers mentioned this pull request Aug 22, 2019

Dev Docs entry: ui/public cleanup 7.4 #39907

Closed

mshustov mentioned this pull request Sep 1, 2020

Cleanup src/test_utils #76355

Closed

8 tasks

Conversation

lukeelmers commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dev Docs

Uh oh!

elasticmachine commented Aug 8, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lukeelmers commented Aug 13, 2019

Uh oh!

lukeelmers Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

lukeelmers Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

lizozom Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

lizozom Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

lukeelmers Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

streamich left a comment

Choose a reason for hiding this comment

Uh oh!

streamich Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

lukeelmers Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

lukeelmers commented Aug 19, 2019

Uh oh!

elasticmachine commented Aug 19, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lukeelmers commented Aug 8, 2019 •

edited

Loading