Skip to content

Conversation

@danielmitterdorfer
Copy link
Member

With this commit we rename the default ILM policy for profiling from profiling to profiling-60-days which describes the policy's intention and allows to add related policies in the future more naturally.

With this commit we rename the default ILM policy for profiling from
`profiling` to `profiling-60-days` which describes the policy's
intention and allows to add related policies in the future more
naturally.
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/profiling (Team:Universal Profiling)

Copy link
Contributor

@rockdaboot rockdaboot left a comment

Choose a reason for hiding this comment

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

LGTM

Just a nit: The string "profiling-60-days" appears in at least 4 places. Does it make sense to have a constant for it ?

Copy link
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

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

LGTM

@danielmitterdorfer
Copy link
Member Author

Just a nit: The string "profiling-60-days" appears in at least 4 places. Does it make sense to have a constant for it ?

Good catch; I checked the usages: One of them is referring to the file name of the ILM definition (wouldn't use the constant for that one), one defines the actual ILM policy name and two usages are in tests. I'd probably keep it as is for now also to have the code in ProfilingIndexTemplateRegistry more uniform but I'll keep it in mind in case this starts to spread more.

@danielmitterdorfer danielmitterdorfer merged commit 52914e4 into elastic:main Jun 21, 2023
@danielmitterdorfer danielmitterdorfer deleted the profiling-ilm-name branch June 21, 2023 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :UniversalProfiling/Application Elastic Universal Profiling REST APIs and infrastructure v8.9.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants