-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Allow the ILM policy name to be configurable #2971
Allow the ILM policy name to be configurable #2971
Conversation
- Add an option --ilm-policy-name to esmapping-generator; - Set the mapping's option in the rollover script (using an env var ES_ILM_POLICY_NAME). The policy's name defaults to jaeger-ilm-policy (both in the mapping generator and in the rollover script), so no breaking changes are added. Signed-off-by: Ricardo Ribeiro <j.ribeiro.fafe@gmail.com>
@jrRibeiro I am not sure our CI covers this part of ES maintenance, did you do some manual testing? If so could you post some test results / evidence? |
Codecov Report
@@ Coverage Diff @@
## master #2971 +/- ##
==========================================
- Coverage 95.97% 95.95% -0.02%
==========================================
Files 223 223
Lines 9712 9718 +6
==========================================
+ Hits 9321 9325 +4
- Misses 324 326 +2
Partials 67 67
Continue to review full report at Codecov.
|
Signed-off-by: Ricardo Ribeiro <j.ribeiro.fafe@gmail.com>
Yes I did some manual testing but it's better to actually test the change as part of the integration tests of the index rollover. I just added a new integration test (and refactored the tests a bit) to test both the default ILM policy name (to make sure that not setting the env var still works) and also a new test to set the env var and test if the ILM policy associated with the created indices matches the one we define. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@albertteoh can you take a look at the integration test changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too.
Pull request has been modified.
Thanks @jrRibeiro! |
Thank you @yurishkuro and @albertteoh! |
Which problem is this PR solving?
Short description of the changes
--ilm-policy-name
toesmapping-generator
, to set the policy name in the mappings file;ES_ILM_POLICY_NAME
).jaeger-ilm-policy
(both in the mapping generator and in theinit
action of the rollover script), so no breaking changes are added.