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 router + license checks to new platform #66331

Merged
merged 50 commits into from
May 29, 2020
Merged

[Reporting]: Move router + license checks to new platform #66331

merged 50 commits into from
May 29, 2020

Conversation

joelgriffith
Copy link
Contributor

@joelgriffith joelgriffith commented May 12, 2020

Moves our routes and relevant factories/classes to new platform idioms. Most of the fundamental pieces are in place, but there is still work needing to be done:

  • Update and fix tests.
  • Remove unused code/typedefs.
  • Smoke test the heck out of this.
  • Fix schema validation differences.

@joelgriffith joelgriffith 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 Feature:NP Migration v7.9.0 labels May 12, 2020
await pluginInstance.setup(coreSetup, {
elasticsearch: coreSetup.elasticsearch,
security: server.newPlatform.setup.plugins.security as SecurityPluginSetup,
usageCollection: server.newPlatform.setup.plugins.usageCollection,
__LEGACY,
licensing: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to verify this is working properly as I'm not fully versed on how this works

@@ -35,7 +34,7 @@ export function enqueueJobFactory(reporting: ReportingCore, parentLogger: Logger
return async function enqueueJob<JobParamsType>(
exportTypeId: string,
jobParams: JobParamsType,
user: string,
username: string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change here helped me tighten down types in downstream modules

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

just a few small nits :)

@pgayvallet
Copy link
Contributor

Ack: will review tomorrow.

@@ -27,6 +27,7 @@ export const WHITELISTED_JOB_CONTENT_TYPES = [
'application/pdf',
CONTENT_TYPE_CSV,
'image/png',
'text/plain',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed now so we can pass long certain job-status messages (they were incorrectly being sent as application/json, and I think hapi was fixing that under-the-hood)

* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed since new config schema doesn't have an API for this

@joelgriffith
Copy link
Contributor Author

Pushed up a larger delta for job-status tests, still more that needs to be done but the rest should be relatively quick since we have patterns in place for how to do this.

import { JobParamsPDF } from '../../types';

export const createJobFactory: CreateJobFactory<ESQueueCreateJobFn<
JobParamsPDF
>> = function createJobFactoryFn(reporting: ReportingCore) {
>> = function createJobFactoryFn(reporting) {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for cleaning up this inferrable types!

@@ -46,30 +48,10 @@ export class ReportingCore {
private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>();
private exportTypesRegistry = getExportTypesRegistry();

constructor(private logger: LevelLogger, private config: ReportingConfig) {}

legacySetup(
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@joelgriffith
Copy link
Contributor Author

Once this is green, my plan is to:

  • Merge it (duh)
  • Get the backport PR started.
  • Fix any backport PR conflicts... might be a few.
  • Start on the folder migration, remove any TS/Lint issues and do a final PR.

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you SO much for the work you have put into this, @joelgriffith. This PR a major, major improvement.

@joelgriffith joelgriffith dismissed tsullivan’s stale review May 29, 2020 21:01

Approved elsewhere, not sure why GH is having this block merge

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@joelgriffith
Copy link
Contributor Author

Backport [7.9]: #67818

joelgriffith pushed a commit that referenced this pull request Jun 2, 2020
…) (#67818)

* [Reporting]: Move router + license checks to new platform (#66331)

* WIP: Move routes to new API, license and other checks inbound

* Move license checks over to np licensing observable

* Fix license checks + remove older modules

* Fixing check_license tests, move to TS/Jest

* Fix licensing setup for mocks

* Move job.test.ts over to np

* WIP: move user checks to higher-order func

* Move more handler logic over to Response factory vs Boom

* Major refactor to consolidate types, remove facades, and udpate helpers

* Fix validation for dates in immediate exports

* Linter fix on check license test

* Fix job generation tests

* Move deps => setupDeps

* fix api test

* fix jobs test

* authorized_user_pre_routing and tests

* Fixing duplicate identifiers

* Fix licensing implementation changes

* WIP: Moving license over to async/observables

* Fix disabled-security case

* finish auth_user_pre_routing cleanup - no more license check

* WIP: Fixing final api tests

* Trying to get schema differences in alignment

* Reverting back to previous generation handler

* Fix final API tests

* Final API test fixes, few more hardening tests and better error messages

* Simplify lower-level module implementation (core only interface) + test updates

* Push some core logic into plugin

* Move some core logic up to plugin

* Marking private setupDeps + downstream fixes

* revert logger as a param

Co-authored-by: Timothy Sullivan <tsullivan@elastic.co>
# Conflicts:
#	x-pack/legacy/plugins/reporting/export_types/printable_pdf/server/create_job/index.ts
#	x-pack/legacy/plugins/reporting/server/routes/generation.ts

* Add back in legacy /viz /search and /dashboard routes

* Add back in and fix compatibility shim/tests
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 v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants