Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI settings move to NP #47590

Merged
merged 31 commits into from
Oct 16, 2019
Merged

UI settings move to NP #47590

merged 31 commits into from
Oct 16, 2019

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Oct 8, 2019

Summary

This PR moves UiSettingsClient creation to the NP API. We don't expose this API to the plugins, the only Legacy platform uses it under the hood.
If SavedObjectsClient is not available in the NP we can work around it and use one from LP. As suggested here.
That unblocks exposing UiSettings client via RequestHandlerContext.
Objections @elastic/kibana-platform ?

I'm going to move uiSettings routes to NP in the following PR

Checklist

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

For maintainers

@mshustov mshustov added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.5.0 labels Oct 8, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

});

describe('getDefaults() argument', () => {
it('casts sync `getDefaults()` to promise', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why it was done in this manner. Probably for the sake of consistency with other accessing methods.

buildNum,
savedObjectsClient,
// we use a function for getDefaults() so that defaults can be different in
// different scenarios, and so they can change over time
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


server.decorate('server', 'uiSettingsServiceFactory', (options = {}) => {
return uiSettingsServiceFactory(server, {
getDefaults,
overrides,
Copy link
Contributor Author

@mshustov mshustov Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found only savedObjectsClient passed as a parameter in the plugins code

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@mshustov mshustov requested a review from rudolf October 15, 2019 07:33
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple docs suggestions 👍

src/core/server/ui_settings/ui_settings_client.ts Outdated Show resolved Hide resolved
src/core/server/ui_settings/ui_settings_service.ts Outdated Show resolved Hide resolved
src/core/server/ui_settings/ui_settings_service.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ML changes LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov merged commit 5676ac0 into elastic:master Oct 16, 2019
@mshustov mshustov deleted the ui-settings-move-np branch October 16, 2019 12:18
mshustov added a commit to mshustov/kibana that referenced this pull request Oct 16, 2019
* add tests for logWithMetadata in LP

* allow passing metadata to log in NP & LP

* move ui_settings_client to NP

* add ui_settings config

* add ui_settings_service

* switch to NP logging

* export types

* bootstrap uiSettings service in NP

* pass NP uiSettings to LP

* move ui_settings mock to NP

* add  test for mixin and switch to NP logger

* make UiSettingsClient.getDefaults sync as it is

* ui_settings_client uses private fields

* ui_settings_client uses private methods

* keep uiSettings config validation in NP only

* update mocks

* core context should know it is mocked

* add tests for ui_settings_service

* remove unused code from ui_settings_mixin test

* improve types in ui_settings_mixin test

* gen docs

* test moved to NP

* set pkg version in tests explicitly

* update mocks in tests

* UiSettingsServiceSetup --> InternalUiSettingsServiceSetup

* add links to types

* address eli comment

* regen docs

* remove unused types
mshustov added a commit that referenced this pull request Oct 16, 2019
* add tests for logWithMetadata in LP

* allow passing metadata to log in NP & LP

* move ui_settings_client to NP

* add ui_settings config

* add ui_settings_service

* switch to NP logging

* export types

* bootstrap uiSettings service in NP

* pass NP uiSettings to LP

* move ui_settings mock to NP

* add  test for mixin and switch to NP logger

* make UiSettingsClient.getDefaults sync as it is

* ui_settings_client uses private fields

* ui_settings_client uses private methods

* keep uiSettings config validation in NP only

* update mocks

* core context should know it is mocked

* add tests for ui_settings_service

* remove unused code from ui_settings_mixin test

* improve types in ui_settings_mixin test

* gen docs

* test moved to NP

* set pkg version in tests explicitly

* update mocks in tests

* UiSettingsServiceSetup --> InternalUiSettingsServiceSetup

* add links to types

* address eli comment

* regen docs

* remove unused types
@mshustov mshustov mentioned this pull request Dec 23, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants