[New Platform] Replace Express with Hapi 17. #18562

azasypkin · 2018-04-25T13:48:06Z

To reduce the risk of breaking something when Kibana starts relying on HTTP server from the new platform, we decided to use Hapi instead of Express in the new platform as well. This will also help us to save time on re-implementing Hapi server specific features for Express (eg. state.strictHeader or validate.options.abortEarly). This PR replaces all Express specific features with the ones provided by Hapi.

This is a temporary solution since we still intend to use Express eventually. For this reason we'll try not to use any Hapi plugins, Hapi own logging etc. in the new platform and it should allows us to use the latest possible version of Hapi.

How it works:

When Kibana creates a new "connection" for the Hapi server (HapiJS 14) it relies on custom HTTP listener provided and managed by the @kbn/platform. Root in @kbn/platform is only started when Kibana calls listen method on that listener. So any HTTP request is handled by @kbn/platform first and only forwarded to Kibana if there is no matching routes defined in the @kbn/platform itself.

In this PR I didn't touch BasePathProxy so it still solely relies on HapiJS 14 and the same server configuration part is presented in both setupConnection and @kbn/platform. As a follow-up I plan to move BasePathProxy to @kbn/platform and do all HTTP server configuration stuff in @kbn/platform only.

To test any new platform route, just follow the steps:

Build example plugins:

$ ./example_plugins/build.sh

Modify kibana.yml:

__newPlatform:
  plugins:
    scanDirs: ['./example_plugins']

Hit endpoint exposed by ./example_plugins/baz (add base path if needed):

$ curl localhost:5601/api/baz/fail

Scope:

Make sure that new platform process its own routes, and "old" Kibana processes the rest
Make sure that HTTP setup works
Make sure that HTTPS/TLS setup works
Make sure that setup with BasePathProxy works (including rewriteBasePath)
Check if it's possible to fully configure Hapi in the new platform and use bare minimum of config options for the always non-TLS Hapi in "old" platform (e.g. define payload.maxByte, state.strictHeader and TLS config only in new platform)

epixa · 2018-04-25T16:21:11Z

How will this work when running alongside Kibana, where hapi is still stuck on v14?

azasypkin · 2018-04-25T16:41:10Z

How will this work when running alongside Kibana, where hapi is still stuck on v14?

Basically in the same way it worked with Express before:

New platform creates Hapi 17 server that is exposed to the outside world
New platform creates a wrapper around Hapi 17 server that follows Listener interface
Old platform creates Hapi 14 server and uses the wrapper provided by new platform as server.listener, so it doesn't create NodeJS "native" server/listener
Old platform registers its routes with Hapi 14
New platform registers its routes with Hapi 17 (via abstracted API)
New platform adds additional catch-all route that will forward all requests that aren't handled by new platform to old platform in a "raw" request-response pair that is successfully consumed by Hapi 14

Do you see any flaws in this approach @epixa (except for non-zero overhead because of additional Hapi server) ?

epixa · 2018-04-25T16:44:53Z

That makes sense, but I thought the main reason we wanted to use hapi instead of express was so all the legacy global behaviors that were attached to hapi 14 like CORS and CSRF handling would surface through the new platform, but that doesn't seem to be covered here. It seems like if we want compatibility here, then we still need to implement all these global things in the new platform. At that point, I'm not sure why we're using hapi.

azasypkin · 2018-04-25T16:59:58Z

Well, correct me if I'm wrong here, but I thought the main reason was to start using new platform as soon as possible (aka not spending time to re-implement/workaround features that are supported and handled by Hapi in Express: cors, state.strictHeader, validate.options.abortEarly etc.) and reduce the risk of breaking something (it's impossible to eliminate that completely, but my assumption is that if we use Hapi instead of Express we can significantly reduce that risk even if it's not the same version).

If all works out as I imagine, with this setup, if cors is enabled in Hapi 17, we don't have to care about it in Hapi 14, if we don't handle XSRF in new platform, then new platform routes won't be protected, but old platform will still be protected by the code implemented in old platform and "attached" to Hapi 14. So once we decide to support XSRF protection in new platform, we'll get rid of it in the old one and everything will be handled in new platform.

azasypkin · 2018-05-02T09:41:30Z

package.json

@@ -101,7 +101,6 @@
    "babel-polyfill": "6.20.0",
    "babel-register": "6.18.0",
    "bluebird": "2.9.34",
-    "body-parser": "1.17.2",


note: removing all ExpressJS specific packages, we can easily return them back when needed.

Can you explain this more? If we are still using ExpressJS, why would we remove these?

Once this PR lands we'll be using only Hapi in new platform, hence we don't need any ExpressJS packages.

azasypkin · 2018-05-02T09:43:25Z

platform/src/lib/utils/url.ts

@@ -0,0 +1,75 @@
+import { parse as parseUrl, format as formatUrl, UrlObject } from 'url';


note: copied and TS-fied from https://github.com/elastic/kibana/blob/master/src/utils/modify_url.js

azasypkin · 2018-05-02T09:52:04Z

platform/src/server/http/http_config.ts

@@ -18,8 +18,16 @@ const createHttpSchema = object({
  port: number({
    defaultValue: 5601,
  }),
+  cors: oneOf([


note: it's not exactly replicating schema that is defined by old Kibana since validate method in @kbn/platform can't get access to env config value right now. But wrong values will be rejected by Joi and won't reach @kbn/platform so it shouldn't be a big problem right now.

We should probably create an issue or something to track adding this env support to the new platform, otherwise I could easily see us creating a regression here when we kill off the old platform. What do you think?

Yeah, if we really want to have functionality similar to Joi (references to any config value during validation and default value initialization) we can build it, although it won't be a trivial task since we'll have to parse and validate config in the right order.

Will be handled separately based on decision made in #18818

azasypkin · 2018-05-02T09:53:48Z

platform/src/server/http/http_server.ts

    }

+    this.server.listener.on('clientError', (err, socket) => {


note: just copied this piece from old Kibana, haven't thought yet if it can be improved :)

azasypkin · 2018-05-02T09:59:28Z

src/server/http/index.js

 import { setupXsrf } from './xsrf';

 export default async function (kbnServer, server, config) {
  server = kbnServer.server = new Hapi.Server();

  const shortUrlLookup = shortUrlLookupProvider(server);

-  setupConnection(server, config, kbnServer.newPlatform);
-  setupBasePathRewrite(server, config);
+  server.connection({


note: the rest of properties is configured in @kbn/platform that's the bare minimum we should define here.

azasypkin · 2018-05-02T09:59:56Z

src/server/http/setup_connection.js

@@ -1,9 +1,7 @@
 import { readFileSync } from 'fs';


note: now this function is only used by BasePathProxy.

Is it possible for the BasePathProxy to share connection setup logic with the new platform so we don't loose the shared setup? I think it's important that the default dev server use as close to a "production" setup as possible and keeping this and the new platform setup logic in sync doesn't feel like intuitive or likely to happen,

I think it's important that the default dev server use as close to a "production" setup as possible and keeping this and the new platform setup logic in sync doesn't feel like intuitive or likely to happen,

Wholeheartedly agree, I just wanted to handle this in the follow-up, but let me check how hard it is to handle this here.

So I started moving BasePathProxy to new platform and it's getting too big to fit into this PR, does it work for you if I create a follow-up right after this one @spalger ? The rest of the comments should be handled now.

Will handle basepath proxy in a separate PR #19424

azasypkin · 2018-05-02T10:04:44Z

Hey @spalger and @archanid,

Would you mind giving a preliminary feedback on the approach used here (see details in #18562 (comment))?

I didn't write/update tests yet, but will do if you're fine with this approach and don't see any issues I might have missed.

azasypkin · 2018-05-02T10:05:46Z

platform/src/server/http/http_server.ts

-      this.app.use((req, res) =>
-        legacyKbnServer.newPlatformProxyListener.proxy(req, res)
-      );
+      this.server.route({


note: based on https://hapijs.com/api#catch-all-route

Should we say legacyKbnServer in the comment instead of kbnServer?

Yeah, ✔️

epixa · 2018-05-02T12:41:01Z

platform/src/lib/utils/url.ts

+ *    - auth
+ *    - hostname (just the name of the host, no port or auth information)
+ *    - port
+ *    - pathmame (the path after the hostname, no query or hash, starts


Typo, should be pathname

epixa · 2018-05-02T12:43:38Z

platform/src/lib/utils/url.ts

+ *      properties on the "parsed" output. Modifying any of these might
+ *      lead to the modifications being ignored (depending on which
+ *      property was modified)
+ *    - It's not always clear wither to use path/pathname, host/hostname,


Typo, should be whether

epixa · 2018-05-02T12:51:24Z

platform/src/server/http/http_server.ts

+          method: route.method,
+          path: `${router.path}${route.path}`,
+          handler: route.handler,
+        });


There's a gap in features here between new platform and old. I'm not sure if that's a problem we need to address in this PR, but I wanted to raise it. We currently use hapi's options hash to specify things like validation, auth, and tags. How do you suppose we should handle these things in the new platform going forward?

Yeah, there is a gap, but I'd define first what we want to support in new platform and why we can't live without it :) Some of the Hapi "features" are just handled differently in new platform. E.g.:

Validation - we have some support for this already, see https://github.com/elastic/kibana/blob/new-platform/example_plugins/baz/src/register_endpoints.ts#L33-L48. Whether it's sufficient or not is a different story.

Auth - we didn't discuss this a lot, but it should be solely handled by security plugin. It will also expose API that other plugins can consume (e.g. to get current user, or mark the route as one that doesn't require authentication or something like this)

Tags - why do we need to support this in new platform? I can see that route metadata can be helpful (e.g. to have self-documented routes), but can't think of anything critical that can't be implemented in the next iteration.

We do use route tags at the moment. Whether or not we want to expose that same concept in the platform is probably a separate discussion.

We currently use route tags to flag whether an endpoint should respond with a redirect to login or a 403. I'm not sure if this is even necessary at this point, but it's functionality that currently exists.

Yeah, it seems to be used only in security plugin, we'll figure that out in the scope of #18024

epixa · 2018-05-02T12:53:32Z

platform/src/server/http/http_server.ts

+        },
+        handler: ({ raw: { req, res }}, h) => {
+          legacyKbnServer.newPlatformProxyListener.proxy(req, res);
+          return h.abandon;


nit: One letter variable names make it more difficult to understand the code. I think short variable names for super common things like req, res, and err are fine, but it would be helpful if we used more expressive names otherwise.

Yeah, agree, used the convention established in Hapi docs by default, but will rename it into something that's clearer.

epixa · 2018-05-02T12:56:42Z

platform/src/server/http/http_server.ts

+        }
+      },
+      state: {
+        strictHeader: false


I thought we had strictHeader enabled on the old server. Did you check that?

Here is what we have in master:

kibana/src/server/http/setup_connection.js

Lines 11 to 13 in 97df91f

state: {

strictHeader: false

},

- strictHeader: false.

epixa · 2018-05-02T13:49:44Z

platform/src/server/http/http_server.ts

+
+    const ssl = config.ssl;
+    if (ssl.enabled) {
+      const tlsOptions: TLSOptions = {


@elastic/kibana-security Just a heads up: There will soon be an additional place to deal with ssl stuff.

epixa · 2018-05-02T16:56:34Z

platform/src/server/http/ssl_config.ts

+  /**
+   * Options that affect the OpenSSL protocol behavior via numeric bitmask of the SSL_OP_* options from OpenSSL Options.
+   */
+  getSecureOptions() {


This seems pretty different from the express stuff, and it's not clear what functionality from express it is replacing. Can you explain a bit what this is doing? Or maybe just point out in this diff where the corresponding express stuff is being removed so I can research on my own?

This seems pretty different from the express stuff

What express stuff do you mean? We never supported secureOptions in new platform, so it's almost a copy-paste of what we have in master.

That's perfect. I was mostly just trying to figure out why you added this code in the first place.

rhoboat · 2018-05-02T20:04:31Z

platform/src/server/http/http_server.ts

+      }
+
+      request.setUrl(newURL);
+      // Raw request can be forwarded to the old platform that doesn't expect base path


This comment is confusing. Should this be actually two sentences? I feel a split between "platform" and "that".

Yeah, agree, will something like this be better: We should update raw request as well since it can be proxied to the old platform where base path isn't expected.?

✔️ (changed to We should update raw request as well since it can be proxied to the old platform where base path isn't expected., feel free to propose a better alternative - I'll fix that.)

rhoboat · 2018-05-02T20:11:24Z

@azasypkin What kinds of tests would help you have confidence in this change? What about the whole http server in general? With that group of tests, I can better prepare the functional/integration tests that need to be written. I suspect testing cors will be a new one to add. We do have a simple test for xsrf currently.

azasypkin · 2018-05-03T09:31:51Z

@azasypkin What kinds of tests would help you have confidence in this change? What about the whole http server in general? With that group of tests, I can better prepare the functional/integration tests that need to be written. I suspect testing cors will be a new one to add. We do have a simple test for xsrf currently.

The tests that involve rewriteBasePath and TLS config will definitely make me feel more confident. Tests for CORS are also nice to have, but I'm not really worried about that at this point: CORS support in prod is really limited and easy to verify manually. Before we merge @kbn/platform we also need to expose at least one endpoint there and write integration test for it to make sure we can call both new and old platform routes (it's really to break this in many different ways if we don't test it properly). Apart from that, I think we may want to test how we deal with common failure scenarios (400, 401, 403, 404).

rhoboat · 2018-05-10T15:05:27Z

platform/src/server/http/http_config.ts

+}, {
+  validate: config => {
+    if (!config.basePath && config.rewriteBasePath) {
+      return 'can not use [rewriteBasePath] when [basePath] is not specified';


typo: cannot is a single word

rhoboat · 2018-05-10T15:06:27Z

platform/src/server/http/http_server.ts

-    this.app.use(router.path, router.router);
+    if (this.isListening()) {
+      throw new Error('Routers can be registered only when HTTP server is stopped.')
+    }


rhoboat · 2018-05-10T15:15:00Z

platform/src/server/http/http_server.ts

+
+      // TODO: Hapi types have a typo in `tls` property type definition: `https.RequestOptions` is used instead of
+      // `https.ServerOptions`, and `honorCipherOrder` isn't presented in `https.RequestOptions`.
+      options.tls = tlsOptions as any;


🤦‍♀️ That's too bad.

Yeah, I'll submit PR to DefinitelyTyped later, we've submitted a bunch of patches to DT already as the result of type-augmentation-cleanup in new platform and kbn/pm. It usually takes a couple of days for the PR to be reviewed and merged.

rhoboat · 2018-05-10T15:25:17Z

platform/src/server/http/ssl_config.ts

+        arrayOf(string()),
+        string(),
+      ])
+    ),
    supportedProtocols: maybe(
      arrayOf(oneOf([literal('TLSv1'), literal('TLSv1.1'), literal('TLSv1.2')]))
    ),


Just wondering, what about the redirectHttpFromPort setting? For context:

Kibana will bind to this port and redirect all http requests to https over the port configured as server.port.

Hmm, good point! I missed that - will see how I can handle that in new platform.

rhoboat · 2018-05-10T15:28:52Z

platform/src/server/http/ssl_config.ts

+      // so if protocol is supported, we should not enable this option.
+      return supportedProtocols.includes(protocolAlias)
+        ? secureOptions
+        : secureOptions | secureOption;


Do we know what this line actually does in a practical sense? Just wondering. We don't see the bitwise or very often.

Just sets the proper bit in bitmask for SSL context options used by OpenSSL? Or what do you mean?

rhoboat · 2018-05-10T15:32:23Z

src/server/http/index.js

 import { setupRedirectServer } from './setup_redirect_server';
 import { registerHapiPlugins } from './register_hapi_plugins';
-import { setupBasePathRewrite } from './setup_base_path_rewrite';
 import { setupXsrf } from './xsrf';

 export default async function (kbnServer, server, config) {
  server = kbnServer.server = new Hapi.Server();


optional nit: While we're here, I just wonder, can we remove the chaining equals in favor of clearer code? Like

kbnServer.server = new Hapi.Server(); server = kbnServer.server;

Sure, will change that.

azasypkin · 2018-05-14T17:49:14Z

platform/src/server/http/http_server.ts

    );
  }
+
+  private async _setupRedirectServer(config: HttpConfig) {


note: now when added it here, I'm thinking maybe I should have http_redirect_server.ts instead that is managed by http_service as it seems to be independent enough from http_server itself?

I'll handle this in the follow-up then.

azasypkin · 2018-05-15T06:34:34Z

@archanid @spalger PR should be ready for another pass.

rhoboat · 2018-05-17T19:45:29Z

@azasypkin What changed since the last time I reviewed it? It looks like the commits were rebased. Do you kind of remember what specifically changed? I see the redirect server added.

azasypkin · 2018-05-17T20:04:40Z

@azasypkin What changed since the last time I reviewed it? It looks like the commits were rebased. Do you kind of remember what specifically changed? I see the redirect server added.

Yeah, the new changes are two last commits.

rhoboat · 2018-05-17T20:17:45Z

Ah, I see! Thanks. Now I see the commits. I think I needed to refresh my tab.

spalger

I haven't run this yet but wanted to flag a couple things

spalger · 2018-05-21T18:40:40Z

platform/src/server/http/http_server.ts


 export class HttpServer {
-  private readonly app: express.Application;
-  private server?: Server;
+  private _server?: Server;


Is there a reason we're using _server with private? I suspect it's to inform people with access to this instance who aren't using TypeScript, but we shouldn't be exposing this instance to non-internal code right? I would prefer that we have a separate public contract from the internal server instance.

The were two reasons:

If we decide to transpile code to JavaScript once again, we'll have to prefix all private variables manually (that was one of the pain point when transpiled new platform to JS), so I thought it'd make sense to keep code in a "close-to-JS" state even if we use TS;

Same for JSDoc - previously we decided not to mention types in JSDoc for TypeScript, but once I transpiled code to JS all type information disappeared and having that info at least in JSDoc would have helped a lot. So I started to write JSDoc as if it was in JS...

IIRC Kim and @archanid voted for "_"-prefixed TypeScript private members in the past, so it became informatl new-platform-team-agreement but I can't remember whether it was in slack or in some PR :/

We can easily change team-wide agreement if we want, I'm somewhat concerned about the first point, what do you think?

re 1: We're going forward with TypeScript now and I'm buying in fully, but if I need to write a script that rewrites all private properties to be _ prefixed in the future I'm down with that.

re 2: 🤷‍♂️ we have more formally agreed to try and follow tslint:recommended preset in #19105 and that preset explicitly forbids underscore prefixed variables and properties.

I'm buying in fully, but if I need to write a script that rewrites all private properties to be _ prefixed in the future I'm down with that.

Now we're talking, we such level of confidence in TypeScript, I'm fine with that too :) What about types in JSDoc by the way?

that preset explicitly forbids underscore prefixed variables and properties.

Oh, interesting, didn't know that.

What about types in JSDoc by the way?

That's a harder one, but since it's not checked by the compiler I don't think we should be putting them in there. Maybe that means we should enable https://palantir.github.io/tslint/rules/no-redundant-jsdoc/ at some point, or maybe that means we should just let JSDoc continue to be a suggestion that may give insight into the authors thought process while also being entirely incorrect at times.

I'm not confident in my ability to automatically create JSDoc comments based on the TS types, but I imagine it's possible to some extent. If we get to a future where we need to rip out TypeScript we should look into that more closely. Otherwise I don't think we're any worse off than we are today if we loose all the type information and don't have type info in the JSDoc comments.

while also being entirely incorrect at times.

I can't agree here, such JSDoc just shouldn't pass review, it's extremely useful to have correct type information even if it's not enforced by compiler/builder/whatever especially with the code base of the Kibana's size and complexity :)

Otherwise I don't think we're any worse off than we are today if we loose all the type information and don't have type info in the JSDoc comments.

Well it will be worse for the "original JS -> TS -> transpiled JS" case if original JS currently uses JSDoc with proper types and we remove them while converting to TS. I also think there are a bunch of tools that can convert proper JSDoc types to say Flow annotations or something like this if we change our mind at some point.

Okay, it sounds like a separate discussion that deserves its own issue/PR, I'll probably keep adding type information in JSDoc where it's reasonable for the time being and take care of it if we decide to enable no-redundant-jsdoc.

while also being entirely incorrect at times.

I can't agree here, such JSDoc just shouldn't pass review,

Yeah, but humans 🤓 I agree though, we should have this discussion with a broader audience

Haha, I just resort to idealism sometimes when I need to prove my point, that's true :)

spalger · 2018-05-21T18:44:23Z

platform/src/server/http/http_server.ts

+      await this._redirectServer.start();
+    } catch (err) {
+      if (err.code === 'EADDRINUSE') {
+        console.log(err);


Is this console.log() intended?

Likely not, thanks 🤦‍♂️

spalger · 2018-05-21T18:51:15Z

platform/src/server/http/router/router.ts

-      response.status(400);
-      response.json({ error: e.message });
-      return;
+      return responseToolkit.response({ error: e.message }).code(400);
    }

    try {
      const kibanaResponse = await handler(kibanaRequest, responseFactory);

      if (kibanaResponse instanceof KibanaResponse) {


I think we should throw an error if the handler doesn't return a KibanaResponse or, perhaps, undefined. Instead we are allowing handlers to return anything that responseToolkit.response() supports, which is not defined here and as far as I know supports everything from Stream instances to plain strings, numbers or objects. Not limiting the supported return values to a very specific list of things opens up the "supported" API too much for my taste.

Even though return type is limited by KibanaResponse<any> | { [key: string]: any }, I agree, it can be a lot of different things. I think it was initially designed to short-circuit {...} ---> HTTP 200/JSON path. I'll limit response to KibanaResponse only and we'll expand it if we need to in the future.

KibanaResponse will still allow { [key: string]: any } payload, but we can make it more sophisticated and strict in the scope of #12464. Does that work for you?

Yeah, as long as KibanaResponse is the one defining the API I'm good with it.

spalger · 2018-05-21T18:53:32Z

platform/src/server/http/router/router.ts

-          response.json({ error: kibanaResponse.payload.message });
-        } else {
-          response.json(kibanaResponse.payload);
+          payload = { error: kibanaResponse.payload.message };


Looks like we're treating an instanceof Error automatically as a Boom instance, but that's not guaranteed right?

Do you think that something like Boom.boomify() is appropriate here and in the error handler below?

Looks like we're treating an instanceof Error automatically as a Boom instance, but that's not guaranteed right?

Hmm, nope, there is no assumption about Boom here, what do you mean?

Do you think that something like Boom.boomify() is appropriate here and in the error handler below?

Well, we haven't used Boom in the new platform yet and relied on very naive error handling approach until now (manually set error code and {error: ....} body). Do you think we should bring that dependency in? I didn't work with Boom too much, to be sure that we really need it (at least in this PR).

I can't find the context now, but I felt like there was a catch handler that was expecting error.output to be a thing, which I assumed would be coming from Boom, but I might be crazy...

spalger · 2018-05-21T18:59:11Z

src/server/http/setup_connection.js

@@ -1,9 +1,7 @@
 import { readFileSync } from 'fs';


Is it possible for the BasePathProxy to share connection setup logic with the new platform so we don't loose the shared setup? I think it's important that the default dev server use as close to a "production" setup as possible and keeping this and the new platform setup logic in sync doesn't feel like intuitive or likely to happen,

epixa

I don't have any feedback besides what @spalger already flagged.

LGTM unless his feedback results in non-trivial changes.

…console.log`.

spalger

It looks like you'll be handling the base path proxy in #19424, LGTM!

azasypkin · 2018-05-28T14:59:05Z

It looks like you'll be handling the base path proxy in #19424, LGTM!

Yes, will handle it next in #19424, thanks!

azasypkin added WIP Work in progress Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Apr 25, 2018

azasypkin force-pushed the new-platform-hapi branch from 3726117 to e4cb4e1 Compare May 2, 2018 09:16

azasypkin commented May 2, 2018

View reviewed changes

azasypkin force-pushed the new-platform-hapi branch from e4cb4e1 to e97f814 Compare May 2, 2018 09:56

azasypkin commented May 2, 2018

View reviewed changes

azasypkin changed the title ~~[WIP][New Platform] Replace Express with Hapi 17.~~ [New Platform] Replace Express with Hapi 17. May 2, 2018

azasypkin added the feedback_needed label May 2, 2018

azasypkin commented May 2, 2018

View reviewed changes

epixa reviewed May 2, 2018

View reviewed changes

rhoboat reviewed May 2, 2018

View reviewed changes

rhoboat reviewed May 10, 2018

View reviewed changes

azasypkin force-pushed the new-platform-hapi branch from 645653c to 2b2940d Compare May 14, 2018 17:46

azasypkin commented May 14, 2018

View reviewed changes

azasypkin force-pushed the new-platform-hapi branch from 2b2940d to 602849f Compare May 17, 2018 04:44

spalger suggested changes May 21, 2018

View reviewed changes

epixa approved these changes May 21, 2018

View reviewed changes

azasypkin force-pushed the new-platform-hapi branch from 602849f to 75bc26d Compare May 22, 2018 15:18

azasypkin added 7 commits May 23, 2018 10:41

[New Platform] Replace Express with Hapi 17.

0546c8c

Handle review feedback №1.

a3696b8

Add new and fix existing unit tests.

1350d23

Handle review feedback №2ю

e2d7a65

Handle redirectHttpFromPort.

e15744e

Limit route handler result to KibanaResponse<T>. Remove forgotten `…

37b6203

…console.log`.

Remove underscore from private variables.

25bb53a

azasypkin force-pushed the new-platform-hapi branch from 36695ed to 25bb53a Compare May 23, 2018 08:43

azasypkin mentioned this pull request May 24, 2018

Move BasePathProxy to the new platform #19424

Merged

spalger approved these changes May 25, 2018

View reviewed changes

azasypkin merged commit b935dbd into elastic:new-platform May 28, 2018

azasypkin deleted the new-platform-hapi branch May 28, 2018 15:00

azasypkin mentioned this pull request Jun 15, 2018

Typescriptify dashboard redux code #19857

Merged

		@@ -0,0 +1,75 @@
		import { parse as parseUrl, format as formatUrl, UrlObject } from 'url';

[New Platform] Replace Express with Hapi 17. #18562

[New Platform] Replace Express with Hapi 17. #18562

Conversation

azasypkin commented Apr 25, 2018 • edited Loading

epixa commented Apr 25, 2018

azasypkin commented Apr 25, 2018 • edited Loading

epixa commented Apr 25, 2018

azasypkin commented Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhoboat commented May 2, 2018

azasypkin commented May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented May 15, 2018 • edited Loading

rhoboat commented May 17, 2018

azasypkin commented May 17, 2018

rhoboat commented May 17, 2018 • edited Loading

spalger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin May 23, 2018 • edited Loading

Choose a reason for hiding this comment

azasypkin commented Apr 25, 2018 •

edited

Loading

azasypkin commented Apr 25, 2018 •

edited

Loading

azasypkin commented Apr 25, 2018 •

edited

Loading

azasypkin May 7, 2018 •

edited

Loading

azasypkin May 2, 2018 •

edited

Loading

azasypkin May 7, 2018 •

edited

Loading

azasypkin commented May 3, 2018 •

edited

Loading

azasypkin commented May 15, 2018 •

edited

Loading

rhoboat commented May 17, 2018 •

edited

Loading

spalger May 22, 2018 •

edited

Loading

azasypkin May 23, 2018 •

edited

Loading

spalger May 24, 2018 •

edited

Loading

azasypkin May 22, 2018 •

edited

Loading