Skip to content

Add enrich policy GET API #41384

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

Merged
merged 18 commits into from
May 28, 2019
Merged

Add enrich policy GET API #41384

merged 18 commits into from
May 28, 2019

Conversation

hub-cap
Copy link
Contributor

@hub-cap hub-cap commented Apr 19, 2019

This commit wires up the Rest calls and Transport calls for GET enrich
policy, as well as tests and rest spec additions.

This commit wires up the Rest calls and Transport calls for GET enrich
policy, as well as tests and rest spec additions.
@hub-cap hub-cap added >enhancement :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v8.0.0 labels Apr 19, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

I left two comments here too. Lets get #41383 first, so that tests can be adapted.

if (policy == null) {
throw new ResourceNotFoundException("policy [{}] is missing", name);
}
return policy;
Copy link
Member

Choose a reason for hiding this comment

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

I think the two if statements in this method should be merged with EnrichStore#getPolicy(...) and also tests should move the EnrichStoreTests. Since the static method will basically by doing nothing, it can be merged with masterOperation(...)


import static org.elasticsearch.xpack.enrich.EnrichPolicyTests.randomEnrichPolicy;

public class GetEnrichPolicyActionResponseTests extends AbstractWireSerializingTestCase<GetEnrichPolicyAction.Response> {
Copy link
Member

Choose a reason for hiding this comment

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

I think this test should extend AbstractSerializingTestCase, so that xcontent serialization is also tested.

@hub-cap
Copy link
Contributor Author

hub-cap commented Apr 22, 2019

I went ahead and removed this transport test as well, ala #41383 (comment), and fixed up the enrich store based on the review.

@hub-cap hub-cap removed the v8.0.0 label Apr 24, 2019
@hub-cap
Copy link
Contributor Author

hub-cap commented Apr 25, 2019

@elasticmachine run elasticsearch-ci/1

@hub-cap hub-cap requested a review from jakelandis April 25, 2019 22:34
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

@martijnvg
Copy link
Member

@elasticmachine run elasticsearch-ci/1

@hub-cap
Copy link
Contributor Author

hub-cap commented May 2, 2019

@elasticmachine run elasticsearch-ci/1

@hub-cap hub-cap merged commit 55f80ae into elastic:enrich May 28, 2019
hub-cap added a commit that referenced this pull request May 29, 2019
This commit wires up the Rest calls and Transport calls for GET enrich
policy, as well as tests and rest spec additions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants