Skip to content

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
24 tasks
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

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.

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 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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants