[New platform] Introduce start phase for core services on server #35297

mshustov · 2019-04-18T14:14:07Z

Summary

The main reason for pushing start phase on the server side is exposing an ability to register a router in New platform.
I added a full support for start method only for http, legacy & plugins services and changed their API accordingly.
Now HTTP service exposed

a setup contract as

interface Setup {
  server: Server;
  options: ServerOptions;
  registerRouter: (router: Router) => void;
  registerAuth: (authenticationHandler,  cookieOptions) => void;
  registerOnRequest: (requestHandler: OnRequestHandler) => void;
}

a start contract as

interface Start {
  isListening: () => boolean;
}

Legacy service stores passed setup contracts and create an instance of KbnServer on start
Other core services expose start method, but it is just noop.
Remaining core services can be updated in following PR:

elasticsearch service - TDB
~~plugin service - after Introduce start lifecycle event to client #35269~~

Checklist

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

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

mshustov · 2019-04-18T15:28:20Z

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


-        await root.setup();
+        const { http } = await root.setup();
+        http.registerRouter(router);


maybe it seems redundant to call both methods to run a test server. that was done to support such cases without introducing a lot of changes.

I believe we can easily to fix this later. For example, we can extend KbnTestServer in a way it creates root, declares all routes and after starts a real server.

src/core/public/plugins/plugins_service.ts

elasticmachine · 2019-04-18T15:54:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-18T16:42:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-19T19:41:38Z

Pinging @elastic/kibana-platform

src/core/server/legacy/legacy_service.ts

joshdover · 2019-04-23T21:48:17Z

src/core/server/http/http_server.ts

    const serverOptions = getServerOptions(config);
    this.server = createServer(serverOptions);

+    return {
+      options: serverOptions,


deepFreeze might be a good idea here

aah yeah. I wanted to add freeze here, but found that we have 2 implementations - one for js in x-pack/secuirty, one for ts in core/public/utils. I think it makes sense to reach an agreement how we going to distribute environment agnostic code snippets, case with deepFreeze is perfect example. Do we have such agreement? If not I will create a discussion

I do not think that we do have a guideline on sharing code other than things that live as @kbn/ packages. Could make sense to move this utility there, but it's so small, I'm not sure the boilerplate of a separate package is worth it.

turned out the correct implementation is not so simple microsoft/TypeScript#13923

for now I will copy this functionality, but we need to extract it in a separate package later

elasticmachine · 2019-04-24T07:51:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

to get rid of Optional<HttpServer> type and make it Require<HttpServer>

elasticmachine · 2019-04-25T09:02:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-25T10:34:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-25T10:57:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

joshdover

Looks really good, really just one small comment.

When you add this to the PluginsService in the follow up PR, do you also plan to introduce this to the plugin spec themselves at the same time? I'm asking because I've already added the start lifecycle event to UI plugins.

src/core/server/server.ts

joshdover · 2019-04-29T16:44:20Z

src/core/server/http/http_server.ts

    const serverOptions = getServerOptions(config);
    this.server = createServer(serverOptions);

+    return {
+      options: serverOptions,


I do not think that we do have a guideline on sharing code other than things that live as @kbn/ packages. Could make sense to move this utility there, but it's so small, I'm not sure the boilerplate of a separate package is worth it.

mshustov · 2019-04-29T17:57:29Z

When you add this to the PluginsService in the follow up PR, do you also plan to introduce this to the plugin spec themselves at the same time? I'm asking because I've already added the start lifecycle event to UI plugins.

agree, I will add it for parity.

…ent side" This reverts commit c04fdd2.

elasticmachine · 2019-04-30T10:53:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…stic#35297) * introduce start phase. setup is bloated with start functionality * fix amp typings: server is part of start contract now * update mock files * root.start(): necessary to run test server * expose setup&start server api to simplify testing * move tests to the new API * test servers also should call root.start() * update docs * update snapshots: this functionality is tested in http server * split setup/start phases * update docs * expose http server if it not started to get rid of Optional<HttpServer> type and make it Require<HttpServer> * adopt test to exposed Http server via SetupContract * udpate docs * cleanup apm changees * check legacy service setup before start * check http server setup before start * restrict server options mutation; unify Promise interface for setup * introduce start pahse for plugins service for parity with client side * Revert "introduce start pahse for plugins service for parity with client side" This reverts commit c04fdd2.

) (#35791) * introduce start phase. setup is bloated with start functionality * fix amp typings: server is part of start contract now * update mock files * root.start(): necessary to run test server * expose setup&start server api to simplify testing * move tests to the new API * test servers also should call root.start() * update docs * update snapshots: this functionality is tested in http server * split setup/start phases * update docs * expose http server if it not started to get rid of Optional<HttpServer> type and make it Require<HttpServer> * adopt test to exposed Http server via SetupContract * udpate docs * cleanup apm changees * check legacy service setup before start * check http server setup before start * restrict server options mutation; unify Promise interface for setup * introduce start pahse for plugins service for parity with client side * Revert "introduce start pahse for plugins service for parity with client side" This reverts commit c04fdd2.

mshustov added 7 commits April 18, 2019 16:21

introduce start phase. setup is bloated with start functionality

4eef5fb

fix amp typings: server is part of start contract now

f116a29

update mock files

faad244

root.start(): necessary to run test server

a037fe8

expose setup&start server api to simplify testing

25aa5a5

move tests to the new API

7ffd2aa

test servers also should call root.start()

fad9a06

mshustov force-pushed the introduce-start-phase branch from 85d85ae to fad9a06 Compare April 18, 2019 14:55

mshustov changed the title ~~Introduce start phase for core services on server~~ [New platform] Introduce start phase for core services on server Apr 18, 2019

mshustov added 2 commits April 18, 2019 17:29

update docs

cfddce2

update snapshots: this functionality is tested in http server

406ae80

mshustov commented Apr 18, 2019

View reviewed changes

mshustov marked this pull request as ready for review April 18, 2019 17:01

mshustov requested review from a team as code owners April 18, 2019 17:01

joshdover mentioned this pull request Apr 18, 2019

Introduce start lifecycle event to client #35269

Merged

mshustov added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v7.2.0 labels Apr 19, 2019

mshustov added the review label Apr 19, 2019

joshdover reviewed Apr 23, 2019

View reviewed changes

mshustov added 3 commits April 24, 2019 09:38

split setup/start phases

bfd7b0b

Merge branch 'master' into introduce-start-phase

38b7394

update docs

f504d00

expose http server if it not started

ea34a0e

to get rid of Optional<HttpServer> type and make it Require<HttpServer>

mshustov added 3 commits April 25, 2019 11:03

adopt test to exposed Http server via SetupContract

3d245f5

udpate docs

05e0333

cleanup apm changees

d46e7a8

mshustov requested review from joshdover and removed request for a team April 25, 2019 08:15

mshustov added 3 commits April 25, 2019 12:06

Merge branch 'master' into introduce-start-phase

62a6724

check legacy service setup before start

255d3c3

check http server setup before start

8880af1

mshustov mentioned this pull request Apr 29, 2019

Spaces - New Platform Migration, Step 1 #35429

Merged

11 tasks

joshdover approved these changes Apr 29, 2019

View reviewed changes

mshustov added 4 commits April 30, 2019 09:07

restrict server options mutation; unify Promise interface for setup

02ccc54

Merge branch 'master' into introduce-start-phase

f8cc8c8

introduce start pahse for plugins service for parity with client side

c04fdd2

Revert "introduce start pahse for plugins service for parity with cli…

37de718

…ent side" This reverts commit c04fdd2.

mshustov merged commit 39091e7 into elastic:master Apr 30, 2019

mshustov mentioned this pull request Apr 30, 2019

[7.x] [New platform] Introduce start phase for core services on server (#35297) #35791

Merged

mshustov removed the review label Apr 30, 2019

tylersmalley added the v8.0.0 label Apr 30, 2019

This was referenced May 3, 2019

[New platform] Support configuring request specific basePath #35951

Merged

[New platform] Security plugin integration #33775

Closed

Cleanup http server integration tests #36256

Closed

mshustov mentioned this pull request May 21, 2019

cleanup integration test #36766

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New platform] Introduce start phase for core services on server #35297

[New platform] Introduce start phase for core services on server #35297

mshustov commented Apr 18, 2019 •

edited

Loading

mshustov Apr 18, 2019

elasticmachine commented Apr 18, 2019

elasticmachine commented Apr 18, 2019

elasticmachine commented Apr 19, 2019

joshdover Apr 23, 2019

mshustov Apr 24, 2019

joshdover Apr 29, 2019

mshustov Apr 30, 2019

elasticmachine commented Apr 24, 2019

elasticmachine commented Apr 25, 2019

elasticmachine commented Apr 25, 2019

elasticmachine commented Apr 25, 2019

joshdover left a comment

joshdover Apr 29, 2019

mshustov commented Apr 29, 2019

elasticmachine commented Apr 30, 2019

[New platform] Introduce start phase for core services on server #35297

[New platform] Introduce start phase for core services on server #35297

Conversation

mshustov commented Apr 18, 2019 • edited Loading

Summary

Checklist

For maintainers

mshustov Apr 18, 2019

Choose a reason for hiding this comment

elasticmachine commented Apr 18, 2019

💔 Build Failed

elasticmachine commented Apr 18, 2019

💚 Build Succeeded

elasticmachine commented Apr 19, 2019

joshdover Apr 23, 2019

Choose a reason for hiding this comment

mshustov Apr 24, 2019

Choose a reason for hiding this comment

joshdover Apr 29, 2019

Choose a reason for hiding this comment

mshustov Apr 30, 2019

Choose a reason for hiding this comment

elasticmachine commented Apr 24, 2019

💚 Build Succeeded

elasticmachine commented Apr 25, 2019

💔 Build Failed

elasticmachine commented Apr 25, 2019

💚 Build Succeeded

elasticmachine commented Apr 25, 2019

💚 Build Succeeded

joshdover left a comment

Choose a reason for hiding this comment

joshdover Apr 29, 2019

Choose a reason for hiding this comment

mshustov commented Apr 29, 2019

elasticmachine commented Apr 30, 2019

💚 Build Succeeded

mshustov commented Apr 18, 2019 •

edited

Loading