[Reporting/np-k8] Remove several oncePerServer usages #50997

tsullivan · 2019-11-19T00:50:02Z

Summary

Pulled from #50973

This is part of Reporting New Platform migration efforts: removing references of the Hapi server object.

Working on New Platform changes to remove references to the Hapi server object caused a trickle of TS check failures. This PR handles mostly the fixes for the failures, as well as the low-fruit changes for removing references to the server object.

We need to make sure that these changes don't create a negative performance impact.

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~~

elasticmachine · 2019-11-19T00:50:04Z

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

elasticmachine · 2019-11-19T02:02:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 0ae84a6

…onceperserver-i

tsullivan · 2019-11-19T21:05:44Z

x-pack/legacy/plugins/reporting/export_types/csv_from_savedobject/server/execute_job.ts

+        'xpack.reporting.exportTypes.csv_from_savedobject.executeJob.failedToAccessPanel',
+        { defaultMessage: 'Failed to access panel metadata for job execution' }
+      );
+    }


This had to be added for types reasoning: panel can be undefined per the TS fixes in this PR.

elasticmachine · 2019-11-19T22:39:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: a95a0e1

…ParamsUrl

elasticmachine · 2019-11-19T23:27:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3abc284

elasticmachine · 2019-11-20T05:04:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ee430a7

tsullivan · 2019-11-20T18:05:12Z

x-pack/legacy/plugins/reporting/types.d.ts

@@ -193,9 +198,10 @@ export interface JobParamPostPayload {

 export interface JobDocPayload {
  headers?: Record<string, string>;
-  jobParams: object;
+  jobParams: any;


The JobDocPayload's jobParams depends on what kind of job it is. It could be an object with savedObjectId (CSV export) or a URL (PDF). Getting to a point where there is a "base" JobDocPayload type that is extended for more specific contexts, has been hard with the way I've tried: any minor change can cause seemingly random TS checking to fail, at any place in Reporting code.

For now, I'm switching this to any but I think the correct way to do this is with TypeScript Generics. At some point in the future, I could see that the way to get the JobDocPayload type in a specific context would be to use a generic:

let fooPayloadA: JobDocPayload<JobParamsSavedObject>; let fooPayloadB: JobDocPayload<JobParamsUrl>;

tsullivan · 2019-11-20T18:09:18Z

x-pack/legacy/plugins/reporting/types.d.ts

-) => Promise<object>;
+) => Promise<JobParamsSavedObject | JobParamsUrl>;
+
+export type ImmediateCreateJobFn = (


Same for here as https://github.com/elastic/kibana/pull/50997/files#r348662563

tsullivan · 2019-11-20T18:34:41Z

x-pack/legacy/plugins/reporting/export_types/csv/server/create_job.ts


-function createJobFn(server: ServerFacade) {
+export function createJobFactory(server: ServerFacade): ESQueueCreateJobFnDiscoverCsv {


Oops, there is another PR that does these changes: https://github.com/elastic/kibana/pull/49426/files#diff-e6c74eb5d977f06c7f116aa268248b9cR11

Let's go with this one and I'll close the other

Inspired by @joelgriffith's PR, I think it would be better to do this like:

export const createJobFactory: CreateJobFactory = function createJobFn(server: ServerFacade) {

That's what I was originally looking for in my changes, but couldn't get the JS right.

tsullivan · 2019-11-20T18:35:52Z

x-pack/legacy/plugins/reporting/export_types/png/server/create_job/index.ts

 import { JobParamsPNG, ESQueueCreateJobFnPNG } from '../../types';

-function createJobFn(server: ServerFacade) {
+export function createJobFn(server: ServerFacade): ESQueueCreateJobFnPNG {


https://github.com/elastic/kibana/pull/49426/files#diff-4dd055be05123b17234a25322487fc0dR12

Oops, bug: should be createJobFactory

tsullivan · 2019-11-20T18:37:34Z

x-pack/legacy/plugins/reporting/server/routes/lib/get_document_payload.ts

@@ -39,7 +38,7 @@ const getReportingHeaders = (output: JobDocOutputExecuted, exportType: ExportTyp
  return metaDataHeaders;
 };

-function getDocumentPayloadFn(server: ServerFacade) {
+export function getDocumentPayloadFactory(server: ServerFacade) {


https://github.com/elastic/kibana/pull/49426/files#diff-a21790aa8cc3b71aadd0e5c6630fbed0R35

joelgriffith

Code LGTM, I didn't see any test changes though? Guess that this is more side-effect in nature anyways.

tsullivan · 2019-11-21T03:23:50Z

test changes

There potentially would have been changes need in some of the mock objects that are tests are using for job params, and job payload, but since those are typed as any in this PR, no changes are needed in there.

tsullivan · 2019-11-21T03:28:30Z

@timroes can I please trouble you to take a 2nd look at this PR, or delegate someone outside of Stack Services to do so?

I'd like to start getting outside teams a little familiar with Reporting code, as well as gather feedback on our types and integration with the new platform. The Reporting code going through change for New Platform migration are what take in all the integrations that all apps have with Reporting (job generation, deriving export type from the job params, etc).

tsullivan · 2019-11-21T17:30:15Z

@elasticmachine merge upstream

elasticmachine · 2019-11-21T18:59:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0dece79

elasticmachine · 2019-11-21T20:12:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bb04e39

tsullivan · 2019-11-21T21:10:38Z

Hi Joel, since this almost was merged with a bug in it. The import reference bug would have been caught if the kibana/x-pack/legacy/plugins/reporting/export_types/*/server/index.js files were converted to TypeScript, but I'm going to hold off on that for a later PR. We talked about some ways we want to change up how export types get registered, and these are the files where that happens.

elasticmachine · 2019-11-21T23:00:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f111212

* [Reporting/np-k8] Remove several oncePerServer usages * ts fixes 1 * ts fixes 2 * more ts fixes * more ts fixes * more ts fixes * ts simplification * improve ts * remove any type for jobParams and define JobParamsSavedObject and JobParamsUrl * ts simplification * Fix ts * ts simplification * fix ts * bug fix * align with joels pr

elasticmachine · 2019-11-22T04:43:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: f111212

* [Reporting/np-k8] Remove several oncePerServer usages * ts fixes 1 * ts fixes 2 * more ts fixes * more ts fixes * more ts fixes * ts simplification * improve ts * remove any type for jobParams and define JobParamsSavedObject and JobParamsUrl * ts simplification * Fix ts * ts simplification * fix ts * bug fix * align with joels pr

[Reporting/np-k8] Remove several oncePerServer usages

0ae84a6

tsullivan added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Stack Services Feature:NP Migration v7.6.0 labels Nov 19, 2019

tsullivan added 6 commits November 18, 2019 21:07

ts fixes 1

959d554

ts fixes 2

b02086d

more ts fixes

c1b6964

more ts fixes

5394bcc

more ts fixes

2c9b077

Merge remote-tracking branch 'upstream/master' into reporting/remove-…

69027d6

…onceperserver-i

tsullivan commented Nov 19, 2019

View reviewed changes

tsullivan added 2 commits November 19, 2019 14:14

ts simplification

a95a0e1

improve ts

3abc284

remove any type for jobParams and define JobParamsSavedObject and Job…

9ca9e61

…ParamsUrl

tsullivan added 4 commits November 19, 2019 19:41

ts simplification

b31a249

Fix ts

a056ca4

ts simplification

be13200

fix ts

ee430a7

tsullivan requested review from timroes, joelgriffith and a team November 20, 2019 18:00

tsullivan commented Nov 20, 2019

View reviewed changes

joelgriffith approved these changes Nov 20, 2019

View reviewed changes

tsullivan mentioned this pull request Nov 20, 2019

Chore/remove once per server #49426

Merged

elasticmachine and others added 2 commits November 21, 2019 09:30

Merge branch 'master' into reporting/remove-onceperserver-i

0dece79

bug fix

bb04e39

align with joels pr

f111212

tsullivan merged commit 1034bb4 into elastic:master Nov 22, 2019

tsullivan deleted the reporting/remove-onceperserver-i branch November 22, 2019 03:06

tsullivan mentioned this pull request Nov 22, 2019

[7.x] [Reporting/np-k8] Remove several oncePerServer usages (#50997) #51416

Merged

tsullivan mentioned this pull request Nov 25, 2019

[Reporting/NP Migration] Remove server.expose of ExportTypeRegistry #50973

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting/np-k8] Remove several oncePerServer usages #50997

[Reporting/np-k8] Remove several oncePerServer usages #50997

tsullivan commented Nov 19, 2019 •

edited

Loading

elasticmachine commented Nov 19, 2019

elasticmachine commented Nov 19, 2019

tsullivan Nov 19, 2019

elasticmachine commented Nov 19, 2019

elasticmachine commented Nov 19, 2019

elasticmachine commented Nov 20, 2019

tsullivan Nov 20, 2019 •

edited

Loading

tsullivan Nov 20, 2019

tsullivan Nov 20, 2019

joelgriffith Nov 20, 2019

tsullivan Nov 20, 2019 •

edited

Loading

tsullivan Nov 20, 2019

tsullivan Nov 21, 2019

tsullivan Nov 20, 2019

joelgriffith left a comment

tsullivan commented Nov 21, 2019

tsullivan commented Nov 21, 2019

tsullivan commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

tsullivan commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

elasticmachine commented Nov 22, 2019


		function createJobFn(server: ServerFacade) {
		export function createJobFactory(server: ServerFacade): ESQueueCreateJobFnDiscoverCsv {

[Reporting/np-k8] Remove several oncePerServer usages #50997

[Reporting/np-k8] Remove several oncePerServer usages #50997

Conversation

tsullivan commented Nov 19, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 19, 2019

elasticmachine commented Nov 19, 2019

💔 Build Failed

tsullivan Nov 19, 2019

Choose a reason for hiding this comment

elasticmachine commented Nov 19, 2019

💔 Build Failed

elasticmachine commented Nov 19, 2019

💚 Build Succeeded

elasticmachine commented Nov 20, 2019

💚 Build Succeeded

tsullivan Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

tsullivan Nov 20, 2019

Choose a reason for hiding this comment

tsullivan Nov 20, 2019

Choose a reason for hiding this comment

joelgriffith Nov 20, 2019

Choose a reason for hiding this comment

tsullivan Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

tsullivan Nov 20, 2019

Choose a reason for hiding this comment

tsullivan Nov 21, 2019

Choose a reason for hiding this comment

tsullivan Nov 20, 2019

Choose a reason for hiding this comment

joelgriffith left a comment

Choose a reason for hiding this comment

tsullivan commented Nov 21, 2019

tsullivan commented Nov 21, 2019

tsullivan commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

💚 Build Succeeded

elasticmachine commented Nov 21, 2019

💚 Build Succeeded

tsullivan commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

💚 Build Succeeded

elasticmachine commented Nov 22, 2019

💔 Build Failed

tsullivan commented Nov 19, 2019 •

edited

Loading

tsullivan Nov 20, 2019 •

edited

Loading

tsullivan Nov 20, 2019 •

edited

Loading