Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
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

@joelgriffith joelgriffith commented Mar 27, 2020

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 zDeprecated 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

joelgriffith commented Mar 27, 2020

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

@joelgriffith
Copy link
Contributor Author

joelgriffith commented Mar 27, 2020

@elasticmachine merge upstream

1 similar comment
@joelgriffith
Copy link
Contributor Author

joelgriffith commented Mar 27, 2020

@elasticmachine merge upstream

@tsullivan
Copy link
Member

tsullivan commented Mar 30, 2020

@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 tsullivan mentioned this pull request Mar 31, 2020
Merged
2 tasks
@tsullivan
Copy link
Member

tsullivan commented Apr 3, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

kibanamachine commented Apr 3, 2020

💚 Build Succeeded

History

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

afharo
afharo reviewed Apr 3, 2020
View reviewed changes
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 :)

x-pack/legacy/plugins/reporting/server/types.d.ts
export interface ReportingSetupDeps {
elasticsearch: ElasticsearchServiceSetup;
security: SecurityPluginSetup;
usageCollection: UsageCollectionSetup;
Copy link
Member

@afharo afharo Apr 3, 2020

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.

x-pack/legacy/plugins/reporting/server/usage/reporting_usage_collector.ts
collectionIsReady
);
usageCollection.registerCollector(collector);
plugins.usageCollection.registerCollector(collector);
Copy link
Member

@afharo afharo Apr 3, 2020

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

x-pack/legacy/plugins/reporting/types.d.ts

export type ReportingResponseToolkit = Legacy.ResponseToolkit;

export type ESCallCluster = CallCluster;
Copy link
Member

@afharo afharo Apr 3, 2020

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

tsullivan commented Apr 3, 2020

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 tsullivan closed this Apr 3, 2020
@tsullivan tsullivan mentioned this pull request Apr 3, 2020
Merged
3 tasks
@tsullivan
Copy link
Member

tsullivan commented Apr 3, 2020 •
edited
Loading

Replaced with #62500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@afharo afharo afharo left review comments

@tsullivan tsullivan Awaiting requested review from tsullivan

@bmcconaghy bmcconaghy Awaiting requested review from bmcconaghy

Assignees

No one assigned

Labels

release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0 zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joelgriffith @tsullivan @jbudz @kibanamachine @afharo @elasticmachine