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

Add UI to select number of non-current versions subject to ILM expiry rule #3088

Merged
merged 9 commits into from
Nov 16, 2023

Conversation

jinapurapu
Copy link
Contributor

@jinapurapu jinapurapu commented Oct 12, 2023

Adds unit selector to Add Lifecycle rule modal to enable selection to expire non-current versions of objects after a number of non-current versions OR a number of days, and UI changes to display non-current version count based rules.

Edit ILM Rule for version count based rules in a separate PR

Screen.Recording.2023-11-01.at.3.14.30.PM.mov

@jinapurapu jinapurapu added the WIP This PR is WIP and cannot be merged yet label Oct 12, 2023
@jinapurapu jinapurapu self-assigned this Oct 12, 2023
@jinapurapu jinapurapu added WIP This PR is WIP and cannot be merged yet and removed WIP This PR is WIP and cannot be merged yet labels Oct 13, 2023
@jinapurapu jinapurapu removed the WIP This PR is WIP and cannot be merged yet label Oct 30, 2023
Copy link
Collaborator

@bexsoft bexsoft left a comment

Choose a reason for hiding this comment

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

LGTM

@cesnietor
Copy link
Collaborator

cesnietor commented Oct 31, 2023

@jinapurapu please remove the console log:
set to valid: true lifecycledays: 231906034276324167843127894132794372979234174138

@cesnietor
Copy link
Collaborator

if the number is too long:
231906034276324167843127894132794372979234174138207324072341
the request fails like:

"parsing body body from \"\" failed, because json: cannot unmarshal number 2.3190603427632416e+59 into Go struct field AddBucketLifecycle.noncurrentversion_expiration_days of type int32"

And there is no ui indication that something went wrong.
Could we add a max allowed field in the input.
And also show the error if it occurs?

@cesnietor
Copy link
Collaborator

cesnietor commented Oct 31, 2023

As UX this text doesn't have a way to let the user know that it's clickable:
Screenshot 2023-10-31 at 1 29 51 PM
So users may never find about it.
We can fix this by simply adding something like an icon e.g. ^ next to the text?

@cesnietor
Copy link
Collaborator

cesnietor commented Oct 31, 2023

Since 0 days is not a valid value.
We correctly disable the button but we don't say what the valid values are, or that 0 is not valid. not sure if this can be done via a simple helper text or something else @bexsoft .
Screenshot 2023-10-31 at 1 39 40 PM

@bexsoft
Copy link
Collaborator

bexsoft commented Oct 31, 2023

Since 0 days is not a valid value. We correctly disable the button but we don't say what the valid values are, or that 0 is not valid. not sure if this can be done via a simple helper text or something else @bexsoft . Screenshot 2023-10-31 at 1 39 40 PM

Yes, it can be done by setting the error prop for this specific field

Jillian Inapurapu and others added 4 commits November 1, 2023 14:55
WIP edit lifecyle rule with versions count

Fixed lint error

Fix lint error

Fix lint errors correctly

Fix lint error

Removed changes to EditILMRule to put in new PR

Removed newer_noncurrent_version_transition elements

UI cleanup enabling button, display correct unit
…der relevant conditions, informative error text if inappropriate value used for lifecycleDays
@jinapurapu jinapurapu requested a review from bexsoft November 1, 2023 22:13
@jinapurapu jinapurapu changed the title Add UI to select number of non-current versions subject to ILM rule Add UI to select number of non-current versions subject to ILM expiry rule Nov 1, 2023
@cesnietor
Copy link
Collaborator

Can we change the validation error to be something like: value should be between 1-<the actual max>. We can save them to google the value by actually telling them the max.

@cesnietor
Copy link
Collaborator

Tested, it now shows the validation warnings as well as some docs on the days/versions.
Screenshot 2023-11-09 at 4 01 43 PM
Screenshot 2023-11-09 at 4 01 39 PM
Screenshot 2023-11-09 at 4 01 32 PM

Copy link
Collaborator

@cesnietor cesnietor left a comment

Choose a reason for hiding this comment

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

Ah @jinapurapu the message should be either less than or equal to 2147483647 or less than 2147483648 cause it allows 2147483647.
Screenshot 2023-11-09 at 4 06 41 PM

@bexsoft bexsoft merged commit da53daf into minio:master Nov 16, 2023
30 of 31 checks passed
cesnietor pushed a commit to cesnietor/console that referenced this pull request Jan 12, 2024
cesnietor pushed a commit to cesnietor/console that referenced this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants