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] Move code out of Legacy #67904

Merged
merged 22 commits into from
Jun 3, 2020

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jun 1, 2020

Summary

Closes #53898

Move all the Reporting migrated code from legacy to the Kibana Platform (NP). Previously, Reporting plugin initialization was happening in legacy code, using mocked plugin context and cross-plugin dependences. The legacy code is completely removed in this code, and the New Platform code was updated to absorb the plugin init responsibilities, using true plugin init context and cross-plugin dependencies

This PR has no changes to user-facing features or breaking changes.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@tsullivan tsullivan force-pushed the chore/reporting-np-move branch 3 times, most recently from 9f0d370 to c1b0429 Compare June 1, 2020 22:02
@tsullivan tsullivan added Feature:NP Migration (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes Team:Reporting Services v7.9.0 v8.0.0 labels Jun 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan tsullivan requested review from joelgriffith and a team June 1, 2020 22:04
@tsullivan tsullivan force-pushed the chore/reporting-np-move branch 3 times, most recently from 0c824ba to 5d7a5fa Compare June 2, 2020 00:29
@@ -9,7 +9,7 @@ import { map, trunc } from 'lodash';
import open from 'opn';
import { ElementHandle, EvaluateFn, Page, Response, SerializableOrJSHandle } from 'puppeteer';
import { parse as parseUrl } from 'url';
import { ViewZoomWidthHeight } from '../../../../export_types/common/layouts/layout';
import { ViewZoomWidthHeight } from '../../../export_types/common/layouts/layout';
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO (future PR) move export_type/common code to server/ and server/lib

error: consoleLogger('error'),
fatal: consoleLogger('fatal'),
log: consoleLogger('log'),
};
Copy link
Member Author

Choose a reason for hiding this comment

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

There may be an easier way to do this, but LoggerFactory is just an interface. It would be nice to have a constructor to call from here instead of implementing the interface.

@tsullivan tsullivan requested a review from a team June 2, 2020 17:46
@tsullivan tsullivan marked this pull request as ready for review June 3, 2020 17:12
@tsullivan tsullivan requested review from a team as code owners June 3, 2020 17:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes LGTM
code review

@@ -40,7 +40,7 @@ echo "Creating bootstrap_cache archive"
# archive cacheable directories
mkdir -p "$HOME/.kibana/bootstrap_cache"
tar -cf "$HOME/.kibana/bootstrap_cache/$branch.tar" \
x-pack/legacy/plugins/reporting/.chromium \
x-pack/plugins/reporting/.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 find on these

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Operations: LGTM

@pgayvallet
Copy link
Contributor

ack: will review tomorrow.

@joelgriffith joelgriffith mentioned this pull request Jun 3, 2020
7 tasks
@joelgriffith
Copy link
Contributor

@pgayvallet this is mostly just us git mving files, however we've done some updates in how our plugin consumes initializer/config arguments. Facades and other legacy stuff are now history.

@tsullivan
Copy link
Member Author

Re #67904 (comment), I've updated the PR description to mention the things that had to be done in the code other than moving files.

@LeeDr
Copy link

LeeDr commented Jun 3, 2020

@tsullivan I didn't see any reviewers up to this point mention that they did any functional testing of reporting. I don't think our automated coverage is very good on generated PDFs but I could have missed some tests. Could you please comment on the automated test coverage on reporting and any manual tests the reporting team has done on this PR. And also what kind of manual testing (if any) still needs to be done before merging this?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan
Copy link
Member Author

tsullivan commented Jun 3, 2020

@LeeDr automated functional test coverage of PDFs is in fact pretty good, and test coverage of reporting is pretty good in general. Both kind of tests have caught critical issues while this PR was in development.

Quick summary of what automated testing has:

  • report generation completes from the UI and by sending POST URL
  • deletion of reports
  • ensure that a user can only see their own reports (when security is enabled)
  • PNG comparison of an exported dashboard
  • CSV exports correctly format the data

Edit: Note that we don't have PDF comparison tests, but we do test that the PDF download status is 200.

@@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { ReportingCore } from '../../../../';
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I'm using the "Organize Imports" feature in VS Code to straighten these out. Not sure why it put this one at the top, but it's ok I guess.

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

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

LGTM

@LeeDr
Copy link

LeeDr commented Jun 3, 2020

@LeeDr automated functional test coverage of PDFs is in fact pretty good, and test coverage of reporting is pretty good in general. Both kind of tests have caught critical issues while this PR was in development.

Quick summary of what automated testing has:

  • report generation completes from the UI and by sending POST URL
  • deletion of reports
  • ensure that a user can only see their own reports (when security is enabled)
  • PNG comparison of an exported dashboard
  • CSV exports correctly format the data

Edit: Note that we don't have PDF comparison tests, but we do test that the PDF download status is 200.

Thanks @tsullivan Good to hear.

@joelgriffith
Copy link
Contributor

joelgriffith commented Jun 3, 2020

@LeeDr alongside Tim's comment, I did the following manual tests (with Sample Data ™️):

  • Export of a Dashboard with Maps
  • Export of a Dashboard with Preserve Layout
  • Export of a Discover Table
    • Export of a one with formulas (testing both escaping + warnings)
    • Export of one wo to ensure no warnings/escaping
  • Export of a Saved Search Table (immediate download).
  • Export via the POST URL cURL call
  • Validate CRUD operations in the UI (can view, delete and page through reports).

Alongside that, I did also add numerous automated unit/functional tests for the generation routes. They're not as clear on this PR, but they're here for illustrative purposes: x-pack/legacy/plugins/reporting/server/routes/generation.test.ts → x-pack/plugins/reporting/server/routes/generation.test.ts. I can point to the changes from the route-specific PR if that's helpful.

About the only thing I haven't had a chance to check is the code-coverage from our package. I'd be curious to see that since my suspicion is that our coverage has gone up.

EDIT: Unit/functional test changes are here: https://github.com/elastic/kibana/pull/66331/files#diff-d1a8df0442cc9e5180f8f3fdd587810b. In most there's a few additional specs I've added where it made sense

FINAL EDIT: We also did shore up a few loose any types as well, meaning our system is a bit more air-tight type-wise. There's a few modules we don't have typed that we're working on removing (esqueue to TaskManager)

@tsullivan tsullivan merged commit 2f67cbf into elastic:master Jun 3, 2020
@tsullivan tsullivan deleted the chore/reporting-np-move branch June 3, 2020 22:41
@joelgriffith
Copy link
Contributor

7.x PR: #68201 (targeting 7.9)

joelgriffith pushed a commit that referenced this pull request Jun 4, 2020
* [Reporting] Move code out of Legacy (#67904)

* [Reporting] Move code out of Legacy

* Elasticsearch is not a plugin dep

* add data as plugin dependo

* diff cleanup 1

* log the browser download

* Update paths in outside code for new Reporting home

* fix download test

* add numeral typing for x-pack/test

* Fix jest tests for np migration

* Shorten import paths

* remove this file, add typings to the node module

* remove local typing that has been provided by node module

* Add optional security plugin dep

* revert conflicting apm typings removal

* fix i18n

* fix snakecase whitelist

Co-authored-by: Joel Griffith <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	.ci/packer_cache.sh
#	src/dev/code_coverage/ingest_coverage/__tests__/transforms.test.js
#	src/dev/code_coverage/ingest_coverage/integration_tests/mocks/jest-combined/coverage-summary-manual-mix.json
#	x-pack/plugins/reporting/server/export_types/printable_pdf/server/create_job/index.ts

* Add back in legacy bwc route

* Kill code_coverage since it's not supposed to be here...

Co-authored-by: Tim Sullivan <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 4, 2020
…ms-column

* 'master' of github.com:elastic/kibana: (63 commits)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  Update documentation/examples of deprecated namespaceAgnostic field (elastic#68039)
  [DOCS] Updates Canvas docs with new menus (elastic#66061)
  chore(NA): avoids imports of server or public code into common (elastic#67231)
  [SIEM] Fix GetOneTimeline graphql type (elastic#68137)
  skip flaky suite (elastic#67838)
  [Uptime] Add loading message for monitor list no items (elastic#67378)
  [Ingest Manager] Update indexing strategy docs to use dataset.* (elastic#68068)
  [Ingest Manager] Fix datasource validation for streams without vars (elastic#67950)
  ...

# Conflicts:
#	x-pack/plugins/index_management/__jest__/client_integration/helpers/index.ts
#	x-pack/plugins/index_management/__jest__/client_integration/home.test.ts
#	x-pack/plugins/index_management/__jest__/client_integration/home/index_templates_tab.helpers.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 4, 2020
* master: (26 commits)
  [Console]remove completion for type for filter queries and aggs (elastic#68103)
  [ML] Transforms: Filter aggregation support (elastic#67591)
  [ES UI Shared] Monaco XJSON (elastic#67485)
  [Index Management] Add data streams functionality to indices tab (elastic#67940)
  [Discover] Fix renaming of saved search not displayed in breadcrumb (elastic#67577)
  [SECURITY] Rename siem plugin to security_solution (elastic#67902)
  [Uptime] Fix Telemetry Api flaky test (elastic#67358)
  [Data plugin] Add configuration property to enable / disable autocomplete (elastic#67847)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Reporting] New Platform Migration
9 participants