Skip to content

Conversation

@romseygeek
Copy link
Contributor

Relates to #29827

@romseygeek romseygeek requested a review from hub-cap November 28, 2018 16:46
Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

Small nits, one question.


static Request executeWatch(ExecuteWatchRequest executeWatchRequest) throws IOException {
RequestConverters.EndpointBuilder builder = new RequestConverters.EndpointBuilder()
.addPathPart("_xpack", "watcher", "watch");
Copy link
Contributor

Choose a reason for hiding this comment

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

these can be .addPathPartAsIs since they are not user supplied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

RequestConverters.EndpointBuilder builder = new RequestConverters.EndpointBuilder()
.addPathPart("_xpack", "watcher", "watch");
if (executeWatchRequest.getId() != null) {
builder.addPathPart(executeWatchRequest.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

addPathPart wont "build" anything thats null, so u can just call it instead of checking for null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++


request.setActionMode("action1", ExecuteWatchRequest.ActionExecutionMode.SIMULATE);

String triggerData = "{ \"entry1\" : \"blah\", \"entry2\" : \"blah\" }";
Copy link
Contributor

Choose a reason for hiding this comment

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

are these mandatory? if they can be null pls add some random() here. If they cant be null then pls add them to the constructor as final's if possible :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

@romseygeek
Copy link
Contributor Author

Thanks @hub-cap, I pushed some changes to address your comments.

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

one minor nit. Also, something to note is that there is a PR that is conflicting with this that removes _xpack from the HLRC and tests etc... If it merges first, ill link it to you, otherwise you win and I have to do the work ;)

#36218

builder.addPathPart(executeWatchRequest.getId());
}
.addPathPartAsIs("_xpack", "watcher", "watch");
builder.addPathPart(executeWatchRequest.getId()); // will ignore if ID is null
Copy link
Contributor

Choose a reason for hiding this comment

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

String endpoint = new RequestConverters.EndpointBuilder()
.addPathPartAsIs("_xpack", "watcher", "watch")
.addPathPart(...getId())
.addPathPartAsIs("_execute").build()

@romseygeek
Copy link
Contributor Author

run the gradle build tests 2

@romseygeek romseygeek merged commit d2886e1 into elastic:master Dec 5, 2018
@romseygeek romseygeek deleted the hlrc-execute-watch branch December 5, 2018 12:41
romseygeek added a commit that referenced this pull request Dec 5, 2018
This change adds support for the execute watch API in the high level rest client
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.

3 participants