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

Add esTypes property to index pattern field #35251

Merged
merged 11 commits into from
Apr 22, 2019
Merged

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Apr 17, 2019

Summary

Adds an array property to the Field object containing all of the types the field is mapped as across ES indices.

Checklist

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

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@Bargs Bargs added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.2.0 labels Apr 17, 2019
@Bargs Bargs requested review from timroes and lukasolson April 17, 2019 19:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@Bargs Bargs requested a review from TinaHeiligers April 17, 2019 19:08
@wylieconlon
Copy link
Contributor

This approach makes sense to me. It was surprising to me that this information was missing when working on the new editor. I assume this needs some kind of testing or migration plan to be ready, but I'm not super familiar with the requirements there.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs Bargs requested a review from a team April 18, 2019 22:13
@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs
Copy link
Contributor Author

Bargs commented Apr 19, 2019

Fixed and added tests and updated unit test fixtures and sample data.

However I did not touch the functional test fixtures. These can't be easily updated automatically afaik. We'd need to load up each data set, refresh the index pattern and save it to the archive in order to get accurate type information from the field_caps API. That's a lot of up front work when none of the current functional tests require this information, and they could be updated one by one as needed. This seems to be the approach taken when similar updates have been made, for example readFromDocValues is missing from some of the older test fixtures like discover.

Unless reviewers strongly disagree with the reasoning above, I'm pretty much done with the code once CI passes and this is ready for review.

@Bargs Bargs added the review label Apr 19, 2019
@Bargs Bargs changed the title [WIP] Add esTypes property to index pattern field Add esTypes property to index pattern field Apr 19, 2019
@Bargs Bargs added the release_note:skip Skip the PR/issue when compiling release notes label Apr 19, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM. One concern I have is that developers will start using this assuming it's available on all index patterns and not just ones you've refreshed. Not sure if a comment in the code will help or if we plan to actually do some sort of migration like Wylie suggested.

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

lgtm

@Bargs
Copy link
Contributor Author

Bargs commented Apr 22, 2019

I thought about that back when I was adding multi field info to the index pattern. Nathan thought it would be a bad idea to automatically refresh index patterns and I agreed. At least for now I think we should build features with the assumption that this info might not be available. The typescript definition marks this property as optional so as long as someone is using TS they'll be forced to handle the situation where this property is undefined. I'll add a comment to the TS definition explaining why it could be undefined.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs Bargs merged commit 03624a1 into elastic:master Apr 22, 2019
Bargs added a commit to Bargs/kibana that referenced this pull request Apr 22, 2019
Adds an array property to the Field object containing all of the types the field is mapped as across ES indices.
Bargs added a commit that referenced this pull request Apr 23, 2019
Adds an array property to the Field object containing all of the types the field is mapped as across ES indices.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes review Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants