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

Move core config service to kbn/config package #76874

Merged
merged 30 commits into from
Sep 16, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
29d8dfc
move deprecations and raw loader to package
pgayvallet Sep 7, 2020
8269fa0
move config service to package
pgayvallet Sep 7, 2020
aa32943
start to adapt the usages
pgayvallet Sep 7, 2020
7d79ca2
adapt yet more usages
pgayvallet Sep 7, 2020
545c867
update generated doc
pgayvallet Sep 7, 2020
92243ba
move logging types to `@kbn/logging`
pgayvallet Sep 10, 2020
c965735
update generated doc
pgayvallet Sep 10, 2020
c016d45
add yarn.lock symlink
pgayvallet Sep 10, 2020
601173c
Merge remote-tracking branch 'upstream/master' into kbn-76003-kbn-con…
pgayvallet Sep 11, 2020
1086b29
Merge branch 'kbn-xxx-kbn-logger-package' into kbn-76003-kbn-config-p…
pgayvallet Sep 11, 2020
67d6575
merge @kbn-logging PR
pgayvallet Sep 11, 2020
f825d42
adapt Env.createDefault
pgayvallet Sep 11, 2020
b2dbebe
update generated doc
pgayvallet Sep 11, 2020
3c1fac0
remove mock exports from the main entrypoint to avoid importing it in…
pgayvallet Sep 11, 2020
e1152a1
use dynamic require to import `REPO_ROOT` from bootstrap file
pgayvallet Sep 14, 2020
ba6b703
Merge remote-tracking branch 'upstream/master' into kbn-76003-kbn-con…
pgayvallet Sep 14, 2020
0bb5c5d
move logger mock to kbn-logging package
pgayvallet Sep 14, 2020
b0c9209
Merge remote-tracking branch 'upstream/master' into kbn-76003-kbn-con…
pgayvallet Sep 15, 2020
d9069c3
address review comments
pgayvallet Sep 15, 2020
d1fe014
Merge remote-tracking branch 'upstream/master' into kbn-76003-kbn-con…
pgayvallet Sep 15, 2020
7999389
import PublicMethodOf from kbn/utility-types
pgayvallet Sep 15, 2020
29e8ae6
fix import conflict
pgayvallet Sep 15, 2020
4d874ef
update generated doc
pgayvallet Sep 15, 2020
fb4bc59
Merge remote-tracking branch 'upstream/master' into kbn-76003-kbn-con…
pgayvallet Sep 15, 2020
8828c18
use the @kbn/std package
pgayvallet Sep 15, 2020
9997809
update generated doc
pgayvallet Sep 15, 2020
fe0fcc5
Merge remote-tracking branch 'upstream/master' into kbn-76003-kbn-con…
pgayvallet Sep 15, 2020
c80dd02
adapt plugin service mock
pgayvallet Sep 15, 2020
c0abb98
Merge remote-tracking branch 'upstream/master' into kbn-76003-kbn-con…
pgayvallet Sep 15, 2020
b77c901
Merge remote-tracking branch 'upstream/master' into kbn-76003-kbn-con…
pgayvallet Sep 16, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@
"@hapi/wreck": "^15.0.2",
"@kbn/analytics": "1.0.0",
"@kbn/babel-preset": "1.0.0",
"@kbn/config": "1.0.0",
"@kbn/config-schema": "1.0.0",
"@kbn/i18n": "1.0.0",
"@kbn/interpreter": "1.0.0",
Expand Down
9 changes: 9 additions & 0 deletions packages/kbn-config/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# `@kbn/config` — Kibana configuration file loader

`@kbn/config-schema` is a TypeScript library inspired by Joi and designed to allow run-time validation of the
Kibana configuration entries providing developers with a fully typed model of the validated data.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to add proper readme once this is finished.

Copy link
Contributor

Choose a reason for hiding this comment

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

This reference to @kbn/config-schema is rather confusing. Isn't it? Let's remove it


## Table of Contents

- [Why `@kbn/config-schema`?](#why-kbnconfig-schema)

28 changes: 28 additions & 0 deletions packages/kbn-config/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"name": "@kbn/config",
"main": "./target/out/index.js",
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
"types": "./target/types/index.d.ts",
"version": "1.0.0",
"license": "Apache-2.0",
"private": true,
"scripts": {
"build": "tsc",
"kbn:bootstrap": "yarn build"
},
"dependencies": {
"@kbn/config-schema": "1.0.0",
"@kbn/dev-utils": "1.0.0",
"rxjs": "^6.5.5",
"js-yaml": "3.13.1"
},
"devDependencies": {
"typescript": "4.0.2",
"tsd": "^0.7.4"
},
"peerDependencies": {
"lodash": "^4.17.15",
"@elastic/safer-lodash-set": "0.0.0",
"moment": "^2.24.0",
"type-detect": "^4.0.8"
}
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export type ConfigPath = string | string[];
/**
* Checks whether specified value can be considered as config path.
* @param value Value to check.
* @internal
* @public
*/
export function isConfigPath(value: unknown): value is ConfigPath {
if (!value) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

export const mockPackage = new Proxy({ raw: {} as any }, { get: (obj, prop) => obj.raw[prop] });
jest.mock('../../../../package.json', () => mockPackage);
jest.mock('../../../package.json', () => mockPackage);

export const mockApplyDeprecations = jest.fn((config, deprecations, log) => config);
jest.mock('./deprecation/apply_deprecations', () => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,22 @@ import { BehaviorSubject, Observable } from 'rxjs';
import { first, take } from 'rxjs/operators';

import { mockPackage, mockApplyDeprecations } from './config_service.test.mocks';
import { rawConfigServiceMock } from './raw_config_service.mock';
import { rawConfigServiceMock } from './raw/raw_config_service.mock';

import { schema } from '@kbn/config-schema';

import { ConfigService, Env } from '.';
import { loggingSystemMock } from '../logging/logging_system.mock';

import { getEnvOptions } from './__mocks__/env';

const emptyArgv = getEnvOptions();
const defaultEnv = new Env('/kibana', emptyArgv);
const logger = loggingSystemMock.create();

// TODO: fix
// import { loggingSystemMock } from '../logging/logging_system.mock';
// const logger = loggingSystemMock.create();
const loggingSystemMock: any = {};
const logger: any = {};

const getRawConfigProvider = (rawConfig: Record<string, any>) =>
rawConfigServiceMock.create({ rawConfig });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,16 @@ import { BehaviorSubject, combineLatest, Observable } from 'rxjs';
import { distinctUntilChanged, first, map, shareReplay, take } from 'rxjs/operators';

import { Config, ConfigPath, Env } from '.';
import { Logger, LoggerFactory } from '../logging';
import { Logger, LoggerFactory } from './logging';
import { hasConfigPathIntersection } from './config';
import { RawConfigurationProvider } from './raw_config_service';
import { RawConfigurationProvider } from './raw/raw_config_service';
import {
applyDeprecations,
ConfigDeprecationWithContext,
ConfigDeprecationProvider,
configDeprecationFactory,
} from './deprecation';
import { LegacyObjectToConfigAdapter } from '../legacy/config';
import { LegacyObjectToConfigAdapter } from './legacy';

/** @internal */
export type IConfigService = PublicMethodsOf<ConfigService>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

import { set } from '@elastic/safer-lodash-set';
import { get } from 'lodash';
import { unset } from '../utils';
import { ConfigDeprecation, ConfigDeprecationLogger, ConfigDeprecationFactory } from './types';
import { unset } from '../../../utils';

const _rename = (
config: Record<string, any>,
Expand Down
28 changes: 28 additions & 0 deletions packages/kbn-config/src/deprecation/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* 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.
*/

export {
ConfigDeprecation,
ConfigDeprecationWithContext,
ConfigDeprecationLogger,
ConfigDeprecationFactory,
ConfigDeprecationProvider,
} from './types';
export { configDeprecationFactory } from './deprecation_factory';
export { applyDeprecations } from './apply_deprecations';
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@ jest.mock('path', () => ({
}));

export const mockPackage = new Proxy({ raw: {} as any }, { get: (obj, prop) => obj.raw[prop] });
jest.mock('../../../../package.json', () => mockPackage);
jest.mock('../../../package.json', () => mockPackage);
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { PackageInfo, EnvironmentMode } from './types';

// `require` is necessary for this to work inside x-pack code as well
// eslint-disable-next-line @typescript-eslint/no-var-requires
const pkg = require('../../../../package.json');
const pkg = require('../../../package.json');
Copy link
Contributor Author

@pgayvallet pgayvallet Sep 7, 2020

Choose a reason for hiding this comment

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

Now that this is in a package, these hardcoded requires won't work anymore, as the compiled file in node_modules it not at the same place, or depth, than in the source. I think we don't have any other option than using the REPO_ROOT constant from @kbn/dev-utils (same lower in file)

Note that as REPO_ROOT is an absolute path, we won't be able to require it and we'll have to fallback to read and parse the file 'manually'.

The mocking (see packages/kbn-config/src/env.test.mocks.ts) will have to be adapted too.

EDIT: done. Fun fact, tests that are mocking the fs package are now exploding because REPO_ROOT is computed at module load.

while (true) {
if (isKibanaDir(cursor)) {
break;
}
const parent = Path.dirname(cursor);
if (parent === rootDir) {
throw new Error(`unable to find kibana directory from ${startDir}`);
}
cursor = parent;
}
export const REPO_ROOT = cursor;

It's quite bad because it's not only impacting core tests, some xpack tests are also failing because of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Commented on this here: #76874 (comment)

Copy link
Contributor

@tylersmalley tylersmalley Sep 9, 2020

Choose a reason for hiding this comment

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

#76518 is almost ready for review including resolving the tests mocking fs by being explicit about the actual files they wish to mock -


/** @internal */
export interface EnvOptions {
Expand Down Expand Up @@ -55,7 +55,7 @@ export class Env {
* @internal
*/
public static createDefault(options: EnvOptions): Env {
const repoRoot = dirname(require.resolve('../../../../package.json'));
const repoRoot = dirname(require.resolve('../../../package.json'));
return new Env(repoRoot, options);
}

Expand Down
43 changes: 43 additions & 0 deletions packages/kbn-config/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* 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.
*/

export {
applyDeprecations,
ConfigDeprecation,
ConfigDeprecationFactory,
configDeprecationFactory,
ConfigDeprecationLogger,
ConfigDeprecationProvider,
ConfigDeprecationWithContext,
} from './deprecation';

export { RawConfigurationProvider, RawConfigService } from './raw';

export { ConfigService, IConfigService } from './config_service';
export { Config, ConfigPath, isConfigPath, hasConfigPathIntersection } from './config';
export { ObjectToConfigAdapter } from './object_to_config_adapter';
export { CliArgs, Env } from './env';
export { EnvironmentMode, PackageInfo } from './types';
export { LegacyObjectToConfigAdapter } from './legacy';

// mocks
export { configMock } from './config.mock';
export { configServiceMock } from './config_service.mock';
export { rawConfigServiceMock } from './raw/raw_config_service.mock';
export { getEnvOptions } from './__mocks__/env';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As these mocks are used in core, I had to expose them on the package's entrypoint. This feels wrong, but I think creating an additional @kbn/config-mocks package is overkill, wdyt?

I couldn't find a way to have something like import { rawConfigServiceMock } from '@kbn/config/mocks', as I was forced to do import { rawConfigServiceMock } from '@kbn/config/target/out/mocks'. Maybe someone has more knowledge than me on npm packages?

Copy link
Contributor

@mshustov mshustov Sep 8, 2020

Choose a reason for hiding this comment

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

Our nodejs version doesn't support exports https://nodejs.org/docs/latest-v12.x/api/esm.html#esm_main_entry_point_export
We could emit *.js files next to *.ts and ignore them in .gitignore.
@elastic/kibana-operations any other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we build in place and can't produce the JS at the root of the module, I don't see any other alternative. Sounds like the best thing right now would be to use target in the import until we upgrade to v12.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions packages/kbn-config/src/legacy/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* 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.
*/

export { LegacyObjectToConfigAdapter } from './legacy_object_to_config_adapter';
Loading