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] Wholesale moves client to newest-platform #58945

Merged
merged 38 commits into from
Mar 17, 2020
Merged

[Reporting] Wholesale moves client to newest-platform #58945

merged 38 commits into from
Mar 17, 2020

Conversation

joelgriffith
Copy link
Contributor

@joelgriffith joelgriffith commented Feb 28, 2020

Summary

Moves all of our UI-facing architecture to newest platform.

TODO

  • Remove client code in legacy space
  • Remove injectedVars and other artifacts in the legacy server
  • Update Panel Actions to be new-platform
  • Fix and update any tests
  • Final smoke test (license changes, rejection messages, and any other regressions).

QA List

  • Biggest thing to check is license handling since all logic got ported to the client.
  • Want to ensure that all the prior functionality was there:
    • Gold licenses should have PNG/PDF reporting working.
    • Basic licenses should have just CSV exporting from Discover and Saved Searches.
    • Open should have none of the above options.

max_attempts: number;
csv_contains_formulas: boolean;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The signature for this component has changed pretty drastically, as we now pass down license-observables, and other clients, through props.

constructor(initializerContext: PluginInitializerContext) {}

public setup(core: CoreSetup) {}
public setup(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Core setup is here.

@tsullivan tsullivan self-requested a review February 28, 2020 21:59
@tsullivan tsullivan mentioned this pull request Mar 6, 2020
19 tasks
import { downloadReport } from '../lib/download_report';
import { jobQueueClient, JobQueueEntry } from '../lib/job_queue_client';

import { ToastsSetup, ApplicationStart } from '../../';
Copy link
Member

Choose a reason for hiding this comment

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

Is it still the case that we can't import these from src/core/public? IIRC that limitation was going on because this code was in legacy earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was to get around not being able to import in sub-modules. I'm not sure that restriction is lifted now, but I'll check.

Copy link
Member

Choose a reason for hiding this comment

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

I believe this can switch to import { ToastsSetup, ApplicationStart } from 'src/core/public';

@@ -23,22 +23,6 @@ export type Job = EventEmitter & {
};
};

export interface ReportingConfigOptions {
Copy link
Contributor Author

@joelgriffith joelgriffith Mar 13, 2020

Choose a reason for hiding this comment

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

No longer needed since we don't export these to the client (and it's not used anywhere according to TS)

@joelgriffith joelgriffith requested a review from a team March 13, 2020 18:44
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.

I found that a lot of the relative imports can be simplified if we import from another UI NP plugin.

@@ -13,6 +13,8 @@ import {
NotificationsStart,
} from '../../../src/core/public';

export { ToastsSetup, HttpSetup, ApplicationStart, IUiSettingsClient } from 'src/core/public';
Copy link
Member

Choose a reason for hiding this comment

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

Let's try to keep this file super lean, and have server things in reporting/server/types.d.ts and UI stuff in reporting/public/types.d.ts. But I don't think we need this export here anyway, because our UI libs are in NP and they can import from src/core/public

import { downloadReport } from '../lib/download_report';
import { jobQueueClient, JobQueueEntry } from '../lib/job_queue_client';

import { ToastsSetup, ApplicationStart } from '../../';
Copy link
Member

Choose a reason for hiding this comment

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

I believe this can switch to import { ToastsSetup, ApplicationStart } from 'src/core/public';

import * as reportingClient from '../lib/reporting_client';
import { ReportingAPIClient } from '../lib/reporting_api_client';
import { toMountPoint } from '../../../../../src/plugins/kibana_react/public';
import { ToastsSetup } from '../../';
Copy link
Member

Choose a reason for hiding this comment

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

import { ToastsSetup } from 'src/core/public';

@@ -8,8 +8,12 @@ import { EuiSpacer, EuiSwitch } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import React, { Component, Fragment } from 'react';
import { ReportingPanelContent } from './reporting_panel_content';
import { ReportingAPIClient } from '../lib/reporting_api_client';
import { ToastsSetup } from '../../';
Copy link
Member

Choose a reason for hiding this comment

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

import { ToastsSetup, ApplicationStart } from 'src/core/public';

import { checkLicense } from '../lib/license_check';
import { LicensingPluginSetup } from '../../../licensing/public';
import { ShareContext } from '../../../../../src/plugins/share/public';
import { ToastsSetup } from '../..';
Copy link
Member

Choose a reason for hiding this comment

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

import { ToastsSetup, ApplicationStart } from 'src/core/public';

import { ShareContext } from '../../../../../../src/plugins/share/public';
import { LicensingPluginSetup } from '../../../licensing/public';
import { ShareContext } from '../../../../../src/plugins/share/public';
import { ToastsSetup, IUiSettingsClient } from '../../';
Copy link
Member

Choose a reason for hiding this comment

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

import { ToastsSetup, IUiSettingsClient } from 'src/core/public';

import { HttpSetup, NotificationsStart } from '../../../../../src/core/public';
import { SourceJob, JobSummary, HttpService } from '../../index.d';
import { JobQueue } from './job_queue';
import { NotificationsStart } from '../../../../../src/core/public';
Copy link
Member

Choose a reason for hiding this comment

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

I think import { NotificationsStart } from 'src/core/public'; would work.

IncompatibleActionError,
} from '../../../../../../src/plugins/ui_actions/public';
import { CoreSetup } from '../../../../../src/core/public';
import { Action, IncompatibleActionError } from '../../../../../src/plugins/ui_actions/public';
Copy link
Member

Choose a reason for hiding this comment

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

import { CoreSetup } from 'src/core/public';
import { Action, IncompatibleActionError } from 'src/plugins/ui_actions/public';

@joelgriffith
Copy link
Contributor Author

Imports have been fixed, carry on

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.

I'm afraid to say, it looks like the "Copy POST URL" buttons are not working. I tested in Discover and PDF / PNG for Visualize and Dashboard.

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM

@tsullivan tsullivan added the RFC/final-comment-period If no concerns are raised in 3 business days this RFC will be accepted label Mar 16, 2020
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!
reviewed the code, tested all the licenses + expired trial license.

@joelgriffith
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Looks a-ok from a Canvas perspective. That one jobCompletionNotification issue is something we'll have to look into but I don't think affects anything at the moment.

@joelgriffith
Copy link
Contributor Author

GREEN BUILD, GREEN BUILD!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@joelgriffith joelgriffith merged commit 6b7731b into elastic:master Mar 17, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 17, 2020
* master: (51 commits)
  do not update cell background if is label cell (elastic#60308)
  FTR configurable test users (elastic#52431)
  [Reporting] Wholesale moves client to newest-platform (elastic#58945)
  [Ingest] Support `show_user` package registry flag (elastic#60338)
  [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380)
  [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108)
  [Fleet] Add config revision to fleet agents (elastic#60292)
  Allow kbn-config-schema to ignore unknown keys (elastic#59560)
  [ML] Functional tests - disable df analytics clone tests
  skip flaky suite (elastic#58643) (elastic#58991)
  [FTR] Add support for --include and --exclude files via tags (elastic#60123)
  [SIEM] Fix link on overview page (elastic#60348)
  skip flaky test (elastic#60369)
  [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242)
  [Lens] Simplify state management from visualization (elastic#58279)
  Changing default type to start and allowing it to be configured by the event category (elastic#60323)
  [ML] Adds the class_assignment_objective to classification (elastic#60358)
  [TSVB] fix text color when using custom background color (elastic#60261)
  Fix import to timefilter from in TSVB (elastic#60296)
  [NP] Get rid of usage redirectWhenMissing service (elastic#59777)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 17, 2020
* alerting/view-in-app: (53 commits)
  fixed typo
  handle optional alerting plugin
  do not update cell background if is label cell (elastic#60308)
  FTR configurable test users (elastic#52431)
  [Reporting] Wholesale moves client to newest-platform (elastic#58945)
  [Ingest] Support `show_user` package registry flag (elastic#60338)
  [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380)
  [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108)
  [Fleet] Add config revision to fleet agents (elastic#60292)
  Allow kbn-config-schema to ignore unknown keys (elastic#59560)
  [ML] Functional tests - disable df analytics clone tests
  skip flaky suite (elastic#58643) (elastic#58991)
  [FTR] Add support for --include and --exclude files via tags (elastic#60123)
  [SIEM] Fix link on overview page (elastic#60348)
  skip flaky test (elastic#60369)
  [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242)
  [Lens] Simplify state management from visualization (elastic#58279)
  Changing default type to start and allowing it to be configured by the event category (elastic#60323)
  [ML] Adds the class_assignment_objective to classification (elastic#60358)
  [TSVB] fix text color when using custom background color (elastic#60261)
  ...
@joelgriffith
Copy link
Contributor Author

7.x PR: #60437

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2020
@joelgriffith joelgriffith removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2020
joelgriffith pushed a commit that referenced this pull request Mar 18, 2020
* Move over to new plugin space, working implementation

* Fixing tests for report_listing snapshots

* WIP: Fixing react-component tests

* Fixing report_info_button tests

* Fixing download linksies

* WIP: Final working implementation

* Fixing attachAction API + API URLs

* Let the past die. Kill it if you have to. That’s the only way to become what you were meant to be.

* Fixing stream-client for new platform APIs

* Fixing types and tests

* Fix broken mock

* Adds back in warnings to report info button

* kibana.json line-breaks on required plugins

* Fixing broked snapshots

* Fix license checks in client-side components

* Adding back in warnings to report_listing component

* Fix danglig unused import

* Adds license checks for basic to our csv panel action

* Fixes issues from prior fork

* Move relative pathing to absolute

* Fix POST URL copying as we've moved from static methods

* Fix layoutId props

* Fixes types for layoutId

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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 release_note:skip Skip the PR/issue when compiling release notes RFC/final-comment-period If no concerns are raised in 3 business days this RFC will be accepted v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants