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

Add Delete Snapshot High Level REST API #31393

Merged

Conversation

danielmitterdorfer
Copy link
Member

With this commit we add the delete snapshot API to the Java high level
REST client.

Relates #27205

With this commit we add the delete snapshot API to the Java high level
REST client.

Relates elastic#27205
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@danielmitterdorfer
Copy link
Member Author

danielmitterdorfer commented Jun 18, 2018

Marked as "WIP" as we should wait for the create snapshot API so we can leverage it in tests (see #31215). Also the CI build below will (intentionally) fail as I inserted a reminder to implement tests that depend on #31215.

@javanna javanna requested a review from hub-cap June 19, 2018 10:06
@hub-cap
Copy link
Contributor

hub-cap commented Jun 19, 2018

@danielmitterdorfer you can use the low level rest API in the tests, and then change them once the high level API is done. This is what I did when implementing GET /repositories before PUT.

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.

This is a great PR, quite nice to review. Just fyi, running the doc build shows that there are failures to the docs. LGTM for the most part, once the test is impl'd and the docs build is fixed.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a small comment as well

* @return the response
* @throws IOException in case there is a problem sending the request or parsing back the response
*/
public DeleteSnapshotResponse deleteSnapshot(DeleteSnapshotRequest deleteSnapshotRequest, RequestOptions options) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

can you call these methods delete without the snapshot part? they will be called as client.snapshot().delete() then which is how they are defined in our SPEC

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I've changed it.

@danielmitterdorfer
Copy link
Member Author

@hub-cap I've implemented the tests now based on the low-level client as per your suggestion and fixed the doc issues. Can you please have another look?

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.

🚢

@hub-cap
Copy link
Contributor

hub-cap commented Jun 20, 2018

Doh that was supposed to be a :shipit: not a 🚢

@danielmitterdorfer
Copy link
Member Author

Both icons work for me. ;) Thanks for the review!

@danielmitterdorfer danielmitterdorfer merged commit 90d62e6 into elastic:master Jun 20, 2018
danielmitterdorfer added a commit that referenced this pull request Jun 21, 2018
With this commit we add the delete snapshot API to the Java high level
REST client.

Relates #27205
Relates #31393
@danielmitterdorfer
Copy link
Member Author

Backported to 6.x in 71b3ac2.

@danielmitterdorfer danielmitterdorfer deleted the client-delete-snapshot branch June 21, 2018 04:31
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.

5 participants