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
@@ -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 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.

tsullivan added a commit to tsullivan/kibana that referenced this pull request Apr 1, 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 added a commit to tsullivan/kibana that referenced this pull request Apr 3, 2020
commit 5107ad7
Merge: 41881bc 408baf2
Author: Elastic Machine <elasticmachine@users.noreply.github.com>
Date:   Fri Apr 3 00:29:11 2020 -0400

    Merge branch 'master' into reporting/np-migration-server-config

commit 408baf2
Author: Nathan L Smith <nathan.smith@elastic.co>
Date:   Thu Apr 2 16:44:29 2020 -0500

    Allow Enterprise license for service map (elastic#62371)

    We were previously only allowing "platinum" and "trial". Change this to allow `hasAtLeast('platinum')` which includes "platinum", "enterprise", and "trial".

    Fixes elastic#62243.

commit d10889a
Author: Brandon Morelli <brandon.morelli@elastic.co>
Date:   Thu Apr 2 14:19:40 2020 -0700

    docs: updates to apm agent config (elastic#61893)

commit 2ae566e
Author: Jen Huang <its.jenetic@gmail.com>
Date:   Thu Apr 2 13:04:32 2020 -0700

    [Ingest] Fix package info request returning 500 (elastic#61712)

    * Fix 500 package info request error

    * Fix installed packages page not loading

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

commit 4cb96ee
Author: Mikhail Shustov <restrry@gmail.com>
Date:   Thu Apr 2 21:41:56 2020 +0200

    move crypto to server utils (elastic#62344)

    * move crypto to server utils

    * fix mocks

commit 7039aba
Author: Mike Côté <mikecote@users.noreply.github.com>
Date:   Thu Apr 2 15:25:03 2020 -0400

    Start indexing documents by default (elastic#62159)

commit 6d8bdf6
Author: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
Date:   Thu Apr 2 14:59:50 2020 -0400

    [Endpoint] Update host field accordion (elastic#61878)

commit d639432
Author: Nicolas Ruflin <spam@ruflin.com>
Date:   Thu Apr 2 20:46:52 2020 +0200

    Add more definitions about Ingest Management (elastic#62049)

    This should help to explain the different terms.

    The docs directory was also renamed from epm to ingest_manager.

commit bb747ab
Author: Stacey Gammon <gammon@elastic.co>
Date:   Thu Apr 2 14:27:51 2020 -0400

    Switch to embeddable factory interface with optional override (elastic#61165)

    * wip

    * typescript map embeddable

    * More updates

    * Address code review comments and update some usages in SIEM and uptime to the new types

    * More clean up - carry over some of the SIEM types to maps for render tool tip

    * fixes

    * fixes

    * Address more review comments

    * fixes

    * fixes

    * fix jest test

    * Fix visualize embeddable

    * fixes after master merge

    * Fixes

    * Prefix variable with name "custom" to make it more obvious

    * Remove layerList from input state

    * fixes

    * Update src/plugins/dashboard/public/embeddable/dashboard_container_factory.tsx

    Co-Authored-By: Vadim Dalecky <streamich@users.noreply.github.com>

    * review updates

    * fixes

    * update maps readme

    Co-authored-by: Vadim Dalecky <streamich@users.noreply.github.com>

commit 09f1bae
Author: Peter Schretlen <peter.schretlen@elastic.co>
Date:   Thu Apr 2 14:23:09 2020 -0400

    fix text error in diagrams (elastic#62101)

commit 74d8413
Author: Sébastien Loix <sabee77@gmail.com>
Date:   Thu Apr 2 20:14:46 2020 +0200

    [Index management] Prepare support Index template V2 format (elastic#61588)

commit d6587d7
Author: Kaarina Tungseth <kaarina.tungseth@elastic.co>
Date:   Thu Apr 2 13:03:50 2020 -0500

    Updates dashboard images (elastic#62011)

commit 41881bc
Merge: b965d81 8b31ce0
Author: Elastic Machine <elasticmachine@users.noreply.github.com>
Date:   Mon Mar 30 18:51:56 2020 -0400

    Merge branch 'master' into reporting/np-migration-server-config

commit b965d81
Merge: 3997034 9ff8be6
Author: Elastic Machine <elasticmachine@users.noreply.github.com>
Date:   Mon Mar 30 17:19:48 2020 -0400

    Merge branch 'master' into reporting/np-migration-server-config

commit 3997034
Merge: 0414532 8d539aa
Author: Elastic Machine <elasticmachine@users.noreply.github.com>
Date:   Fri Mar 27 18:58:21 2020 -0400

    Merge branch 'master' into reporting/np-migration-server-config

commit 0414532
Merge: 412635e 878ab20
Author: Joel Griffith <joel.griffith@elastic.co>
Date:   Fri Mar 27 12:48:10 2020 -0700

    Merge remote-tracking branch 'upstream/master' into reporting/np-migration-server-config

commit 412635e
Merge: b1b2bfa 5a537d1
Author: Joel Griffith <joel.griffith@elastic.co>
Date:   Fri Mar 27 11:32:11 2020 -0700

    Merge remote-tracking branch 'upstream/master' into reporting/np-migration-server-config

commit b1b2bfa
Merge: 60220fb 9a53c08
Author: Joel Griffith <joel.griffith@elastic.co>
Date:   Thu Mar 26 09:22:52 2020 -0700

    Merge remote-tracking branch 'upstream/master' into reporting/np-migration-server-config

commit 60220fb
Author: Joel Griffith <joel.griffith@elastic.co>
Date:   Wed Mar 25 10:10:24 2020 -0700

    Revert "Revert "[Reporting/New Platform Migration] Use a new config service on server-side (elastic#55882)""

    This reverts commit 5c04d9e.
@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