-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[NP] Expose global config to the plugins #51478
Changes from 3 commits
a7d5e97
395f26f
7ea1c50
d9ec3d3
4e31b77
88bc0b1
592e935
db6b130
5f7c0e3
7b7ea57
82b62c8
6caab53
dc9b692
e341ee2
edd8760
03ecab3
3502f9a
146161a
8f67cc9
e729d5e
2bc7916
684e89d
83f1d80
23a34f8
4847fe9
4adebd5
31cbab2
535f50e
79b2133
5951986
9337633
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import { createPluginInitializerContext } from './plugin_context'; | ||
import { CoreContext } from '../core_context'; | ||
import { Env } from '../config'; | ||
import { configServiceMock } from '../config/config_service.mock'; | ||
import { loggingServiceMock } from '../logging/logging_service.mock'; | ||
import { getEnvOptions } from '../config/__mocks__/env'; | ||
import { PluginManifest } from './types'; | ||
import { first } from 'rxjs/operators'; | ||
|
||
const logger = loggingServiceMock.create(); | ||
const configService = configServiceMock.create({ getConfig$: { globalConfig: { subpath: 1 } } }); | ||
|
||
let coreId: symbol; | ||
let env: Env; | ||
let coreContext: CoreContext; | ||
|
||
function createPluginManifest(manifestProps: Partial<PluginManifest> = {}): PluginManifest { | ||
return { | ||
id: 'some-plugin-id', | ||
version: 'some-version', | ||
configPath: 'path', | ||
kibanaVersion: '7.0.0', | ||
requiredPlugins: ['some-required-dep'], | ||
optionalPlugins: ['some-optional-dep'], | ||
server: true, | ||
ui: true, | ||
...manifestProps, | ||
}; | ||
} | ||
|
||
beforeEach(() => { | ||
coreId = Symbol('core'); | ||
env = Env.createDefault(getEnvOptions()); | ||
coreContext = { coreId, env, logger, configService }; | ||
}); | ||
|
||
test('should return a globalConfig handler in the context (to be deprecated)', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: don't think the test name should have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests will need to be changed once we come up with a solution for the long-run. So they are likely to be changed as well as the behaviour. That's why I wanted to make it clear in the test name: it's kind of a warning of "this should work until we decide to rethink this" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the variable name already provides this info. doesn't it? |
||
const manifest = createPluginManifest(); | ||
const opaqueId = Symbol(); | ||
const pluginInitializerContext = createPluginInitializerContext(coreContext, opaqueId, manifest); | ||
|
||
expect(pluginInitializerContext.config.globalConfig__deprecated$).toBeDefined(); | ||
|
||
const configObject = await pluginInitializerContext.config.globalConfig__deprecated$ | ||
.pipe(first()) | ||
.toPromise(); | ||
|
||
const configPaths = configObject.getFlattenedPaths(); | ||
expect(configPaths).toHaveLength(1); | ||
expect(configPaths).toStrictEqual(['globalConfig.subpath']); | ||
expect(configObject.has('globalConfig.subpath')).toBe(true); | ||
expect(configObject.get('globalConfig.subpath')).toBe(1); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,12 @@ | |
* under the License. | ||
*/ | ||
|
||
import { map } from 'rxjs/operators'; | ||
import { CoreContext } from '../core_context'; | ||
import { PluginWrapper } from './plugin'; | ||
import { PluginsServiceSetupDeps, PluginsServiceStartDeps } from './plugins_service'; | ||
import { PluginInitializerContext, PluginManifest, PluginOpaqueId } from './types'; | ||
import { CoreSetup, CoreStart } from '..'; | ||
import { CoreSetup, CoreStart, ConfigPath } from '..'; | ||
|
||
/** | ||
* This returns a facade for `CoreContext` that will be exposed to the plugin initializer. | ||
|
@@ -65,6 +66,19 @@ export function createPluginInitializerContext( | |
* Core configuration functionality, enables fetching a subset of the config. | ||
*/ | ||
config: { | ||
/** | ||
* Global configuration | ||
* Note: naming not final here, it will be renamed in a near future (https:/elastic/kibana/issues/46240) | ||
* @deprecated | ||
*/ | ||
globalConfig__deprecated$: coreContext.configService.getConfig$().pipe( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should only expose whitelisted fields. We don't want to expose the entire config tree to any plugin, only a handpicked list of configs that we know are needed by many plugins (I believe they should be listed in the issue). I would expect this to look more like: type SharedGlobalConfig = Readonly<{
// Maybe use a recursive readonly type if we have one
kibana: Readonly<{
defaultAppId: string;
index: string;
disabledWelcomeScreen: string;
// ...etc
}>;
path: Readonly<{
data: string;
}>;
// ...etc
}
globalConfig__deprecated$: Observable<SharedGlobalConfig>; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh! Thanks! I misunderstood the conversation in the original issue then. I'll rework this :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, this is actually way better than my suggestion |
||
map(conf => ({ | ||
has: (configPath: ConfigPath) => conf.has(configPath), | ||
get: (configPath: ConfigPath) => conf.get(configPath), | ||
getFlattenedPaths: () => conf.getFlattenedPaths(), | ||
})) | ||
), | ||
|
||
/** | ||
* Reads the subset of the config at the `configPath` defined in the plugin | ||
* manifest and validates it against the schema in the static `schema` on | ||
|
@@ -82,6 +96,8 @@ export function createPluginInitializerContext( | |
}; | ||
} | ||
|
||
// function exposeReadOnlyMethods(conf: ) | ||
|
||
/** | ||
* This returns a facade for `CoreContext` that will be exposed to the plugin `setup` method. | ||
* This facade should be safe to use only within `setup` itself. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I think the junit
test
-based suites is kind of the old way of structuring test files, I would prefer a jasminedescribe/it
structure. Can someone confirm that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originally coming from
mocha
I'm happier with thedescribe/it
, but I sawit
is simply an alias oftest
and, according to thejest
documentation, they root fortest
https://jestjs.io/docs/en/apiI don't mind one or another. Whatever we want to go for as a team :)