-
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
"Create index pattern" wizard. #12689
"Create index pattern" wizard. #12689
Conversation
@@ -1,139 +0,0 @@ | |||
import angular from 'angular'; |
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 removed this file because I think these types of assertions are better handled by a combination of functional tests and very narrowly-defined unit tests. I'll create unit tests for the new directives I've created.
@cjcenizal How about removing translations (from these templates) entirely? We need to do this anyway for all of the initial translations stuff when we re-approach phase 2 of the internationalization effort. |
As an aside, that'll help with all of those literal html tags you're seeing those screenshots :) |
5df3c5a
to
cc10b00
Compare
36e4caa
to
febb4f2
Compare
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.
Some early review feedback, since we're either going to need to remove the check for indices or find some other solution before this can move forward.
class="kuiVerticalRhythm" | ||
> | ||
<p class="kuiText kuiStatusText kuiStatusText--error kuiVerticalRhythmSmall"> | ||
Recent changes to Elasticsearch have made this option largerly unnecessary. It will likely be removed in a future version of Kibana. |
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.
It's safe to remove "likely" from this line, we're trying to figure out the soonest we can remove it right now elastic/elasticsearch#25577
id="indexPatternNameField" | ||
class="kuiTextInput kuiTextInput--large" | ||
data-test-subj="createIndexPatternNameInput" | ||
ng-model="stepIndexPattern.indexPatternName" |
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 really think this needs at least a small debounce. The loading indicator flicker is quite distracting and it results in a ton of unnecessary network communication since each keystroke results in three requests. the ng-model-options strategy in master should do the trick without many changes necessary.
this.fetchExistingIndices = () => { | ||
this.isFetchingExistingIndices = true; | ||
Promise.all([ | ||
indicesService.getIndices('*'), |
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.
This method doesn't return indices accessible via cross-cluster search, and I'm not certain we will be able to. Since the only APIs that are cross-cluster search enabled are the _search
and _field_caps
APIs we might be able to use a terms aggregation on _index
to find all of the index names, but that probably has pretty bad performance characteristics for a request that might be executing pretty often, and anyway it's blocked on elastic/elasticsearch#25606.
- Create a directive for each step in the wizard. - Reorganize files into conventional folder structure. - Rename files with more conventional and consistent naming patterns. - Improve translation key names. - Display indices, partial matches, exact matches. - Add loading, empty, and success states. - Add option to include system indices.
9c48ba4
to
a86fddd
Compare
I'm going to put this on hold and try to get it into the next release. CCSNotes for testing cross-cluster search:
ProblemsIf I used the index pattern
|
const searchResponse = await esAdmin.search({ | ||
index: indexPattern, | ||
ignore: [404], | ||
allow_no_indices: true, |
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.
Adding terminate_after: 1
here will make sure that es only reads a single document from each index, meaning that the search won't need to do more work than necessary and should make this as performant as possible.
I agree with @skearns64's comment - "The layout doesn't strongly connect the index pattern input and the search results" I think we can resolve this (to an extent) with suggestions from #12310 (comment) What are your thoughts around bolding (and possibly coloring) the indices where the index pattern matches? (in the current styling) |
Closing in favor of #13154 |
Replaces #12530.
Changes
Changes to the UX
Loading indices
Searching for matching indices
Submitting the new index pattern
To-do later
Feedback
From @skearns64
From @alexfrancoeur