Make `core` responsible for reading and merging of config files. Simplify legacy config adapter. by azasypkin · Pull Request #21956 · elastic/kibana

azasypkin · 2018-08-14T10:24:58Z

This PR switches "legacy" Kibana to read and merge config files via core functionality. This will be done only by core once we merge #22190.

I've also simplified legacy config adapter and removed support for __newPlatform config node, if we need something like this in the future we'll add it back, but since new platform is in master already I don't see any reason to keep this unused functionality.

~~Blocked by #21885~~
Unblocks #19994

…lify legacy config adapter.

elasticmachine · 2018-08-24T10:19:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-24T12:50:43Z

src/core/server/config/raw_config.ts

+   * in extreme cases when there is no other better way, e.g. bridging with the
+   * "legacy" systems that consume and process config in a different way.
+   */
+  toRaw(): Record<string, any>;


note: will be needed here, here and here.

azasypkin · 2018-08-24T12:51:02Z

src/core/server/config/raw_config_service.ts

-  constructor(readonly configFile: string) {
+  constructor(
+    readonly configFiles: ReadonlyArray<string>,
+    configAdapter: (rawValue: Record<string, any>) => RawConfig = rawValue =>


note: I need this "middleware" to be able to apply cli/keystore overrides to config before it will be published to any subscriber.

azasypkin · 2018-08-24T12:51:26Z

src/core/server/index.ts

-      throw new Error(`some config paths are not handled: ${JSON.stringify(unhandledConfigPaths)}`);
+      // We don't throw here since unhandled paths are verified by the "legacy"
+      // Kibana right now, but this will eventually change.
+      this.log.trace(


note: still left trace here, just as a reminder... I'm not sure what to do with that yet. Currently core uses keys defined in the "legacy" schema, so "legacy" Kibana won't complain about unused keys. But once we start using some config keys in the core exclusively and don't define them in the legacy schema then we'll need to expose something like core.getAppliedConfigKeys() so that "legacy" Kibana doesn't mark these keys as unused/unknown (or just filter out core-only settings from config we send to the "legacy" kibana, that would be even easier).

azasypkin · 2018-08-24T12:53:06Z

This one is ready for review @spalger :)

Thanks!

spalger · 2018-08-27T22:02:53Z

Alright, I'll take a look today (was out Friday)

spalger

Code looks good, and things run nicely, haven't dug too deep into the tests, but spot checked and things look good. I'm struggling a bit with the naming, as noted in the comments, but I suspect these are more situations where things will make more sense once we get the rest of the big PR in...

spalger · 2018-08-28T00:49:46Z

src/core/server/config/read_config.ts

+  });
+}
+
+function merge(target: any, value: any, key: string = '') {


feels like key should just be optional, not default to an empty string.

Yes, was too lazy to write else if (key !== undefined) { to make lodash.set type signature happy :) But agree, will change that.

spalger · 2018-08-28T01:08:56Z

src/core/server/config/raw_config_service.ts

+      new ObjectToRawConfigAdapter(rawValue)
+  ) {
    this.config$ = this.rawConfigFromFile$.pipe(
      filter(rawConfig => rawConfig !== notRead),


Could this filtering be avoided by using a ReplaySubject(1) instead of a BehaviorSubject(notRead)?

Absolutely! We also we'll need to review all other places in the server core where observables are used - kbn/observable didn't have a lot of useful operators/factories that we have now.

spalger · 2018-08-28T01:40:21Z

src/core/server/config/object_to_raw_config_adapter.ts

 */
 export class ObjectToRawConfigAdapter implements RawConfig {
-  constructor(private readonly rawValue: { [key: string]: any }) {}
+  constructor(protected readonly rawValue: Record<string, any>) {}


Looks like this can stay private.

Yep, thanks! Was experimenting with something and likely forgot to revert

spalger · 2018-08-28T01:42:27Z

src/core/server/config/object_to_raw_config_adapter.ts

 * Allows plain javascript object to behave like `RawConfig` instance.
 * @internal
 */
 export class ObjectToRawConfigAdapter implements RawConfig {


What is the ObjectTo... part of this name supposed to represent?

Well, I'm bad at naming 🙁

At some point we had "adapter" for the legacy config (src/server/config/config.js) to be able to use it everywhere where RawConfig was expected, so it was called LegacyConfigToRawConfigAdapter (like in Number.toString() or just NumberToString 🙂 ):

LegacyConfig (src/server/config/config.js) + ----> (to) + RawConfig (src/core/server/config/raw_config.ts)

But for the non-legacy world we have a plain JavaScript object (or just Object) instead, that was parsed from yaml config file, so it was called ObjectToRawConfigAdapter (probably PlainObjectTo... would have been better):

Record<string, any> (Object) + ----> (to) + RawConfig (src/core/server/config/raw_config.ts)

We got rid of dependency on the legacy config now and can work with underlying raw config object, but we still need some transformation before core can use it (e.g. logging config format is different), so to bridge it with the legacy world I created LegacyObjectToRawConfigAdapter, Legacy part is just to stress that we do something there to support legacy world, and ObjectToRawConfigAdapter part as explained above.

If it sounds really confusing we can definitely rename this at any point, just let me know what name would sound better?

spalger · 2018-08-28T01:44:50Z

src/core/server/config/raw_config_service.ts

-  constructor(readonly configFile: string) {
+  constructor(
+    readonly configFiles: ReadonlyArray<string>,
+    configAdapter: (rawValue: Record<string, any>) => RawConfig = rawValue =>


Feels like this is mapping the rawConfig to a config instance, which is why the input observable is called rawConfigFromFile$ and the result observable is config$, but the name configAdapter doesn't seem to communicate that, and the fact that it's supposed to return a RawConfig is even more confusing IMO.

Agree, it's a way to confusing now. Let's leave raw to real "raw" plain JavaScript config object parsed from yaml and rename RawConfig to just Config (and remove raw from names of adapters as well). I was previously concerned about RawConfig.toRaw, and having Config.toRaw makes me feel a bit better :)

…Script raw config and `Config` class instance.

elasticmachine · 2018-08-28T12:15:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-28T14:28:52Z

6.x/6.5: dc04be0

azasypkin added blocked Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform v6.5.0 labels Aug 14, 2018

azasypkin mentioned this pull request Aug 14, 2018

[skip ci] Use new platform CLI as the main entry point #19994

Closed

22 tasks

azasypkin force-pushed the issue-xxx-core-raw-config branch 2 times, most recently from 583fcf7 to 7843504 Compare August 20, 2018 14:30

azasypkin mentioned this pull request Aug 20, 2018

Implement LegacyService. Use core to start legacy Kibana. #22190

Merged

azasypkin force-pushed the issue-xxx-core-raw-config branch from 7843504 to 66a3d7d Compare August 22, 2018 10:08

azasypkin removed the blocked label Aug 23, 2018

azasypkin changed the title ~~[skip ci] Make core responsible for reading and merging of config files. Simplify legacy config adapter.~~ Make core responsible for reading and merging of config files. Simplify legacy config adapter. Aug 23, 2018

azasypkin force-pushed the issue-xxx-core-raw-config branch from 66a3d7d to 5454031 Compare August 23, 2018 20:50

This comment has been minimized.

Sign in to view

azasypkin force-pushed the issue-xxx-core-raw-config branch from 5454031 to 320b45b Compare August 24, 2018 06:34

This comment has been minimized.

Sign in to view

Make core responsible for reading and merging of config files. Simp…

51fb213

…lify legacy config adapter.

azasypkin force-pushed the issue-xxx-core-raw-config branch from 320b45b to 51fb213 Compare August 24, 2018 08:40

azasypkin commented Aug 24, 2018

View reviewed changes

azasypkin requested a review from spalger August 24, 2018 12:53

spalger approved these changes Aug 28, 2018

View reviewed changes

azasypkin added 5 commits August 28, 2018 09:20

Merge branch 'master' into issue-xxx-core-raw-config

fe99940

ObjectToRawConfigAdapter.rawValue can stay private.

9c04cd5

Rename RawConfig to Config and make separation between plain Java…

2b15fac

…Script raw config and `Config` class instance.

Use ReplaySubject instead of BehaviorSubject in RawConfigService.

d21d693

Make key argument optional in read_config.merge.

9678afc

Remove <number> debug leftover.

a2f0001

This comment has been minimized.

Sign in to view

Specify value type explicitly in ReplaySubject.

f4a6a36

This comment has been minimized.

Sign in to view

azasypkin merged commit 6034cc7 into elastic:master Aug 28, 2018

azasypkin deleted the issue-xxx-core-raw-config branch August 28, 2018 12:18

azasypkin added the backport pending label Aug 28, 2018

azasypkin mentioned this pull request Aug 28, 2018

[6.x] Make core responsible for reading and merging of config files. Simplify legacy config adapter. (#21956) #22447

Merged

azasypkin added backported and removed backport pending labels Aug 28, 2018

azasypkin mentioned this pull request Nov 26, 2018

Handle deprecated SSL config settings. #26196

Merged

Conversation

azasypkin commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Aug 24, 2018

💚 Build Succeeded

Uh oh!

azasypkin Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin commented Aug 24, 2018

Uh oh!

spalger commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Aug 28, 2018

💚 Build Succeeded

Uh oh!

azasypkin commented Aug 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

azasypkin commented Aug 14, 2018 •

edited

Loading

azasypkin Aug 24, 2018 •

edited

Loading

azasypkin Aug 24, 2018 •

edited

Loading

spalger commented Aug 27, 2018 •

edited

Loading

azasypkin Aug 28, 2018 •

edited

Loading