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 #55882

Merged
merged 6 commits into from
Mar 23, 2020

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jan 24, 2020

Part of #53898

This PR integrates the ReportingConfig service. based on a New-Platform schema, throughout server-side Reporting. This removes the biggest legacy dependency in Reporting.

Getting access to the config requires access to the ReportingCore object on the server.

async function getConfig(reportingCore: ReportingCore) {
   return await reportingCore.getConfig();
}

The getConfig function returns an object with a get method, and a kbnConfig object property that also has a get method. I'm using config.get() to read the Reporting config, and config.kbnConfig.get() to read the global Kibana config. The kbnConfig part is to keep the call signatures about the same as the legacy code, when reading the global config. For example, to get the server host name, the legacy call is server.config().get('server.host') and now it's: config.kbnConfig.get('server', 'host'). And it is also type-safe unlike the legacy calls.

Because the access is asynchronous, the code tries to strip the config out of the reportingCore object when passing down to other modules, such as routes registration. This is to avoid the need to convert those modules to asynchronous.

As part of the new accessor on the ReportingCore object, this PR also adds an accessor for the Elasticsearch service:

async function getElasticsearch(reportingCore: ReportingCore) {
  return await reportingCore.getElasticsearchService();
}

Summary of the changes:

  • Remove legacy config code
  • Add new config schema to NP Reporting plugin
  • Change function signatures to accept the ReportingConfig object
  • Change createJobFactory / executeJobFactory functions to be async
  • For async functions that accept the ReportingCore object, don't pass the ElasticsearchServiceSetup API: use await reporting.getElasticsearchService() instead.
  • Add createConfig$ to update defaults as needed, because config.set is not available anymore.

Todo

  • [ ] Access path.data from New Platform API this is a TODO for after moving the code to NP
  • Unit test for the new schema declaration
  • Unit test for function createConfig$
  • Unit test for validating the server host
  • Unit test for random encryption key generation
  • I18N for the modified files 🎉
  • Make config.get() and config.kbnConfig.get() type-safe 🎉
  • Resolution on comments about the disableSandbox code changes: https://github.com/elastic/kibana/pull/55882/files#r395927154

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@tsullivan tsullivan force-pushed the reporting/np-server-config branch 2 times, most recently from 33fa8d4 to 2775800 Compare January 24, 2020 18:56
@tsullivan tsullivan changed the title [Reporting ] [Reporting/New Platform Migration] Use a new config service on server-side Jan 24, 2020
@tsullivan tsullivan force-pushed the reporting/np-server-config branch 3 times, most recently from 331e64e to f4daf24 Compare January 28, 2020 20:26
@tsullivan tsullivan force-pushed the reporting/np-server-config branch from f4daf24 to 0e980a8 Compare January 28, 2020 20:35
@@ -45,13 +43,6 @@ export const reporting = (kibana: any) => {
embeddableActions: ['plugins/reporting/panel_actions/get_csv_panel_action'],
home: ['plugins/reporting/register_feature'],
managementSections: ['plugins/reporting/views/management'],
injectDefaultVars(server: Legacy.Server, options?: ReportingConfigOptions) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The UI needs to be updated in-step with this change cc @joelgriffith

@tsullivan tsullivan force-pushed the reporting/np-server-config branch from 0e980a8 to 58196ce Compare January 28, 2020 20:45
@tsullivan tsullivan force-pushed the reporting/np-server-config branch 3 times, most recently from 4636a55 to 9f345ff Compare January 28, 2020 23:17
@tsullivan tsullivan mentioned this pull request Jan 29, 2020
19 tasks
@tsullivan tsullivan force-pushed the reporting/np-server-config branch 5 times, most recently from c6d213a to 537e11d Compare January 30, 2020 16:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan tsullivan force-pushed the reporting/np-server-config branch 6 times, most recently from 794760d to cf384f6 Compare February 18, 2020 22:30
@tsullivan tsullivan force-pushed the reporting/np-server-config branch 2 times, most recently from 9acaa92 to 80e00d9 Compare February 19, 2020 22:15
Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

LGTM! Going to test this today and see how it all plays out.

@joelgriffith
Copy link
Contributor

@elasticmachine merge upstream

@joelgriffith
Copy link
Contributor

Smoke tests looking good. Will merge once CI is green

@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 5755b2a into elastic:master Mar 23, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 24, 2020
* master: (34 commits)
  [APM] add service map config options to legacy plugin (elastic#61002)
  [App Arch] migrate legacy CSS to new platform (core_plugins/kibana_react) (elastic#59882)
  Migrated styles for "share" plugin to new platform (elastic#59981)
  [ML] Module setup with dynamic model memory estimation (elastic#60656)
  Drilldowns (elastic#59632)
  Upgrade mocha dev-dependency from 6.2.2 to 7.1.1 (elastic#60779)
  [SIEM] Overview: Recent cases widget (elastic#60993)
  [ML] Functional tests - stabilize df analytics clone tests (elastic#60497)
  [SIEM] Updates process and TLS tables to use ECS 1.5 fields (elastic#60854)
  Migrate doc view part of discover (elastic#58094)
  Revert "[APM] Collect telemetry about data/API performance (elastic#51612)"
  fix(NA): log rotation watchers usage (elastic#60956)
  [SIEM] [CASES] Build lego blocks case details view (elastic#60864)
  Create Painless Lab app (elastic#57538)
  [SIEM] Move Timeline Template field to first step of rule creation (elastic#60840)
  [Reporting/New Platform Migration] Use a new config service on server-side (elastic#55882)
  [Alerting] allow email action to not require auth (elastic#60839)
  [Maps] Default ES document layer scaling type to clusters and show scaling UI in the create wizard (elastic#60668)
  [APM] Collect telemetry about data/API performance (elastic#51612)
  Implement Kibana Login Selector (elastic#53010)
  ...
joelgriffith pushed a commit that referenced this pull request Mar 24, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 55882 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 25, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 55882 or prevent reminders by adding the backport:skip label.

@tsullivan tsullivan deleted the reporting/np-server-config branch March 30, 2020 17:58
tsullivan added a commit to tsullivan/kibana that referenced this pull request Mar 30, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 55882 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 55882 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 55882 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 55882 or prevent reminders by adding the backport:skip label.

@tsullivan tsullivan added the backport:skip This commit does not require backporting label Apr 9, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 9, 2020
@tsullivan
Copy link
Member Author

Reverted: #61075

@sophiec20 sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting (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 reverted v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants