Remove core->cli dependency #95145

pgayvallet · 2021-03-23T10:48:33Z

Summary

Remove the core->cli circular dependency by having the src/cli/serve script starting the CliDevMode instead of delegating that job to a 'stub' core's Server instance. See #76935 (comment) for more details on the issue and the chosen approach.

Unwiring and moving all the required parts to achieve that was a little tricky, but to summarize, I had to:

Create the @kbn/http-tools package

The BasePathProxyServer was using some utility functions from core (createServer, getServerOptions, getListenerOptions and so on), that are also used by core's HttpServer. As we do want to keep that code factorized, in case we change our underlying http lib at some point, I created this package instead of just duplicating the code we wanted (which is just the correct practice, overall)

Create the @kbn/crypto package

This package contains what was previously in src/core/server/utils/crypto, as it was used by the code we extracted to @kbn/http-tools. I could have just put the crypto utils in http-tools, but as these utils were not exclusively used by http-related stuff, I thought it made more sense to have our own package. Note that I initially wanted to put that in @kbn/utils, but some tests of the crypto extracted code relies on @kbn/dev-utils, so that would have created a circular dev-utils->utils->dev-utils dependency. I still think having our own crypto dedicated package make sense anyway.

Create the @kbn/dev-cli-mode package

This package contains what was previously under src/dev/cli_dev_mode. All this code is now self-contained into a package, making sure we'll not be adding unwanted dependencies to core later.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

kibanamachine · 2021-03-24T08:37:25Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 54ee08a
Storybooks not built yet
This comment will update when the build is complete

Failed CI Steps

History

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

kibanamachine · 2021-03-24T09:16:13Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 6a64c1c
Storybooks not built yet
This comment will update when the build is complete

Failed CI Steps

Check Types

History

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

…-cli-dev-mode

packages/kbn-crypto/tsconfig.json

packages/kbn-crypto/package.json

packages/kbn-http-tools/src/get_server_options.ts

packages/kbn-http-tools/src/get_request_id.ts

mshustov · 2021-03-26T13:49:03Z

src/core/server/http/integration_tests/router.test.ts

+describe('Timeouts', () => {
+  let httpServer: HttpServer;
+  let router: Router;
+
+  beforeEach(() => {
+    httpServer = new HttpServer(logger, 'foo');
+    const enhanceWithContext = (fn: (...args: any[]) => any) => fn.bind(null, {});
+    router = new Router('', logger.get(), enhanceWithContext);


Shouldn't it be placed in src/core/server/http/http_server.test.ts? We can move the whole file into integration_tests later

src/cli/serve/serve.js

mshustov · 2021-03-26T13:56:55Z

packages/kbn-cli-dev-mode/src/config/dev_config.ts

+  public basePathProxyTargetPort: number;
+
+  constructor(rawConfig: DevConfigType) {
+    this.basePathProxyTargetPort = rawConfig.basePathProxyTarget;


Do we really need a wrapper for a boolean flag?

mshustov · 2021-03-26T13:59:06Z

packages/kbn-cli-dev-mode/src/cli_dev_mode.ts

+      pluginPaths: config.plugins.additionalPluginPaths,
+      pluginScanDirs: config.plugins.pluginSearchPaths,


we can remove getPluginSearchPaths usage in @kbn/optimizer then?

src/cli/serve/serve.js

mshustov · 2021-03-26T14:23:46Z

src/cli/serve/serve.js

+    };
+    const bootstrapScript = getBootstrapScript(cliArgs.dev);
+
+    await bootstrapScript({


@spalger What is the main reason we want dev_cli controlling the Kibana server creation? It always confuses me when debugging the Kibana server with Chrome

kibana/packages/kbn-cli-dev-mode/src/using_server_process.ts

Line 32 in 77e5625

...(ACTIVE_INSPECT_FLAG ? [`${ACTIVE_INSPECT_FLAG}=${process.debugPort + 1}`] : []),

The main reason is because the dev cli implements auto restart on changes and ensuring that we don't see failed requests while it's restarting, and so that there's a single command to run that sets that and the optimizer up.

If you instead run node scripts/kibana without --dev, or instead of using yarn start, you can run the server without the dev cli wrapper that makes dev a lot easier in most cases.

…-cli-dev-mode

packages/kbn-cli-dev-mode/package.json

spalger

LGTM

…-cli-dev-mode

This reverts commit e46a74f

…-cli-dev-mode

kibanamachine · 2021-03-30T11:05:00Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #116052 failed 28ef9e8
💔 Build #115965 failed f47fc07
💔 Build #115930 failed 19a018e
💚 Build #115838 succeeded dabd6eb
💔 Build #115732 failed 107bb39

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

* extract http_tools to package * fix readme * start moving stuff * cleaning up `isDevCliParent` * choose bootstrap script * fix bootstrap script logic * fix watch paths logic * import REPO_ROOT from correct package * create the @kbn/crypto package * update core's `dev` config * only export bootstrap function * extract sslConfig to http-tools package * fix core types * fix optimizer tests * fix cli_dev_mode tests * fix basePath proxy tests * update generated doc * fix unit tests * create @kbn/dev-cli-mode package * remove useless comment * self-review NITS * update CODEOWNERS file * add devOnly flag * use variable for DEV_MODE_PATH * review comments * fix logger/log adapter * fix log calls in base path proxy server * address some review comments * rename @kbn/http-tools to @kbn/server-http-tools * more review comments * move test to correct file * add comment on getBootstrapScript * fix lint * lint * add cli-dev-mode to eslint dev packages * review comments * update yarn.lock * Revert "[ci] skip building ts refs when not necessary (elastic#95739)" This reverts commit e46a74f # Conflicts: # .github/CODEOWNERS

* extract http_tools to package * fix readme * start moving stuff * cleaning up `isDevCliParent` * choose bootstrap script * fix bootstrap script logic * fix watch paths logic * import REPO_ROOT from correct package * create the @kbn/crypto package * update core's `dev` config * only export bootstrap function * extract sslConfig to http-tools package * fix core types * fix optimizer tests * fix cli_dev_mode tests * fix basePath proxy tests * update generated doc * fix unit tests * create @kbn/dev-cli-mode package * remove useless comment * self-review NITS * update CODEOWNERS file * add devOnly flag * use variable for DEV_MODE_PATH * review comments * fix logger/log adapter * fix log calls in base path proxy server * address some review comments * rename @kbn/http-tools to @kbn/server-http-tools * more review comments * move test to correct file * add comment on getBootstrapScript * fix lint * lint * add cli-dev-mode to eslint dev packages * review comments * update yarn.lock * Revert "[ci] skip building ts refs when not necessary (#95739)" This reverts commit e46a74f # Conflicts: # .github/CODEOWNERS

pgayvallet added 2 commits March 23, 2021 11:47

extract http_tools to package

7fdc347

fix readme

3c7ca94

pgayvallet mentioned this pull request Mar 23, 2021

Remove circular dependencies between CLI & core #76935

Closed

pgayvallet added 8 commits March 23, 2021 16:41

start moving stuff

a617ef8

cleaning up isDevCliParent

10dcd75

choose bootstrap script

023c6af

fix bootstrap script logic

38ab1af

fix watch paths logic

fb098c0

import REPO_ROOT from correct package

0967c20

create the @kbn/crypto package

54ee08a

update core's dev config

1ef4ddd

pgayvallet added 2 commits March 24, 2021 09:56

only export bootstrap function

d2e1ba0

extract sslConfig to http-tools package

6a64c1c

pgayvallet added 6 commits March 24, 2021 10:45

fix core types

790b74a

fix optimizer tests

3ddd3e7

fix cli_dev_mode tests

a25f37c

fix basePath proxy tests

f4892b4

update generated doc

783e4f7

fix unit tests

a7a0b1f

pgayvallet added chore release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Mar 24, 2021

pgayvallet added 6 commits March 25, 2021 08:04

Merge remote-tracking branch 'upstream/master' into kbn-76935-extract…

f19dfa1

…-cli-dev-mode

create @kbn/dev-cli-mode package

77e5625

remove useless comment

6908053

self-review NITS

587d275

Merge remote-tracking branch 'upstream/master' into kbn-76935-extract…

47db89a

…-cli-dev-mode

update CODEOWNERS file

3092af1

pgayvallet added 3 commits March 26, 2021 08:46

review comments

eafa84c

fix logger/log adapter

83943d1

fix log calls in base path proxy server

bd2a5a8

mshustov approved these changes Mar 26, 2021

View reviewed changes

mshustov reviewed Mar 26, 2021

View reviewed changes

pgayvallet added 9 commits March 29, 2021 10:02

Merge remote-tracking branch 'upstream/master' into kbn-76935-extract…

63f059a

…-cli-dev-mode

address some review comments

b14bdcc

rename @kbn/http-tools to @kbn/server-http-tools

78d0d42

more review comments

70dadaa

move test to correct file

08ed6ea

add comment on getBootstrapScript

fd26fba

fix lint

107bb39

lint

324d2d8

add cli-dev-mode to eslint dev packages

dabd6eb

spalger reviewed Mar 29, 2021

View reviewed changes

packages/kbn-cli-dev-mode/package.json Outdated Show resolved Hide resolved

packages/kbn-cli-dev-mode/package.json Outdated Show resolved Hide resolved

review comments

19a018e

spalger approved these changes Mar 29, 2021

View reviewed changes

pgayvallet added 5 commits March 29, 2021 23:59

Merge remote-tracking branch 'upstream/master' into kbn-76935-extract…

e2ab226

…-cli-dev-mode

update yarn.lock

f47fc07

Merge remote-tracking branch 'upstream/master' into kbn-76935-extract…

28ef9e8

…-cli-dev-mode

Revert "[ci] skip building ts refs when not necessary (elastic#95739)"

b007289

This reverts commit e46a74f

Merge remote-tracking branch 'upstream/master' into kbn-76935-extract…

a123e34

…-cli-dev-mode

pgayvallet merged commit 265cc76 into elastic:master Mar 30, 2021

pgayvallet mentioned this pull request Mar 30, 2021

[7.x] Remove core->cli dependency (#95145) #95775

Merged

pgayvallet mentioned this pull request Mar 30, 2021

[cli_dev_mode] use correct config prefix for server #95792

Merged

spalger mentioned this pull request Mar 30, 2021

[cli-dev-mode] complete shutdown once devServer stops gracefully #95822

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove core->cli dependency #95145

Remove core->cli dependency #95145

pgayvallet commented Mar 23, 2021 •

edited

Loading

kibanamachine commented Mar 24, 2021

kibanamachine commented Mar 24, 2021

mshustov Mar 26, 2021

mshustov Mar 26, 2021

mshustov Mar 26, 2021

mshustov Mar 26, 2021 •

edited

Loading

spalger Mar 27, 2021

spalger Mar 27, 2021 •

edited

Loading

spalger left a comment

kibanamachine commented Mar 30, 2021

		pluginPaths: config.plugins.additionalPluginPaths,
		pluginScanDirs: config.plugins.pluginSearchPaths,

Remove core->cli dependency #95145

Remove core->cli dependency #95145

Conversation

pgayvallet commented Mar 23, 2021 • edited Loading

Summary

Checklist

kibanamachine commented Mar 24, 2021

⏳ Build in-progress, with failures

Failed CI Steps

History

kibanamachine commented Mar 24, 2021

⏳ Build in-progress, with failures

Failed CI Steps

History

mshustov Mar 26, 2021

Choose a reason for hiding this comment

mshustov Mar 26, 2021

Choose a reason for hiding this comment

mshustov Mar 26, 2021

Choose a reason for hiding this comment

mshustov Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

spalger Mar 27, 2021

Choose a reason for hiding this comment

spalger Mar 27, 2021 • edited Loading

Choose a reason for hiding this comment

spalger left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 30, 2021

💚 Build Succeeded

Metrics [docs]

History

pgayvallet commented Mar 23, 2021 •

edited

Loading

mshustov Mar 26, 2021 •

edited

Loading

spalger Mar 27, 2021 •

edited

Loading