-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Index Patterns Management - use /_resolve
endpoint for data streams support
#70271
Index Patterns Management - use /_resolve
endpoint for data streams support
#70271
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
@elasticmachine merge upstream |
…kibana into index_pattern_mgmt_resolve_api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI design looks good! Thanks for making the edits!
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (chrome macos), and it works great!
Added a few comments but code overall LGTM
...ex_pattern_wizard/components/step_index_pattern/components/status_message/status_message.tsx
Outdated
Show resolved
Hide resolved
...mponents/create_index_pattern_wizard/__snapshots__/create_index_pattern_wizard.test.tsx.snap
Outdated
Show resolved
Hide resolved
...ex_pattern_wizard/components/step_index_pattern/components/status_message/status_message.tsx
Outdated
Show resolved
Hide resolved
...ern_management/public/components/create_index_pattern_wizard/create_index_pattern_wizard.tsx
Outdated
Show resolved
Hide resolved
{ canPreselect, timeFieldName }: { canPreselect: boolean; timeFieldName?: string }, | ||
matchedItem | ||
) => { | ||
const dataStreamItem = matchedItem.item as ResolveIndexResponseItemDataStream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Would be nice to use typeguards to differentiate between the different possible matchedItem.item
types so that you don't need casting here.
Or perhaps make MatchedItem
generic so that you can optionally specify an item type if it is known at compile time:
type MatchedItemTypes =
| ResolveIndexResponseItemIndex
| ResolveIndexResponseItemAlias
| ResolveIndexResponseItemDataStream;
export interface MatchedItem<I extends MatchedItemTypes = MatchedItemTypes> {
name: string;
tags: Tag[];
item: I;
}
// usage:
type MatchedDataStreamItem = MatchedItem<ResolveIndexResponseItemDataStream>;
export const canPreselectTimeField = (indices: MatchedDataStreamItem[]) => {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just made the MatchedItem['item'] definition more flexible. I think the larger problem is that I'm trying to determine a specific type from a general type. It seems that typescript will allow me to write a function to do it but that seems like overkill just to check a single field. The best fix would be to track Data source items in their own array but I'd rather not make that change at this stage.
@@ -106,22 +111,43 @@ export const Header: React.FC<HeaderProps> = ({ | |||
isInvalid={isInputInvalid} | |||
onChange={onQueryChanged} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be debouncing this?
While testing locally I noticed that we are firing a new HTTP request on every keystroke, which can be a lot if you are typing a longer index pattern name.
To be clear, this behavior existed before this PR, but since we are updating related behavior here I wonder if it'd be worth throwing this in a lodash debounce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted this but the debounce functionality was becoming troublesome to implement since it was conflicting with the code that appends a *
. I'll try to address in a separate PR. If nothing else, the calls to _resolve/indices
are much more performant than running queries as the code previously did.
…dd pluralization, typescript improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM, pending green build!
I did not retest in a browser to verify the pluralization changes, but the syntax looks correct AFAICT.
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we got pinged due to a change in the rollup plugin, but the code in question pertains to index patterns. I'm going to approve without reviewing to dismiss the codeowner requirement.
… support (elastic#70271) * Index Patterns Management - use `/_resolve` endpoint for data streams support
Summary
Addresses #70271 #61368 #67329 #61368
_/resolve/indices
endpoint - Resolve index API elasticsearch#57626Step 1 - define pattern
Before
After
Step 1 - matched items
Before
After
Step 2
Before
After
Useful commands for creating data streams. Create an index template, a data stream, and then add aa doc to create the index behind the data stream
Create an alias
Checklist
Delete any items that are not applicable to this PR.