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

Move reindex request parsing into request #43107

Merged
merged 1 commit into from Jun 14, 2019

Conversation

Projects
None yet
4 participants
@tbrooks8
Copy link
Contributor

commented Jun 11, 2019

Currently the fromXContent logic for reindex requests is implemented in
the rest action. This is inconsistent with other requests where the
logic is implemented in the request. Additionally, it requires access to
the rest action in order to parse the request. This commit moves the
logic and tests into the ReindexRequest.

Move reindex request parsing into request
Currently the fromXContent logic for reindex requests is implemented in
the rest action. This is inconsistent with other requests where the
logic is implemented in the request. Additionally, it requires access to
the rest action in order to parse the request. This commit moves the
logic and tests into the ReindexRequest.
@elasticmachine

This comment has been minimized.

Copy link

commented Jun 11, 2019

@ywelsch
Copy link
Contributor

left a comment

LGTM

@henningandersen
Copy link
Contributor

left a comment

I would like to also have a test case demonstrating that we can read the x-content generated from toXContent using fromXContent. So far, these have been fairly disconnected but going forward we will rely on from/toXContent being in strong agreement. I think AbstractSerializingTestCase has the framework for it though asserting equals will require more work (possiblely just add equals to ReindexRequest?).

Otherwise looking good. You are welcome to defer above to a separate PR if you think that is better.

@tbrooks8 tbrooks8 merged commit ba80287 into elastic:master Jun 14, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
@tbrooks8

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

You are welcome to defer above to a separate PR if you think that is better.

I will open a new PR for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.