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

[Logs UI][Metrics UI] Replace usage of deprecated IndexPattern types #114448

Conversation

miltonhultgren
Copy link
Contributor

@miltonhultgren miltonhultgren commented Oct 11, 2021

Summary

Closes #107887

Replaces usages of the following deprecated types in the Infra plugin code:
IIndexPattern
IndexPattern
IndexPatternField
IndexPatternsContract

Concerns:
Not sure what to set 'type' to, it's set to undefined unless the index is for a rollup
The data plugin still uses the old IndexPattern types so for now I cast to any but that feels bad

Follow up:
We'll need to rename our functions/variables to match the new concepts as everything is still called "index pattern" in our code

@miltonhultgren miltonhultgren self-assigned this Oct 11, 2021
@miltonhultgren miltonhultgren added auto-backport Deprecated - use backport:version if exact versions are needed Feature:Logs UI Logs UI feature Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.16.0 v8.0.0 labels Oct 11, 2021
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

At first glance this looks great. 👍 I left just two questions below.

@@ -82,7 +83,7 @@ class WithKueryAutocompletionComponent extends React.Component<
query: expression,
selectionStart: cursorPosition,
selectionEnd: cursorPosition,
indexPatterns: [indexPattern],
indexPatterns: [indexPattern as any], // How to fix this without changing the dependency?
Copy link
Member

Choose a reason for hiding this comment

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

Instead of casting to any, how about we use a // @ts-expect-error? That could increase the likelihood that the person updating the getQuerySuggestions signature will be prompted by CI to remove the workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks!

@@ -5,7 +5,8 @@
* 2.0.
*/

import { IndexPattern, KBN_FIELD_TYPES } from '../../../../../../../src/plugins/data/public';
import { DataView } from '../../../../../../../src/plugins/data/common';
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep importing from public? I'm not sure cross-plugin imports from common into public are officially supported. In this case it seems easy to avoid them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that was sloppy of me!
It's because the data plugin doesn't export DataView from public, but in fact that type lives within the data_view plugin and there it is exported from public.
The data plugin only re-exports parts of the data_views types (like IndexPattern).

I'll fix that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the other files, they seem to pull mostly form /common, is that something I should look to change as well?

@miltonhultgren miltonhultgren marked this pull request as ready for review October 11, 2021 13:25
@miltonhultgren miltonhultgren requested a review from a team as a code owner October 11, 2021 13:25
@miltonhultgren miltonhultgren requested review from a team October 11, 2021 13:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@miltonhultgren
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
infra 507 271 -236

History

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

cc @miltonhultgren

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

It looks good and builds correctly, well done.

@miltonhultgren miltonhultgren merged commit 8ea719c into elastic:master Oct 12, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 12, 2021
…lastic#114448)

* [Logs UI][Metrics UI] Replace usage of deprecated IndexPattern types (elastic#107887)
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 12, 2021
…114448) (#114590)

* [Logs UI][Metrics UI] Replace usage of deprecated IndexPattern types (#107887)

Co-authored-by: Milton Hultgren <milton.hultgren@elastic.co>
artem-shelkovnikov pushed a commit to artem-shelkovnikov/kibana that referenced this pull request Oct 20, 2021
…lastic#114448)

* [Logs UI][Metrics UI] Replace usage of deprecated IndexPattern types (elastic#107887)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Logs UI Logs UI feature Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Replace references to IIndexPattern
4 participants