[Reporting] Remove any types and references to Hapi by tsullivan · Pull Request #49250 · elastic/kibana

tsullivan · 2019-10-24T20:57:22Z

Summary

Follow along #48825 in migrating Reporting to the Kibana new platform.

Find several any types and replace them with the typed correction

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.

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

tsullivan · 2019-10-24T21:07:57Z

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

 */

-interface AvailableTotal {
+export interface AvailableTotal {


This change doesn't seem to fix any problems, but I am not sure how this works without the export. Another file imports this interface!

elasticmachine · 2019-10-25T20:41:21Z

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

joelgriffith

Really nice to see these types coming through, just a few follow ups/questions I had!

joelgriffith · 2019-10-29T21:03:55Z

x-pack/legacy/plugins/reporting/server/lib/create_worker.ts

-    const workerFn = (job: JobSource, jobdoc: JobDocPayload | JobDoc, cancellationToken?: any) => {
+    const workerFn = (
+      job: JobSource,
+      arg1: JobDocPayload | JobDoc,


Maybe:

arg1 => job
arg2 => cancelToken?

This is not a great part of the code - without Typescript, some bad design problems were hidden.

The idea is "worker functions" can come from different export types, but one export type changes the idea about what a worker function could be, that's the "immediate" execute type, which is in CSV export from Panel Action and doesn't go through job queuing. It registers a worker function that has a different signature. It'll be great to figure this out better when ESQueue goes away :)

I'm going to leave this as-is because the arguments don't have a single semantic meaning

joelgriffith · 2019-10-29T21:04:56Z

x-pack/legacy/plugins/reporting/server/lib/validate/validate_config.ts

 import crypto from 'crypto';
-import { Logger } from '../../../types';
+import { ServerFacade, Logger } from '../../../types';
+


This is a tough one since it's setting and defaulting a config... I wonder if we can push this in our joi schema somehow?

EDIT: This was said with the thought that we should insulate ourselves from the server object as much as possible.

I agree, it is tough because we could do more here to protect the environment from bad things happening. But giving them a made-up encryptionKey on the fly does help the first-time user experience.

joelgriffith · 2019-10-29T21:07:00Z

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

    switch (exportType.jobContentEncoding) {
      case 'base64':
-        return Buffer.from(content, 'base64');
+        return content ? Buffer.from(content, 'base64') : content;


This is an interesting change, could you explain why we only do Buffer.from when it's defined (assuming the truthy check is for that)

It is because Buffer.from complains about null, so this may have been a bug hiding here.

tsullivan · 2019-11-13T22:37:08Z

@elasticmachine merge upstream

elasticmachine · 2019-11-13T23:50:19Z

💔 Build Failed

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

elasticmachine · 2019-11-14T03:11:14Z

💚 Build Succeeded

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

joelgriffith · 2019-11-14T18:32:47Z

Nice work! We'll need to figure out that encryption key stuff at some point, but save that for another PR

tsullivan · 2019-11-14T19:03:14Z

x-pack/legacy/plugins/reporting/server/lib/validate/validate_config.ts

-    config.set('xpack.reporting.encryptionKey', crypto.randomBytes(16).toString('hex'));
+
+    // @ts-ignore: No set() method on KibanaConfig, just get() and has()
+    config.set('xpack.reporting.encryptionKey', crypto.randomBytes(16).toString('hex')); // update config in memory to contain a usable encryption key


Hm, this part in particular might need a followup, if .set is not in the type because it is going away...

elasticmachine · 2019-11-14T19:36:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4965b66

tsullivan · 2019-11-14T19:56:46Z

Backporting to 7.x/7.6 depends on #50694 going in first

…ger-ace-theme * 'master' of github.com:elastic/kibana: (54 commits) [ML] Fixes word wrap in Overview page sidebar on IE (elastic#50668) Upgrade to TypeScript 3.7.2 (elastic#47188) fix: hide 'edit' button for mobile for dashboards (elastic#50639) fixes conditional links tests (elastic#50642) [SIEM] Fix IE11 timeline drag and drop issue (elastic#50528) [SIEM] Add SavedQuery in Timeline (elastic#49813) chore(NA): remove code plugin from codeowners (elastic#50451) [DOCS] Adds documentation on telemetry settings (elastic#50739) [Logs UI] Add IE11-specific CSS fixes for anomalies table (elastic#49980) [DOCS][SIEM]: Change Kibana advanced settings to match UI (elastic#50679) Change URLs for support menu (elastic#50700) [Reporting] Remove any types and references to Hapi (elastic#49250) [DOCS] Adds note about backups to Upgrade doc (elastic#50525) [Logs UI] Improve infra plugin compatibility with TS 3.7 (elastic#50491) [Task manager] Adds ensureScheduling api to allow safer rescheduling of existing tasks (elastic#50232) [DOCS] Adds link to content security policy doc (elastic#50698) Remove duplicate but in error message (elastic#50530) [ML] DF Analytics: Ensure creation flyout can be opened when no jobs exist (elastic#50417) Add filebeat notice (elastic#49065) [Monitoring] De-duplicate pipeline ids based on the ephemeral_id changing (elastic#49978) ... # Conflicts: # x-pack/legacy/plugins/grokdebugger/public/components/grok_debugger/brace_imports.ts

tsullivan · 2019-11-18T18:33:24Z

Backport: #50948

* [Reporting] Remove any types and references to Hapi * clarification comment * fix import

tsullivan requested a review from joelgriffith October 24, 2019 20:57

[Reporting] Remove any types and references to Hapi

470c47f

tsullivan force-pushed the reporting/new-platform-shim-part-iii branch from f779325 to 470c47f Compare October 24, 2019 21:05

tsullivan commented Oct 24, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

tsullivan added zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:enhancement Team:Stack Services v7.6.0 v8.0.0 labels Oct 25, 2019

joelgriffith suggested changes Oct 29, 2019

View reviewed changes

clarification comment

ba10077

Merge branch 'master' into reporting/new-platform-shim-part-iii

b0ad892

fix import

b1a9b04

Merge branch 'master' into reporting/new-platform-shim-part-iii

4965b66

joelgriffith approved these changes Nov 14, 2019

View reviewed changes

tsullivan commented Nov 14, 2019

View reviewed changes

tsullivan merged commit 08471cc into elastic:master Nov 14, 2019

tsullivan deleted the reporting/new-platform-shim-part-iii branch November 14, 2019 19:56

tsullivan mentioned this pull request Nov 18, 2019

[7.x] [Reporting] Remove any types and references to Hapi (#49250) #50948

Merged

tsullivan added a commit to tsullivan/kibana that referenced this pull request Nov 18, 2019

[Reporting] Remove any types and references to Hapi (elastic#49250)

c4211b8

* [Reporting] Remove any types and references to Hapi * clarification comment * fix import

tsullivan added a commit that referenced this pull request Nov 18, 2019

[Reporting] Remove any types and references to Hapi (#49250) (#50948)

960800f

* [Reporting] Remove any types and references to Hapi * clarification comment * fix import

Conversation

tsullivan commented Oct 24, 2019

Summary

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

elasticmachine commented Oct 25, 2019

Uh oh!

joelgriffith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Nov 13, 2019

Uh oh!

elasticmachine commented Nov 13, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

Uh oh!

joelgriffith commented Nov 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

Uh oh!

tsullivan commented Nov 14, 2019

Uh oh!

tsullivan commented Nov 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants