-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Reporting] Wholesale moves client to newest-platform #58945
Conversation
max_attempts: number; | ||
csv_contains_formulas: boolean; | ||
} | ||
|
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core setup is here.
…me what you were meant to be.
import { downloadReport } from '../lib/download_report'; | ||
import { jobQueueClient, JobQueueEntry } from '../lib/job_queue_client'; | ||
|
||
import { ToastsSetup, ApplicationStart } from '../../'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)
There was a problem hiding this 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.
x-pack/plugins/reporting/index.d.ts
Outdated
@@ -13,6 +13,8 @@ import { | |||
NotificationsStart, | |||
} from '../../../src/core/public'; | |||
|
|||
export { ToastsSetup, HttpSetup, ApplicationStart, IUiSettingsClient } from 'src/core/public'; |
There was a problem hiding this comment.
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 '../../'; |
There was a problem hiding this comment.
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 '../../'; |
There was a problem hiding this comment.
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 '../../'; |
There was a problem hiding this comment.
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 '../..'; |
There was a problem hiding this comment.
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 '../../'; |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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';
Imports have been fixed, carry on |
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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.
@elasticmachine merge upstream |
There was a problem hiding this 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.
GREEN BUILD, GREEN BUILD! |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* 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) ...
* 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) ...
7.x PR: #60437 |
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. |
* 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>
Summary
Moves all of our UI-facing architecture to newest platform.
TODO
QA List