-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Allowing {index}/_xpack/rollup/data to accept comma delimited list #34115
Allowing {index}/_xpack/rollup/data to accept comma delimited list #34115
Conversation
Pinging @elastic/es-search-aggs |
Jenkins, retest this please |
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.
Good catch @benwtrent ! I left some comments
--- | ||
"Verify job caps by rollup index comma delimited list": | ||
|
||
- do: |
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.
You'll need to add a skip section to not run this test on version that don't have the fix:
- skip:
version: " - 6.99.99"
reason: "comma delimited index support was fixed in 7.0"
After the backport to 6x we'll change the skip version to 6.4.99.
@@ -27,7 +28,8 @@ public RestGetRollupIndexCapsAction(Settings settings, RestController controller | |||
protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) { | |||
String index = restRequest.param(INDEX.getPreferredName()); | |||
IndicesOptions options = IndicesOptions.fromRequest(restRequest, IndicesOptions.STRICT_EXPAND_OPEN_FORBID_CLOSED); | |||
GetRollupIndexCapsAction.Request request = new GetRollupIndexCapsAction.Request(new String[]{index}, options); | |||
GetRollupIndexCapsAction.Request request = | |||
new GetRollupIndexCapsAction.Request(Strings.commaDelimitedListToStringArray(index), options); |
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.
Can you use Strings.splitStringByCommaToArray
, it should be equivalent but that's the function used by the other action when they parse the index
param.
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.
LGTM, thanks @benwtrent
…34115) * Allowing `{index}/_xpack/rollup/data` to accept comma delimited list * Address PR comments
…34115) * Allowing `{index}/_xpack/rollup/data` to accept comma delimited list * Address PR comments
{index}/_xpack/rollup/data is supposed to be able to return given a comma delimited list of indices, there was a slight oversight on the REST facing side that caused index lookups to be the literal string and not an expanded list from the parameter.
I apologize if the labels are not correct, this is my first bug PR referencing a released version.