-
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] Download/install the browser in the setup phase #113212
[Reporting] Download/install the browser in the setup phase #113212
Conversation
41219f8
to
d1c71e8
Compare
d1c71e8
to
cbbce1b
Compare
paths: ChromiumArchivePaths; | ||
} | ||
|
||
export const initializeBrowserDriverFactory = async (core: ReportingCore, logger: LevelLogger) => { |
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.
chromium.createDriverFactory
is a one-liner now, so this helper function isn't helpful :)
|
||
logger.info(`Browser executable: ${binaryPath}`); | ||
|
||
resolve(binaryPath); |
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 previous code didn't have a lot of reason for using an observable here.
cbbce1b
to
2de3595
Compare
constructor(core: ReportingCore, binaryPath: string, logger: LevelLogger) { | ||
this.core = core; | ||
this.binaryPath = binaryPath; | ||
const config = core.getConfig(); |
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.
There are a lot of changes in this code due to core.getConfig
can only be called after the setup phase of the Reporting plugin.
In other words, the ReportingConfig
object is asynchronously instantiated due to being implemented before #88981
2de3595
to
b12eb43
Compare
@elasticmachine merge upstream |
12f2f1c
to
effdecf
Compare
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / jest / Jest Tests.x-pack/plugins/reporting/server/export_types/png_v2.returns content of generatePng getBuffer base64 encodedStandard Out
Stack Trace
Kibana Pipeline / jest / Jest Tests.x-pack/plugins/reporting/server/routes/diagnostic.POST /diagnose/screenshot returns a 200 by defaultStandard Out
Stack Trace
Kibana Pipeline / jest / Jest Tests.x-pack/plugins/reporting/server/routes/diagnostic.POST /diagnose/screenshot returns a 200 when it fails and sets success to falseStandard Out
Stack Trace
and 10 more failures, only showing the first 3. Metrics [docs]Public APIs missing comments
Public APIs missing exports
History
To update your PR or re-run it, just comment with: |
Closing this as Mike Dokolin is working on this part of the code. I'll just keep a bookmark to this draft |
Summary
Problem: Something that would have made working on #112905 better, is if the browser drivers were initialized in the
setup
phase of the Reporting plugin, rather than thestart
. That would allowed dev to work through this without the need to have Elasticsearch connected.This PR makes Reporting register its browser driver factory register in the plugin
setup
method, which means not waiting for Elasticsearch to start before checking the existence of the browser and validating the checksum.