Advanced settings component registry ⇒ kibana platform plugin by mattkime · Pull Request #55940 · elastic/kibana

mattkime · 2020-01-25T01:04:26Z

Summary

Move advanced settings component registry to kibana platform plugin.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
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~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2020-01-25T03:09:44Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

mattkime · 2020-01-25T03:16:01Z

src/plugins/advanced_settings/public/component_registry/index.ts

+
+registerDefaultComponents();
+
+export const componentRegistry = {


API right here

mattkime · 2020-01-27T04:56:26Z

@elasticmachine merge upstream

legrego · 2020-01-27T22:32:37Z

I’ll review tomorrow!

legrego

Spaces changes LGTM! Tested locally

mattkime · 2020-01-29T16:30:39Z

@elasticmachine merge upstream

mattkime · 2020-01-29T16:30:43Z

@elasticmachine merge upstream

x-pack/legacy/plugins/spaces/public/advanced_settings/advanced_settings_service.test.tsx

src/plugins/advanced_settings/public/component_registry/index.ts

lukeelmers · 2020-01-29T21:12:13Z

src/plugins/advanced_settings/public/types.ts

+export interface AdvancedSettingsSetup {
+  componentRegistry: typeof componentRegistry;
+}
+export interface AdvancedSettingsStart {
+  componentRegistry: typeof componentRegistry;
+}


We probably don't want these interfaces to be the same since it is a registry, right?

i.e. on Setup maybe only register should be available, and on Start only get, so that nobody is trying to register after setup

It looks like its sufficient to provide register access in the setup lifecycle although it doesn't seem necessary to me - but I might be missing something. That said, I'm a little uncertain how this relates to legacy code - I'd be using register after setup is complete, correct?

The reasoning behind doing it this way in other registries has been that, if you only allow registering in the setup lifecycle, by the time you get to start you can guaranteed that all items that are going to be registered are already present, and can then safely use get to retrieve anything from the registry.

This avoids the scenario where you may try to do something with the items in a registry which has not been fully populated yet.

It's a little weird in this case because I believe the only place the registry is being read from is in the advanced settings UI itself, which means new registry items will likely just re-render in React if they come in late. So I'm not sure if it would have a material impact in this particular plugin or not; I'm mostly bringing it up for the sake of consistency with how we've dealt with registries in other plugins. Examples of places where we've done this are expressions, inspector, management.

Hm, what about license changes? How does that play with the setup / start, register / get pattern?

lukeelmers · 2020-01-29T21:14:53Z

src/plugins/advanced_settings/public/component_registry/index.ts

+  componentType: {
+    PAGE_TITLE_COMPONENT,
+    PAGE_SUBTITLE_COMPONENT,
+    PAGE_FOOTER_COMPONENT,
+  },


It feels like these should be static exports instead of part of the runtime contract, WDYT?

Perhaps in public/index.ts as:

export const component = { PAGE_TITLE_COMPONENT, PAGE_SUBTITLE_COMPONENT, PAGE_FOOTER_COMPONENT, };

Seems like thats likely one more import. Changed to an enum which seems to make code a little more compact. Past that I don't have a strong opinion.

src/plugins/advanced_settings/public/plugin.ts

...ins/kibana/public/management/sections/settings/__snapshots__/advanced_settings.test.tsx.snap

lukeelmers · 2020-01-31T20:39:42Z

do I really want the UI to reconfigure itself without input from the user?

I assume that would be the desired behavior for a scenario where someone makes a license change though, right? I'd assume the available advanced settings reconfigure themselves immediately after that change.

Both of the observable-based suggestions make sense to me and seem like a nice way to handle it. In particular I think the second approach is a nice experience because you don't have to track enabled/disabled, and users who aren't concerned about license changes don't have to know anything about the observables implementation.

mattkime · 2020-01-31T21:19:17Z

I assume that would be the desired behavior for a scenario where someone makes a license change though, right?

That makes sense if one person is using Kibana, but what if there are a number of people using it?

…ttkime/kibana into adv_settings_componenet_reg_to_plugin

lukeelmers · 2020-01-31T22:36:11Z

The license is still for the entire Kibana / stack installation... so if an administrator changes the license, the license observable should be emitting an update to anybody who is looking at Kibana advanced settings, and their UI would adjust accordingly, no?

Edit: or perhaps in that scenario a page refresh would be required for other users to see the changes

legrego · 2020-02-03T18:07:34Z

I'm curious if this specific registry really needs the ability to respond to license updates. As far as I'm aware, this registry is used by two consumers:

Telemetry
Spaces

Telemetry is now part of OSS, so there are no license considerations there...it will always be available, unless the plugin itself is disabled. Disabling the plugin requires a restart anyway, so the UI wouldn't need to update itself dynamically in this case.

Spaces is part of X-Pack, but it is always available under the Basic license. Like telemetry, the only way to remove it is to disable the plugin entirely, which requires a restart.

If we aren't aware of any future consumers who need this functionality, then is it something we actually need to solve for here?

This reverts commit 335a8e8.

mattkime · 2020-02-03T23:04:52Z

@legrego

If we aren't aware of any future consumers who need this functionality, then is it something we actually need to solve for here?

Heh, good point. No.

mattkime · 2020-02-04T02:49:58Z

@elasticmachine merge upstream

…ttkime/kibana into adv_settings_componenet_reg_to_plugin

kibanamachine · 2020-02-04T15:18:03Z

💚 Build Succeeded

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

History

💔 Build #24221 failed 65e7b7b
💔 Build #24195 failed 5ae8a5e
💔 Build #24190 failed 44df84b
💚 Build #23887 succeeded 8ddbcc7
💔 Build #23852 failed f7390f4

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

lukeelmers

Changes all LGTM! And @legrego raises a great point on licensing. Definitely not an issue that needs to block this PR given the limited usage of this particular service.

…c#55940) * advanced settings component registry to new platform

#56808) * advanced settings component registry to new platform

* master: (23 commits) Properly handle password change for users authenticated with provider other than `basic`. (elastic#55206) Improve pull request template proposal (elastic#56756) Only change handlers as the element changes (elastic#56782) [SIEM][Detection Engine] Final final rule changes (elastic#56806) [SIEM][Detection Engine] critical blocker, wrong ilm policy, need to match beats ilm policy Move ui/agg_types in to shim data plugin (elastic#56353) [SIEM] Fixes Signals count spinner (elastic#56797) [docs] Update upgrade version path (elastic#56658) [Canvas] Use unique Id for Canvas Embeddables (elastic#56783) [Rollups] Adjust max width for job detail panel (elastic#56674) Prevent http client from converting our form data (elastic#56772) Disable creating alerts client instances when ESO plugin is using an ephemeral encryption key (elastic#56676) Bumps terser-webpack-plugin to 2.3.4 (elastic#56662) Advanced settings component registry ⇒ kibana platform plugin (elastic#55940) [Endpoint] EMT-67: add kql support for endpoint list (elastic#56328) Implement UI for Create Alert form (elastic#55232) Fix: Filter pill base coloring (elastic#56761) fix open close signal on detail page (elastic#56757) [Search service] Move loadingCount to sync search strategy (elastic#56335) Rollup TSVB integration: Add test and fix warning text (elastic#56639) ...

mattkime added 2 commits January 24, 2020 19:02

advanced settings component registry to new platform

be0e773

change plugin name, fix some i18n

9e7d525

mattkime changed the title ~~advanced settings component registry to new platform~~ Advanced settings component registry ⇒ new platform Jan 25, 2020

mattkime changed the title ~~Advanced settings component registry ⇒ new platform~~ Advanced settings component registry ⇒ kibana platform plugin Jan 25, 2020

fix karma mocks

f3b467b

mattkime added v8.0.0 v7.7.0 Team:AppArch labels Jan 25, 2020

mattkime added the release_note:skip Skip the PR/issue when compiling release notes label Jan 25, 2020

mattkime commented Jan 25, 2020

View reviewed changes

mattkime added 2 commits January 24, 2020 22:29

fix jest tests

380838c

fix jest tests

b13abeb

mattkime marked this pull request as ready for review January 25, 2020 06:20

mattkime requested review from a team as code owners January 25, 2020 06:20

Merge branch 'master' into adv_settings_componenet_reg_to_plugin

ffdf36e

legrego approved these changes Jan 28, 2020

View reviewed changes

Merge branch 'master' into adv_settings_componenet_reg_to_plugin

7b225aa

mattkime requested a review from lukeelmers January 29, 2020 17:50

lukeelmers reviewed Jan 29, 2020

View reviewed changes

mattkime added 5 commits January 29, 2020 21:23

refactor component registry

4a00882

refactor component registry

5ef28a5

refactor component registry

fdacef1

Merge branch 'master' into adv_settings_componenet_reg_to_plugin

095efaf

fix spaces test

f10a154

mattkime added 2 commits January 31, 2020 16:14

fix snapshot, change component type to use strings as ids

01689f3

Merge branch 'adv_settings_componenet_reg_to_plugin' of github.com:ma…

f7390f4

…ttkime/kibana into adv_settings_componenet_reg_to_plugin

mattkime added 4 commits January 31, 2020 17:09

Merge branch 'master' into adv_settings_componenet_reg_to_plugin

823a0c3

fix tests

5b0caf9

karma fix

8ddbcc7

Merge branch 'master' into adv_settings_componenet_reg_to_plugin

66215ef

legrego mentioned this pull request Feb 3, 2020

Spaces -> Client to NP #54298

Merged

mattkime added 3 commits February 3, 2020 16:58

use BehaviorSubject for subscribing to component state changes

335a8e8

Merge branch 'master' into adv_settings_componenet_reg_to_plugin

832b1c4

Revert "use BehaviorSubject for subscribing to component state changes"

44df84b

This reverts commit 335a8e8.

remove comment

5ae8a5e

elasticmachine and others added 3 commits February 3, 2020 21:50

Merge branch 'master' into adv_settings_componenet_reg_to_plugin

65e7b7b

Merge branch 'master' into adv_settings_componenet_reg_to_plugin

b0b2d13

Merge branch 'adv_settings_componenet_reg_to_plugin' of github.com:ma…

ea8c1bf

…ttkime/kibana into adv_settings_componenet_reg_to_plugin

lukeelmers approved these changes Feb 4, 2020

View reviewed changes

mattkime merged commit 16d412f into elastic:master Feb 4, 2020

mattkime mentioned this pull request Feb 4, 2020

[7.x] Advanced settings component registry ⇒ kibana platform plugin (#55940) #56784

Closed

mattkime added a commit to mattkime/kibana that referenced this pull request Feb 4, 2020

Advanced settings component registry ⇒ kibana platform plugin (elasti…

1c08a01

…c#55940) * advanced settings component registry to new platform

mattkime added a commit to mattkime/kibana that referenced this pull request Feb 4, 2020

Advanced settings component registry ⇒ kibana platform plugin (elasti…

71795a1

…c#55940) * advanced settings component registry to new platform

mattkime mentioned this pull request Feb 4, 2020

[7.x] Advanced settings component registry ⇒ kibana platform plugin (#55940) #56808

Merged

mattkime added a commit that referenced this pull request Feb 5, 2020

Advanced settings component registry ⇒ kibana platform plugin (#55940) (

2b26a39

#56808) * advanced settings component registry to new platform


		registerDefaultComponents();

		export const componentRegistry = {

Conversation

mattkime commented Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jan 25, 2020

Uh oh!

mattkime Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

mattkime commented Jan 27, 2020

Uh oh!

legrego commented Jan 27, 2020

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

mattkime commented Jan 29, 2020

Uh oh!

mattkime commented Jan 29, 2020

Uh oh!

Uh oh!

Uh oh!

lukeelmers Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

mattkime Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

lukeelmers Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

mattkime Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

lukeelmers Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

mattkime Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lukeelmers commented Jan 31, 2020

Uh oh!

mattkime commented Jan 31, 2020

Uh oh!

lukeelmers commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legrego commented Feb 3, 2020

Uh oh!

mattkime commented Feb 3, 2020

Uh oh!

mattkime commented Feb 4, 2020

Uh oh!

kibanamachine commented Feb 4, 2020

💚 Build Succeeded

History

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mattkime commented Jan 25, 2020 •

edited

Loading

lukeelmers commented Jan 31, 2020 •

edited

Loading