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] Update some runtime validations #53975

Merged
merged 8 commits into from
Jan 9, 2020

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jan 4, 2020

Summary

Release note: added a self-check for Reporting to prevent an incompatible config from stopping Reporting from working.

  • Unskip a Mocha test
  • Add a runtime validation for incompatible server.host: "0"

Checklist

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

@elasticmachine
Copy link
Contributor

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

@tsullivan
Copy link
Member Author

This PR would be a good time to add string tags for translation in this validation files

@tsullivan tsullivan force-pushed the reporting/validate-server-host branch from 78dca6a to 7bf9f8b Compare January 7, 2020 00:49
const reportingKibanaHostName = config.get(configKey);

if (!reportingKibanaHostName && serverHost === '0') {
// @ts-ignore: No set() method on KibanaConfig, just get() and has()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is brought up as a concern in the Reporting migration issue: #53898

@tsullivan
Copy link
Member Author

ready for review

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.

Just a small note on potential breakages in 7.6.0 with the new validation. Reporting might not work, but I'd hate for us to prevent kibana from starting due to breaking config change.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

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

@tsullivan tsullivan merged commit c2362d4 into elastic:master Jan 9, 2020
@tsullivan tsullivan deleted the reporting/validate-server-host branch January 9, 2020 17:14
tsullivan added a commit to tsullivan/kibana that referenced this pull request Jan 9, 2020
* [Reporting] Update some runtime validations

* fix unit test

* i18n

* make warning logging of encryptionKey possible

* update snapshot

* revert unrelated config change
tsullivan added a commit that referenced this pull request Jan 9, 2020
* [Reporting] Update some runtime validations

* fix unit test

* i18n

* make warning logging of encryptionKey possible

* update snapshot

* revert unrelated config change
@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
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:enhancement review v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants