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/New Platform Migration] Use a new config service on server-side (re-do) #61696

Closed
wants to merge 8 commits into from
Closed

[Reporting/New Platform Migration] Use a new config service on server-side (re-do) #61696

wants to merge 8 commits into from

Conversation

joelgriffith
Copy link
Contributor

Summary

Re-introduces the reporting config change that was reverted mid-week: #61075

Original PR: #55882.

This PR has successfully undergone the flaky-test runner: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/312/testReport/

@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 v7.8.0 labels Mar 27, 2020
@joelgriffith
Copy link
Contributor Author

Goal here is to re-run the full CI process a few times.

@joelgriffith
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@joelgriffith
Copy link
Contributor Author

@elasticmachine merge upstream

@tsullivan
Copy link
Member

@elasticmachine merge upstream

@tsullivan tsullivan changed the title Reporting/np migration server config [Reporting/New Platform Migration] Use a new config service on server-side (re-do) Mar 30, 2020
@jbudz
Copy link
Member

jbudz commented Mar 30, 2020

@elasticmachine merge upstream

@tsullivan
Copy link
Member

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Some NITs from the Telemetry end :)

@@ -11,16 +11,17 @@ import { PluginStart as DataPluginStart } from '../../../../../src/plugins/data/
import { SecurityPluginSetup } from '../../../../plugins/security/server';
import { XPackMainPlugin } from '../../xpack_main/server/xpack_main';
import { ReportingPluginSpecOptions } from '../types';
import { ReportingConfig, ReportingConfigType } from './core';

export interface ReportingSetupDeps {
elasticsearch: ElasticsearchServiceSetup;
security: SecurityPluginSetup;
usageCollection: UsageCollectionSetup;
Copy link
Member

Choose a reason for hiding this comment

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

In the kibana.json, usageCollection is an "optionalDependency", which is the way to go.
ReportingSetupDeps should define it as an optional dependency.

exportTypesRegistry,
collectionIsReady
);
usageCollection.registerCollector(collector);
plugins.usageCollection.registerCollector(collector);
Copy link
Member

Choose a reason for hiding this comment

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

plugins.usageCollection might be undefined

@@ -93,51 +90,6 @@ export type ReportingResponseToolkit = Legacy.ResponseToolkit;

export type ESCallCluster = CallCluster;
Copy link
Member

Choose a reason for hiding this comment

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

Type-wise it should be the same and there shouldn't be any issues but CallCluster is now APICaller in NP from 'src/core/server'

@tsullivan
Copy link
Member

Thanks for the comments, @afharo! Unfortunately I'm going to have to close this PR. It was having problems from added instability, so we are working on making these changes through a series of smaller PRs. The first one is here: #62086

@tsullivan
Copy link
Member

tsullivan commented Apr 3, 2020

Replaced with #62500

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 v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants