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

[Reporting/New Platform] Prepare Typedefs for Legacy Shimming #48825

Merged
merged 18 commits into from
Oct 23, 2019

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Oct 21, 2019

Summary

This PR continues New Platform migration for Reporting.

  1. Replace custom KbnServer type with new ServerFacade type based on Legacy.Server
  2. Replace references to Hapi with new RequestFacade type and Legacy.* types.
  3. Removed a few indirections left over from supporting dual browsers (PhantomJS & Chromium) in the config.
  4. Change the config fields in server.route object to options (config was deprecated in Hapi v17)
  5. Various cleanup and fixes that had to be done as a result of improving the types

Release Notes: Changes to prepare Reporting for Kibana v8.0.0

Checklist

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

@tsullivan tsullivan added review (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 Team:Stack Services Feature:NP Migration v7.6.0 labels Oct 21, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services (Team:Stack Services)

@tsullivan tsullivan requested review from bmcconaghy and a team October 21, 2019 21:02
@@ -182,22 +177,23 @@ export const reporting = (kibana) => {
}).default();
},

// TODO: Decouple Hapi: Build a server facade object based on the server to
// pass through to the libs. Do not pass server directly
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -193,13 +242,6 @@ export interface ExportTypeDefinition {
validLicenses: string[];
}

// Note: this seems to be nearly a duplicate of ExportTypeDefinition
export interface ExportType {
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this because it was duplicate of ExportTypeDefinition

server.route({
path: `${BASE_GENERATE}/{p*}`,
method: 'GET',
config: getRouteConfig(),
Copy link
Member Author

Choose a reason for hiding this comment

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

This was removed because the routing middleware this adds is not applicable to the GET request for this.

@tsullivan tsullivan force-pushed the reporting/new-platform-shim-part-i branch from 6c46e61 to 6de1d4b Compare October 22, 2019 21:53
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elastic elastic deleted a comment from elasticmachine Oct 23, 2019
@elastic elastic deleted a comment from elasticmachine Oct 23, 2019
@elastic elastic deleted a comment from elasticmachine Oct 23, 2019
@elastic elastic deleted a comment from elasticmachine Oct 23, 2019
@elastic elastic deleted a comment from elasticmachine Oct 23, 2019
@elastic elastic deleted a comment from elasticmachine Oct 23, 2019
@elastic elastic deleted a comment from elasticmachine Oct 23, 2019
@@ -21,15 +22,15 @@ import { clean } from './clean';
* @return {Promise<undefined>}
*/
export async function ensureBrowserDownloaded(browserType: BrowserType) {
await ensureDownloaded([BROWSERS_BY_TYPE[browserType]]);
await ensureDownloaded([chromium]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

@joelgriffith
Copy link
Contributor

LGTM, seems mostly Type/name changes from what I read. Did you have any other concerns @tsullivan?

@tsullivan
Copy link
Member Author

LGTM, seems mostly Type/name changes from what I read. Did you have any other concerns @tsullivan?

Nope, no concerns. The purpose is just to get rid of our custom types that are provided by the core legacy framework, and get rid of direct references to Hapi.

@tsullivan tsullivan merged commit b23cfbd into elastic:master Oct 23, 2019
@tsullivan tsullivan deleted the reporting/new-platform-shim-part-i branch October 23, 2019 21:49
tsullivan added a commit to tsullivan/kibana that referenced this pull request Oct 23, 2019
…c#48825)

* [Reporting/New Platform] Prepare Typedefs for Legacy Shim

* cleanup redundant

* fix opts.interval / opts.pollInterval

* comment on next todo

* fix pretty

* captureconfig typo

* fix typescript conflict with shim

* clean up loc

* custom response toolkit type

* remove unecessary route options

* move getRouteOptions CSV

* fix route configs

* cleanup

* split up type exports

* fix types

* fix ts errors

* remove an any type
tsullivan added a commit that referenced this pull request Oct 24, 2019
#49124)

* [Reporting/New Platform] Prepare Typedefs for Legacy Shim

* cleanup redundant

* fix opts.interval / opts.pollInterval

* comment on next todo

* fix pretty

* captureconfig typo

* fix typescript conflict with shim

* clean up loc

* custom response toolkit type

* remove unecessary route options

* move getRouteOptions CSV

* fix route configs

* cleanup

* split up type exports

* fix types

* fix ts errors

* remove an any type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants