Skip to content
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

Remove unnecessary time units in ILM policy dialog #140815

Merged
merged 5 commits into from Sep 20, 2022

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Sep 15, 2022

Summary

Fix #137081

Please remove unnecessary time units (less than minutes) in the Index Lifecycle Policy creation dialog. There is no use case for these rollover periods and they are potentially harmful when selected by accident.

Also cleans up separate units used in downsample action #138748 (comment)

This cleans up smaller units from the following inputs:

Screenshot 2022-09-15 at 15 19 28

Screenshot 2022-09-15 at 15 19 38

  • If users used no longer supported units, when they open the form, their old unit is displayed.
  • these inputs support only integers. so it is no longer possible to configure e.g 1.5 minutes intervals using 90 seconds in the input. (I guess this is OK)

@Dosant
Copy link
Contributor Author

Dosant commented Sep 15, 2022

@elasticmachine merge upstream

@Dosant Dosant added release_note:fix enhancement New value added to drive a business result Feature:ILM v8.5.0 labels Sep 19, 2022
@Dosant Dosant marked this pull request as ready for review September 19, 2022 10:42
@Dosant Dosant requested a review from a team as a code owner September 19, 2022 10:42
@Dosant
Copy link
Contributor Author

Dosant commented Sep 19, 2022

@elasticmachine merge upstream

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up @Dosant!

I was testing locally and noticed if I create a policy with unsupported time units, the unit shows in the dropdown for the min age, but not the rollover max age. Is that expected?

// ms value, but does not show in dropdown
Screen Shot 2022-09-19 at 8 46 53 AM

// s value and shows in the dropdown
Screen Shot 2022-09-19 at 8 47 02 AM

Also, I noticed on large screens this input field is not aligned with the rest. I don't think this is related to your PR, but just pointing out. I can open up an issue if there isn't one already.

Screen Shot 2022-09-19 at 8 42 21 AM

@Dosant
Copy link
Contributor Author

Dosant commented Sep 19, 2022

I was testing locally and noticed if I create a policy with unsupported time units, the unit shows in the dropdown for the min age, but not the rollover max age. Is that expected?

Yes. We have these very similar controls implemented in two very different ways. I went with the simplest approach to "fallback" in each implementation.
I would prefer the simpler approach, like in rollover max-age, then the one in min-age, but I couldn't do it in min-age because it uses a select component from EUI, and it expects all the options in the dropdown array.

Would you suggest changing the current behavior somehow?

Also, I noticed on large screens this input field is not aligned with the rest. I don't think this is related to your PR, but just pointing out. I can open up an issue if there isn't one already.

Not sure if the issue exists.
Agree that this or my recent prs didn't cause this

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation @Dosant! I'm happy with these changes 👍

@Dosant
Copy link
Contributor Author

Dosant commented Sep 20, 2022

@elasticmachine merge upstream

@Dosant Dosant enabled auto-merge (squash) September 20, 2022 13:24
@Dosant Dosant merged commit 36a9aa5 into elastic:main Sep 20, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
indexLifecycleManagement 158.7KB 157.6KB -1.1KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting enhancement New value added to drive a business result Feature:ILM release_note:fix v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unnecessary rollover time units in ILM policy dialog
4 participants