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

[indexPattern] Added #toDetailedIndexList() #5638

Merged

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Dec 10, 2015

indexPattern#toIndexList() now has a detailedResponse

We need access to the bounds for each index, so this new parameter returns the indices as objects rather than strings, and each object details the index, start, and end for each index.

@@ -177,18 +177,21 @@ define(function (require) {
return this.intervalName && _.find(intervals, { name: this.intervalName });
};

self.toIndexList = function (start, stop, sortDirection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than adding complexity to this function by making it polymorphic, how about we move all of this logic into a new toDetailedIndexList function, and then modify the existing toIndexList to do something like:

self.toIndexList = function (start, stop, sortDirection) {
  return this.toDetailedIndexList()
  .then(indices => return _.pluck(indices, 'index'));
};

@spalger spalger force-pushed the implement/indexPatternToIndexListDetailedResponse branch 2 times, most recently from 64ec792 to 6805797 Compare December 10, 2015 20:57
@spalger spalger changed the title [indexPattern/toIndexList] Added detailedResponse param [indexPattern/toIndexList] Added #toDetailedIndexList() param Dec 10, 2015
@spalger spalger changed the title [indexPattern/toIndexList] Added #toDetailedIndexList() param [indexPattern] Added #toDetailedIndexList() Dec 10, 2015
@epixa
Copy link
Contributor

epixa commented Dec 10, 2015

Can you give a bit of a summary for the best way to test this? I can review the code and such, but I'm not really sure what behaviors I should be testing in kibana itself.

@epixa
Copy link
Contributor

epixa commented Dec 10, 2015

Or is there a ticket associated with this that describes the issue?

@spalger
Copy link
Contributor Author

spalger commented Dec 10, 2015

There isn't actually any issue, and the new method isn't used in Kibana at the time being. This is an API to support new functionality I'm working on. For now only the tests test it.

@spalger spalger force-pushed the implement/indexPatternToIndexListDetailedResponse branch 2 times, most recently from df41329 to 2825c8d Compare December 10, 2015 23:39
@spalger spalger force-pushed the implement/indexPatternToIndexListDetailedResponse branch from 2825c8d to e63838d Compare December 10, 2015 23:40
@epixa
Copy link
Contributor

epixa commented Dec 11, 2015

Ah, that makes a lot more sense.

.value();
// TODO: remove when we get to es 2.2, see elastic/elasticsearch#14404
let min = field.min_value;
if (typeof min === 'string') min = moment(min).valueOf();
Copy link
Contributor

Choose a reason for hiding this comment

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

Will these definitely always be strings? Or more specifically, is it possible for them to be integer timestamps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After we upgrade to es 2.2, elasticsearch will always pass back unix timestamps. Until then times are passed back in whatever format they are stored in, and our only option is to attempt to parse that with moment()

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes complete sense. I totally had this reversed in my head.

@epixa
Copy link
Contributor

epixa commented Dec 11, 2015

If those constraints will always be strings or moment objects, then I'd say this LGTM.

@epixa epixa assigned spalger and unassigned epixa Dec 11, 2015
@spalger spalger assigned jbudz and unassigned spalger Dec 11, 2015
@jbudz
Copy link
Member

jbudz commented Dec 11, 2015

LGTM

@jbudz jbudz assigned spalger and unassigned jbudz Dec 11, 2015
epixa added a commit that referenced this pull request Dec 11, 2015
…stDetailedResponse

[indexPattern] Added #toDetailedIndexList()
@epixa epixa merged commit c1e8546 into elastic:master Dec 11, 2015
@elasticsearch-bot
Copy link

Court Ewing merged this into the following branches!

Branch Commits
4.x 14c70b3, 31a83f0
4.3 c5c0cf6, ccfce44
4.3.1 1f82968, 230555c

spalger pushed a commit that referenced this pull request Dec 11, 2015
spalger pushed a commit that referenced this pull request Dec 11, 2015
spalger pushed a commit that referenced this pull request Dec 11, 2015
spalger pushed a commit that referenced this pull request Dec 11, 2015
spalger pushed a commit that referenced this pull request Dec 11, 2015
spalger pushed a commit that referenced this pull request Dec 11, 2015
@spalger spalger deleted the implement/indexPatternToIndexListDetailedResponse branch February 25, 2016 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants