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 bug in xpackInfo in which keys were being camel-cased during refresh but not init #29304

Merged
merged 5 commits into from
Jan 29, 2019

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jan 25, 2019

This fixes a bug in a CCR feature we're working on, but introduces an error in Beats Management:

image

This error comes from this line: https://github.com/elastic/kibana/blob/master/x-pack/plugins/beats_management/public/lib/adapters/framework/kibana_framework_adapter.ts#L114

It stems from the fact that Beats Management attempts to retrieve xpackInfo using its plugin ID, beats_management.

I can think of a couple solutions:

  1. Beats management changes its implementation to use a camel-cased ID to retrieve its xpack info.
  2. We change xpackInfo's implementation to not camel-case the keys its using. I'm somewhat leaning towards this solution since it seems like surprising behavior for it to alter the data its receiving. But I'd like to get Shaunak's perspective first to make sure we don't lose anything by doing so.

@cjcenizal cjcenizal added bug Fixes for quality problems that affect the customer experience v7.0.0 v6.7.0 labels Jan 25, 2019
@cjcenizal
Copy link
Contributor Author

cjcenizal commented Jan 25, 2019

Tagging Shaunak for review because he originally introduced the camel-casing logic in X-Pack.

@cjcenizal cjcenizal requested review from ycombinator and removed request for lukasolson January 25, 2019 00:19
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ycombinator
Copy link
Contributor

I don't know the full context of this PR but I can certainly try to explain a bit of history on where the camel-casing logic came from, if that might help:

At the time the xpackInfo functionality was developed (circa June 2016 IIRC), Kibana had a convention of using snake_case in API response bodies. And it also had the convention of using camelCase in JS object structures. The xpackInfo data structure lives as a JS object on the server-side, is serialized to JSON and transported to the client-side via an HTTP API. Hence the logic of changing the case back and forth.

Hope that helps!

@mattapperson
Copy link
Contributor

Seems to me we often still try to keep this convention. So for that reason I would lean to BeatsCM changing. Just curious if that’s a convention we WANT to keep, or one we have kept just because.

@cjcenizal
Copy link
Contributor Author

@mattapperson Sounds good, I agree. Would you mind submitting a PR into this one to update Beats?

@cjcenizal
Copy link
Contributor Author

CI should go green once Beats is updated.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

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

Code LGTM

@cjcenizal
Copy link
Contributor Author

Retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elastic elastic deleted a comment from elasticmachine Jan 29, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cjcenizal
Copy link
Contributor Author

CI went green and I only pushed a code comment, so I'm going to merge without waiting for CI to pass again.

@cjcenizal cjcenizal merged commit 89733ae into elastic:master Jan 29, 2019
@cjcenizal cjcenizal deleted the bug/xpack-info-camel-casing branch January 29, 2019 23:48
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Jan 30, 2019
…esh but not init (elastic#29304)

* Update Beats Management to use camelCased plugin ID to access xpackInfo.
cjcenizal added a commit that referenced this pull request Jan 30, 2019
…esh but not init (#29304) (#29568)

* Update Beats Management to use camelCased plugin ID to access xpackInfo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker bug Fixes for quality problems that affect the customer experience v6.7.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants