-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Index lifecycle management] Add deprecated policy warnings #174150
[Index lifecycle management] Add deprecated policy warnings #174150
Conversation
/ci |
@elasticmachine merge upstream |
/ci |
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.
Suggesting to tweak a little the message description to:
- not repeat the title
- be slightly more explicit about what this deprecation means (the policy might/will be removed eventually, which is different than just stay without being maintained)
- say what should a user using it do?
...dex_lifecycle_management/public/application/sections/edit_policy/components/edit_warning.tsx
Outdated
Show resolved
Hide resolved
...dex_lifecycle_management/public/application/sections/policy_list/components/policy_table.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
Thanks for working on this @sabarasaba! I just took a look at your screenshots and read some more of the background. I noticed this comment in this ES issue:
What do you think about doing something like this (ie, hide the deprecated policies by default behind a toggle or something)? While I think the "Deprecated" badge is helpful, it does still seem potentially confusing to see both On another note, I'm wondering if we should reconsider showing the error banner "Editing a managed policy can break Kibana", especially with the comments on #172683. This is outside the scope of your PR, just something I noticed while reviewing the screenshots 😄 |
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.
Thanks for working on this @sabarasaba! Tested locally and code changes LGTM 👍
I only have a question on the index template changes as I was wondering why they are needed.
Edit: Sorry, I just noticed that @alisonelizabeth added some good suggestions. I will review again if further changes are made.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Thanks for the review @ElenaStoeva and @alisonelizabeth! I've updated the PR to have the deprecated polcies hidden by default and with a table filter we can toggle them on/off, so things seem a little bit tidier now.
Yeah I think we should think about that a bit more, I've created a separate issue for dealing with that #174579 |
@elasticmachine merge upstream |
|
||
// Now we should see all deprecated policies | ||
deprecatedPolicies = findTestSubject(rendered, 'deprecatedPolicyBadge'); | ||
expect(deprecatedPolicies.length).toBe(10); |
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.
nit: I'm wondering if it would be a good idea to expect the length to be greater than 0 rather than exactly 10 (so that we future proof the test in case some managed policies are added/removed in the test environment)?
Also nit, could we test that actually clicking the "Deprecated" button shows the deprecated policies?
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.
The table filters dont have a property for adding a data-test-subj, so the only way I could find how to test it was by inputting a filter in the search bar!
Thanks for updating the PR @sabarasaba. I tested locally again and verified that the deprecated policies are hidden by default and that the "Deprecated" button works correctly. New code changes also LGTM (with a minor comment on the tests). 👍 |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @sabarasaba |
Partially addresses #170805
Summary
With elastic/elasticsearch#101148 we now can alert the users when an ILM policy will soon be deprecated and shouldnt be relied upon. This PR adds a badge and a callout to the ILM UI to alert the users about this.
How to test
Create deprecated policy
Screenshots