-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Reporting/New Platform Migration] Use a new config service on server-side #55882
Conversation
33fa8d4
to
2775800
Compare
331e64e
to
f4daf24
Compare
f4daf24
to
0e980a8
Compare
@@ -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) { |
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 UI needs to be updated in-step with this change cc @joelgriffith
0e980a8
to
58196ce
Compare
4636a55
to
9f345ff
Compare
c6d213a
to
537e11d
Compare
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
794760d
to
cf384f6
Compare
9acaa92
to
80e00d9
Compare
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! Going to test this today and see how it all plays out.
@elasticmachine merge upstream |
Smoke tests looking good. Will merge once CI is green |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* 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) ...
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
…ervice on server-side (elastic#55882)" (elastic#61075)" This reverts commit 427848c.
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Reverted: #61075 |
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.The
getConfig
function returns an object with aget
method, and akbnConfig
object property that also has a get method. I'm usingconfig.get()
to read the Reporting config, andconfig.kbnConfig.get()
to read the global Kibana config. ThekbnConfig
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 isserver.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:Summary of the changes:
ReportingConfig
objectcreateJobFactory
/executeJobFactory
functions to be asyncReportingCore
object, don't pass theElasticsearchServiceSetup
API: useawait reporting.getElasticsearchService()
instead.createConfig$
to update defaults as needed, becauseconfig.set
is not available anymore.Todo
[ ] Accessthis is a TODO for after moving the code to NPpath.data
from New Platform APIfunction createConfig$
config.get()
andconfig.kbnConfig.get()
type-safe 🎉disableSandbox
code changes: https://github.com/elastic/kibana/pull/55882/files#r395927154Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This includes a feature addition or change that requires a release note and was labeled appropriately