Removes Set Policy API in favour of setting index.lifecycle.name directly#34304
Removes Set Policy API in favour of setting index.lifecycle.name directly#34304colings86 merged 7 commits intoelastic:index-lifecyclefrom colings86:ilm/remove-set-policy-api
Conversation
|
Pinging @elastic/es-core-infra |
|
If we're doing this in the setting, I think we should remove the We could do it as a followup though. What do you think? |
There was a problem hiding this comment.
A couple minor comments, otherwise LGTM (though I second @dakrone's comment, and pending tests passing).
Also, ++ for +9 −1,884.
There was a problem hiding this comment.
Ok, I'll reinstate it then
There was a problem hiding this comment.
Might be worth replacing this test case with one that makes sure changing the lifecycle policy via the settings API works as expected (i.e. actually runs the new policy)
There was a problem hiding this comment.
Agreed, that was the intention of the outstanding task in the description
There was a problem hiding this comment.
My bad, I misread that task.
There was a problem hiding this comment.
NP. My intention was to have this as a server side test rather than a test in the HLRC because its testing ILM functionality rather than the client or the API. Does that work for you or do you feel strongly this should be tested as part of the client?
There was a problem hiding this comment.
I’m good with testing it server side.
I agree this would be a good thing to do but I see it as lower priority given that the remove policy API works currently whereas this API does not. SO my preference would to do this as a subsequent change and for it to not be a blocker |
|
I opened #34310 for the remove policy API |
|
@talevy @gwbrown I added the test mentioned int he description, could you take a look? |
|
@colings86 yup, will take a look today. hopefully the conflicting files are minimal |
talevy
left a comment
There was a problem hiding this comment.
I took a look at it, overall looks good to me!
the only reason I didn't approve is because this still has some conflicts with upstream.
|
@elasticmachine test this please |
1 similar comment
|
@elasticmachine test this please |
directly (#34304) * Removes Set Policy API in favour of setting index.lifecycle.name directly * Reinstates matcher that will still be used * Cleans up code after rebase * Adds test to check changing policy with ndex settings works * Fixes TimeseriesLifecycleActionsIT after API removal * Fixes docs tests * Fixes case on close where lifecycle service was never created
Since elastic#34304, `index.lifecycle.name` is no longer an internal index setting. This was done to simplify the API and remove the dedicated API for setting a policy on an index after it was created. The idea is that this is best done as an index setting update. The same could be said about the remove_policy API. This commit removes the dedicated API command to remove a policy from an index. Update Settings API should be used instead to set `index.lifecycle.name` to null or the empty string.
Still to do:
Closes #34302