[NP] Migrate uiSettings owned by Kibana app by maryia-lapata · Pull Request #64321 · elastic/kibana

maryia-lapata · 2020-04-23T14:17:52Z

Summary

Fixes #63597.

This PR migrates the following ui settings to the new platform:

to discover:
- defaultColumns
- discover:sampleSize
- discover:aggs:terms:size
- discover:sort:defaultOrder
- discover:searchOnPageLoad
- doc_table:hideTimeColumn
- fields:popularLimit
- context:defaultSize
- context:step
- context:tieBreakerFields
to data:
- metaFields
- doc_table:highlight
to charts:
- visualization:colorMapping
to vis_type_vislib:
- visualization:dimmingOpacity
- visualization:heatmap:maxBuckets
to saved_objects:
- savedObjects:perPage
- savedObjects:listingLimit
to vis_type_timeseries:
- metrics:max_buckets
remove visualization:loadingDelay since it's unused.

i18n IDs were updated respectively.

maryia-lapata · 2020-04-24T11:57:40Z

@elasticmachine merge upstream

elasticmachine · 2020-04-24T11:57:42Z

merge conflict between base and head

kertal · 2020-04-27T11:31:00Z

@elasticmachine merge upstream

elasticmachine · 2020-04-27T11:31:02Z

merge conflict between base and head

kertal · 2020-04-28T09:34:05Z

@elasticmachine merge upstream

elasticmachine · 2020-04-28T09:34:07Z

merge conflict between base and head

nreese

Maps changes LGTM
code review

src/plugins/saved_objects_management/public/management_section/mount_section.tsx

pgayvallet

I'm +1 with @restrry's comment here:

I wonder if moving the uiSettings name constants to their respective new owners and exposing these constants to other plugins is really the correct approach here.

Main issue with this is that you can import / use settings declared from plugins you don't have a declared dependency with (see first review comment)

I think exposing accessors to the settings via their owners contracts is probably more compliant with KP way of doing things, and will also ensure that you cannot use/access settings from plugins you don't depend on.

Accessing a setting from within the owner plugin's code would remain unchanged

import { MAX_BUCKETS_SETTING } from '../common/constants';
 const maxBuckets = config.get(MAX_BUCKETS_SETTING);

But accessing a setting owned by another plugin would be something like

const externalSetting = startDeps.dependencyPlugin.settings.getMaxBuckets()

I'm aware these are way more impactful changes than this PR's implementation, but I really think that's the way to go.

@restrry / @joshdover are we on the same page?

src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts

…ettings - no consumers need to set this value via initialPageSize

…to setting accessors

joshdover · 2020-05-11T19:45:53Z

I agree with that plugins should only have access to the settings that either (1) Core exposes (2) plugins they depend on expose (3) they own themselves.

I also think that to accomplish that we need a much larger change to how this service works in order to enforce those boundaries. Same goes for saved object types. We probably shouldn't allow arbitrary access to any SO type.

So maybe for now we try to follow the pattern @pgayvallet laid out above where it's easy & feasible, but if there are some cases that don't fit nicely into that pattern we can solve that separately later?

kertal · 2020-05-11T20:28:34Z

So maybe for now we try to follow the pattern @pgayvallet laid out above where it's easy & feasible, but if there are some cases that don't fit nicely into that pattern we can solve that separately later?

I do agree and I'm currently adapting the PR to the pattern @pgayvallet suggested, where possible

src/plugins/discover/server/ui_settings.ts

mshustov · 2020-05-12T15:20:06Z

src/plugins/data/server/ui_settings.ts

+    schema: schema.arrayOf(schema.string()),
+  },


Optional: I'm wondering whether we know all of them to declare validation as a union of well-known strings.

here are mapping fields. but not all e.g. _score of the query context is missing, think it should stay flexible, just in case
https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-fields.html

src/plugins/dashboard/public/application/legacy_app.js

src/plugins/saved_objects/server/plugin.ts

src/plugins/vis_type_vislib/server/plugin.ts

src/plugins/visualize/public/application/listing/visualize_listing.js

flash1293

Tested against current master and all settings are still there. LGTM.

While looking through the code I noticed we need to do the same thing for a bunch of timelion settings, I will create a separate issue for that.

src/plugins/charts/server/plugin.ts

kibanamachine · 2020-05-15T12:31:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: fe8dc06

History

💚 Build #47891 succeeded 8d4b8c2
💔 Build #47690 failed ca7915a
💔 Build #47601 failed 185d29c
💚 Build #47097 succeeded 785c3a5
💔 Build #46937 failed 72e29b7

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

alexwizp · 2020-05-15T12:42:52Z

...re_plugins/kibana/public/management/sections/index_patterns/edit_index_pattern/tabs/tabs.tsx

 import { fieldWildcardMatcher } from '../../../../../../../../../plugins/kibana_utils/public';
 import { IndexPatternManagementStart } from '../../../../../../../../../plugins/index_pattern_management/public';
 import { IndexPattern, IndexPatternField } from '../../../../../../../../../plugins/data/public';
+import { META_FIELDS_SETTING } from '../../../../../../../../../plugins/data/common';


If I'm not wrong we are trying to avoid imports from /common folder.

Also I think that this approach introduce a lot of fields in our static contract which also not good.

Maybe we can move all into one variable:

const UI_SETTINGS = { META_FIELDS_SETTING: 'meta_field', DEFAULT_QUERY_LANGUAGE: 'kuery', META_FIELDS_SETTING: `'metaFields',` }

sorry, seems we had a comment/merge race condition. seen this comment after the merge. since this part of Kibana was in the legacy world, and it's currently migrated, this could be done in or after #65026

* Move META_FIELDS_SETTING, DOC_HIGHLIGHT_SETTING to data plugin.ts * Refactor table_list_view.tsx to no longer get PER_PAGE_SETTING by uiSettings * Migrate graph, visualize, dashboard usage of saved_objects module constants to accessor functions * Remove redundant logging in plugins start and setup methods Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co> # Conflicts: # src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts

* Move META_FIELDS_SETTING, DOC_HIGHLIGHT_SETTING to data plugin.ts * Refactor table_list_view.tsx to no longer get PER_PAGE_SETTING by uiSettings * Migrate graph, visualize, dashboard usage of saved_objects module constants to accessor functions * Remove redundant logging in plugins start and setup methods Co-authored-by: Maryia Lapata <mary.lopato@gmail.com> Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co>

* master: (191 commits) [Maps] Get number of categories from palette (elastic#66454) move oss features registration to KP (elastic#66524) [kbn/plugin-helpers] typescript-ify (elastic#66513) Add kibana-operations as codeowners for .ci/es-snapshots and vars/ (elastic#66746) FTR: move basic services under common folder (elastic#66563) Migrate Beats Management UI to KP (elastic#65791) [CI] Add 20 minutes to overall build timeout lint import from restricted zones for export exressions (elastic#66588) [SIEM][Detection Engine] Add validation for Rule Actions (elastic#63332) KP plugins shouldn't need package.json (elastic#66654) Replace agent metrics link with the new one (elastic#66632) [CI] Add one retry to setup step (elastic#66638) [CI] Add slack alerts to tracked branch jobs, change default channel, change formatting (elastic#66580) [docLinks] Add docLinks to CoreSetup. (elastic#66631) [DOCS] Rename monitoring collection from internal to legacy (elastic#65781) unskip newsfeed tests (elastic#66562) [NP] Migrate uiSettings owned by Kibana app (elastic#64321) [ML] Functional tests - stabilize typing in DFA mml input (elastic#66706) [Map] return bounding box for static feature collection without joins (elastic#66607) remove trailing slash in graph sample data links (elastic#66358) ...

elasticmachine · 2020-05-25T18:07:14Z

Pinging @elastic/kibana-app (Team:KibanaApp)

Migrate uiSettings

4efa64e

maryia-lapata added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.8.0 labels Apr 23, 2020

maryia-lapata added 3 commits April 24, 2020 10:08

Migrate uiSettings to visTypeVislib

77b75cd

Refactoring

db60224

Update index_pattern.ts

6761400

Merge remote-tracking branch 'kibana/master' into ui_settings

cf723a9

maryia-lapata assigned sulemanof Apr 24, 2020

kertal added 2 commits April 27, 2020 13:37

Merge master /fix conflicts

83b29f7

Add defaults for fixing failing tests

c0c8327

kertal self-assigned this Apr 28, 2020

Merge master /fix conflicts

1d21edd

kertal marked this pull request as ready for review April 28, 2020 13:27

kertal requested a review from a team April 28, 2020 13:27

kertal requested review from a team as code owners April 28, 2020 13:27

kertal requested a review from flash1293 April 28, 2020 13:28

nreese approved these changes Apr 28, 2020

View reviewed changes

mshustov reviewed Apr 28, 2020

View reviewed changes

src/plugins/saved_objects_management/public/management_section/mount_section.tsx Outdated Show resolved Hide resolved

Merge master /fix conflicts

33587b0

pgayvallet reviewed May 1, 2020

View reviewed changes

src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts Outdated Show resolved Hide resolved

kertal added 2 commits May 11, 2020 15:41

Refactor table_list_view.tsx to no longer get PER_PAGE_SETTING by uiS…

72e29b7

…ettings - no consumers need to set this value via initialPageSize

Migrate graph usage of saved_objects module constants for uiSettings …

785c3a5

…to setting accessors

mshustov reviewed May 12, 2020

View reviewed changes

kertal added 4 commits May 13, 2020 12:52

Adapt Visualize code

185d29c

Cleanups

17c291a

Adapt dashboard

5df721d

Merge master /fix conflicts

ca7915a

mshustov approved these changes May 13, 2020

View reviewed changes

Fix types

8d4b8c2

kertal requested a review from mattkime May 14, 2020 15:43

mattkime approved these changes May 14, 2020

View reviewed changes

lukeelmers mentioned this pull request May 14, 2020

[uiSettings]: Migrate items owned by app arch to new platform #66040

Merged

7 tasks

flash1293 approved these changes May 15, 2020

View reviewed changes

src/plugins/charts/server/plugin.ts Outdated Show resolved Hide resolved

kertal added 2 commits May 15, 2020 12:20

Merge remote-tracking branch 'upstream/master' into ui_settings

6119854

Remove chart plugin logging

fe8dc06

alexwizp reviewed May 15, 2020

View reviewed changes

kertal merged commit 7c8eea1 into elastic:master May 15, 2020

kertal added v7.9.0 and removed v7.8.0 labels May 15, 2020

kertal mentioned this pull request May 15, 2020

[7.x] [NP] Migrate uiSettings owned by Kibana app (#64321) #66730

Merged

lukeelmers mentioned this pull request May 19, 2020

[data] Expose uiSettings in plugin contract #67060

Closed

kertal added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label May 25, 2020

VladLasitsa mentioned this pull request Oct 7, 2020

Removed visualization:loadingDelay setting docs #79841

Merged

8 tasks

Conversation

maryia-lapata commented Apr 23, 2020 • edited by kertal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

maryia-lapata commented Apr 24, 2020

Uh oh!

elasticmachine commented Apr 24, 2020

Uh oh!

kertal commented Apr 27, 2020

Uh oh!

elasticmachine commented Apr 27, 2020

Uh oh!

kertal commented Apr 28, 2020

Uh oh!

elasticmachine commented Apr 28, 2020

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshdover commented May 11, 2020

Uh oh!

kertal commented May 11, 2020

Uh oh!

Uh oh!

mshustov May 12, 2020

Choose a reason for hiding this comment

Uh oh!

kertal May 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented May 15, 2020

💚 Build Succeeded

History

Uh oh!

alexwizp May 15, 2020

Choose a reason for hiding this comment

Uh oh!

kertal May 15, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

maryia-lapata commented Apr 23, 2020 •

edited by kertal

Loading