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

Fix: Run with reporting disabled #25829

Merged
merged 4 commits into from
Nov 28, 2018

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Nov 16, 2018

Closes #25728

Fixes reporting import and config checking.

  • Imports from file directly, instead of statically via the plugins alias
  • Wraps config.get so it doesn't throw when reporting is disabled

To test, simply set xpack.reporting.enabled: false in your kibana.yml. Previously, it would fail to build, and crash the server even if it would.

screenshot 2018-11-16 14 06 43

@w33ble w33ble added review v7.0.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.6.0 v6.5.2 labels Nov 16, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rashidkpc
Copy link
Contributor

  • Reproduced
  • Checked that Kibana starts
  • Checked that attempting to export a report when reporting is disabled fails:

screen shot 2018-11-19 at 2 56 53 pm

@stacey-gammon
Copy link
Contributor

Why is the error message calling out Chromium instead of the enabled/disabled setting? Just might be confusing since chromium is the default now, so they shouldn't actually have to add that, just make sure it's not set to phantom, and make sure it's not disabled.

@w33ble
Copy link
Contributor Author

w33ble commented Nov 28, 2018

@stacey-gammon yeah, I was thinking about that too. Planning to make a follow-on PR that shows a different message when Reporting is just disabled. Opened #26357 to track it.

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Just FYI: your test plan states one should

set xpack.reporting.disabled

That threw an error for me. Did a quick search, found it was xpack.reporting.enabled: false. :-)

Otherwise, all went well. Apologies for my delay in review...!

@w33ble
Copy link
Contributor Author

w33ble commented Nov 28, 2018

@clintandrewhall I really should proof-read what I write I guess, heh. Thanks for the review.

@w33ble w33ble force-pushed the fix/run-with-reporting-disabled branch from 0a1196f to 50cff8d Compare November 28, 2018 20:37
the code was duplicated from canvas, so it's equally as broken, this duplicates the fix
@w33ble
Copy link
Contributor Author

w33ble commented Nov 28, 2018

Quick refactor and addition after testing on master. It was broken there too, in a different way, thanks to code duplication (as mentioned in #26375).

Was seeing this error:

server   error  [21:02:08.523]  Error: Unknown config key: xpack.reporting.capture.browser.type
    at Config.get (/repos/kibana/src/server/config/config.js:140:15)
    at server.injectUiAppVars (/repos/kibana/src/core_plugins/interpreter/init.js:28:41)
    at injectedVarProviders.filter.reduce (/repos/kibana/src/ui/ui_apps/ui_apps_mixin.js:62:18)
    at <anonymous>

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@w33ble w33ble merged commit b020c20 into elastic:master Nov 28, 2018
w33ble added a commit to w33ble/kibana that referenced this pull request Nov 28, 2018
* fix: relative link instead of static

* fix: handle missing reporting config value

* chore: refactor reportingBrowserType check

* fix: core interpreter reportingBrowserType

the code was duplicated from canvas, so it's equally as broken, this duplicates the fix
w33ble added a commit to w33ble/kibana that referenced this pull request Nov 28, 2018
* fix: relative link instead of static

* fix: handle missing reporting config value

* chore: refactor reportingBrowserType check

* fix: core interpreter reportingBrowserType

the code was duplicated from canvas, so it's equally as broken, this duplicates the fix
w33ble added a commit that referenced this pull request Nov 29, 2018
* fix: relative link instead of static

* fix: handle missing reporting config value

* chore: refactor reportingBrowserType check

* fix: core interpreter reportingBrowserType

the code was duplicated from canvas, so it's equally as broken, this duplicates the fix
@w33ble
Copy link
Contributor Author

w33ble commented Nov 29, 2018

6.5 fe7b69b
6.x c658f44

w33ble added a commit that referenced this pull request Nov 29, 2018
* fix: relative link instead of static

* fix: handle missing reporting config value

* chore: refactor reportingBrowserType check

* fix: core interpreter reportingBrowserType

the code was duplicated from canvas, so it's equally as broken, this duplicates the fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.5.2 v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants