Skip to content

Add SLM Start/Stop/Status APIs #4245

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Dec 13, 2019
Merged

Add SLM Start/Stop/Status APIs #4245

merged 8 commits into from
Dec 13, 2019

Conversation

codebrain
Copy link
Contributor

No description provided.

@codebrain codebrain changed the base branch from master to 7.x December 2, 2019 07:03
@codebrain codebrain marked this pull request as ready for review December 2, 2019 12:22
@Mpdreamz Mpdreamz mentioned this pull request Dec 3, 2019
Closed
24 tasks
russcam
russcam suggested changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

Looks like there are some failing integration tests

@Mpdreamz
Copy link
Member

Mpdreamz commented Dec 9, 2019 

Tests.XPack.Slm.SlmApiTests.GetSnapshotLifecycleStatusResponse
System.Exception : Nest.GetSlmStatusResponse is not expected response type Nest.GetIlmStatusResponse

Tests.XPack.Slm.SlmApiTests.StopSnapshotLifecycleResponse
System.Exception : Nest.StopSlmResponse is not expected response type Nest.StopIlmResponse

russcam
russcam suggested changes Dec 13, 2019
View reviewed changes
Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

I've left some comments.

I think the Slm in type names should be expanded out to SnapshotLifecycle, to match the other existing SLM APIs released. I think the shortened method names are fine, as they are namespaced and reflect the names in the spec.

russcam
russcam approved these changes Dec 13, 2019
View reviewed changes
Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

LGTM.

We had a conversation about whether the types should be SnapshotLifecycle* or SnapshotLifecycleManagement* for start, stop and status APIs. We agreed that including Management was probably a good idea in this instance, as the APIs relate to the plugin as a whole.

@codebrain codebrain merged commit d4c6b80 into 7.x Dec 13, 2019
codebrain added a commit that referenced this pull request Dec 13, 2019
@codebrain
Add SLM Start/Stop/Status APIs (#4245)
fae4565 
(cherry picked from commit d4c6b80)
@codebrain codebrain deleted the feature/7.5/slm-apis branch December 13, 2019 07:44
@codebrain
Copy link
Contributor Author

Ported to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mpdreamz Mpdreamz Mpdreamz left review comments

@russcam russcam russcam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@codebrain @Mpdreamz @russcam