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 agg type shims and move paginated table to kibana_legacy #57695

Closed
wants to merge 6 commits into from

Conversation

flash1293
Copy link
Contributor

This PR fixes the default editor shim (some parts of legacy imports around agg types were actually stateful) and changes import from ui/agg_types to core_plugins/data where possible.

Additionally it moves the paginate directive to kibana_legacy to unblock the table directive in this regard.

@flash1293 flash1293 added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Feb 14, 2020
@flash1293 flash1293 marked this pull request as ready for review February 17, 2020 16:07
@flash1293 flash1293 requested a review from a team February 17, 2020 16:07
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Feb 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

services: {
dataShim: {
search: {
aggs: { types: aggTypes, aggTypeFieldFilters },
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about getting rid of aggTypeFieldFilters in the data search services?
The only thing it is responsible for is adding a filter in rollup and using it in the default editor.
This is pretty the same thing as we've already got rid of editorConfig registry in #56501
This is maybe a question to @lukeelmers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, this would simplify the setup as more stuff becomes static. We still need a stateful plugin for types to keep it extensible, but it takes away complexity. Also cc @ppisljar

Have there been thoughts about this already?

Copy link
Member

Choose a reason for hiding this comment

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

I'm currently working out the long term interface for agg types in #57064, which I'm hoping should be ready to merge by next week.

So far the items we will for sure be keeping in the lifecycle contract are types (registry of agg types) and createAggConfigs (a function that returns a new AggConfigs(), which will make the AggConfigs class itself internal to the data plugin).

Everything else is temporarily under a __LEGACY namespace while work out whether they are required or if they can be relocated/removed. For example, aggTypeFieldFilters is currently data.search.aggs.__LEGACY.aggTypeFieldFilters, but I agree @sulemanof, it may not be necessary to keep at all.

#57064 will be the best place to follow along, as many of these details should be sorted out in the coming days. Would welcome any input you have on that @sulemanof @flash1293 ... I'll probably add you as reviewers once I get it wrapped up 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it's not urgent I will postpone work on this PR till @lukeelmer's is merged. @kertal I will ping you once it becomes ready again :)

Copy link
Member

Choose a reason for hiding this comment

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

Just as an update here: #58462 and #58805 have both been merged this week, and I'm actively working now on the PR to cutover agg types to the new platform.

Since doing the cutover is the highest priority in terms of unblocking teams, we decided to postpone sorting out aggTypeFieldFilters and some of the other items under __LEGACY as it should be possible to address them after the service is in the new platform, since they are no longer relying on legacy world functionality.

As we clean up those APIs we will upstream affected downstream applications accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, as there are some parts that definitely will stay stateful anyway it's a very small change to make later on.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@sulemanof
Copy link
Contributor

Due to agg_types have been moved to new platform, I will take this up in another pr #60276

@sulemanof sulemanof closed this Mar 16, 2020
@cqliu1 cqliu1 mentioned this pull request Jun 25, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants