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

feat: add max validation rule for CPU and memory in Form.Item. #2692

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

yomybaby
Copy link
Member

@yomybaby yomybaby commented Sep 5, 2024

TL;DR

Normally, users cannot set the value above the maximum limit because it automatically changes when the user focuses out of the input (blur event). However, it can be set programmatically above the maximum limit. For example, on the model service modification page, the existing model service can have a value above the maximum limit.

This PR added maximum value validation for CPU and memory allocation in the resource allocation form.

What changed?

  • Added a new rule to validate the maximum CPU value based on resource limits.
  • Implemented a validator for memory allocation to ensure it doesn't exceed the maximum limit.
  • Added a new translation key "MaxValueNotification" across all language files to support the new validation messages.

How to test?

  1. Open model service modification page.
  2. Change the initialValue to value above the maximum limit (ServiceLauncherPageContent.tsx)

Copy link
Member Author

yomybaby commented Sep 5, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @yomybaby and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added area:ux UI / UX issue. area:i18n Localization size:M 30~100 LoC labels Sep 5, 2024
@yomybaby yomybaby marked this pull request as ready for review September 5, 2024 04:36
@ironAiken2 ironAiken2 changed the base branch from fix/modification-of-model-services-resource to graphite-base/2692 September 5, 2024 05:25
@ironAiken2 ironAiken2 force-pushed the feat/max-validation-rule-cpu-mem branch from f90f62e to d933aec Compare September 5, 2024 05:36
@ironAiken2 ironAiken2 changed the base branch from graphite-base/2692 to main September 5, 2024 05:37
@ironAiken2 ironAiken2 force-pushed the feat/max-validation-rule-cpu-mem branch from d933aec to 3c5c8e7 Compare September 5, 2024 05:37
Copy link

github-actions bot commented Sep 5, 2024

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements 5.47% 337/6156
🔴 Branches 5.06% 214/4232
🔴 Functions 3.11% 63/2023
🔴 Lines 5.37% 323/6012

Test suite run success

90 tests passing in 11 suites.

Report generated by 🧪jest coverage report action from bacb099

Copy link
Contributor

@lizable lizable left a comment

Choose a reason for hiding this comment

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

@yomybaby
It's kinda minor issue, but I think validation rule works differently when user add numbers after maximum values and just input number that exceeds maximum value.
For example, if the maximum value of CPU is 19 Core, then we have two cases.
It doesn't affects any error when updating model service since exceeding the maximum value is regarded as invalid, but I want to check whether is acceptable or not.

Here's the video clip that I mentioned about the different validation result.

🎥 Video uploaded on Graphite:

Copy link

graphite-app bot commented Sep 5, 2024

Your org requires the Graphite merge queue for merging into main

Add the label “flow:merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “flow:hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@yomybaby yomybaby force-pushed the feat/max-validation-rule-cpu-mem branch from 3c5c8e7 to b191501 Compare September 9, 2024 05:05
Copy link
Member Author

yomybaby commented Sep 9, 2024

To make it have same behavior with Ant'd Input max, I pushed a commit. Please check

@github-actions github-actions bot added size:L 100~500 LoC and removed size:M 30~100 LoC labels Sep 9, 2024
Copy link
Contributor

@lizable lizable left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link

graphite-app bot commented Sep 9, 2024

Merge activity

### TL;DR

Normally, users cannot set the value above the maximum limit because it automatically changes when the user focuses out of the input (blur event). However, it can be set programmatically above the maximum limit. For example, on the model service modification page, the existing model service can have a value above the maximum limit.

This PR added maximum value validation for CPU and memory allocation in the resource allocation form.

### What changed?

- Added a new rule to validate the maximum CPU value based on resource limits.
- Implemented a validator for memory allocation to ensure it doesn't exceed the maximum limit.
- Added a new translation key "MaxValueNotification" across all language files to support the new validation messages.

### How to test?

1. Open model service modification page.
2. Change [the initialValue ](https://github.com/lablup/backend.ai-webui/blob/f90f62e5112bab03bbd92d7b6e16611c0ec3d682/react/src/components/ServiceLauncherPageContent.tsx#L601-L607) to value above the maximum limit (ServiceLauncherPageContent.tsx)
@yomybaby yomybaby force-pushed the feat/max-validation-rule-cpu-mem branch from 967e995 to bacb099 Compare September 9, 2024 07:33
@graphite-app graphite-app bot merged commit bacb099 into main Sep 9, 2024
6 checks passed
@graphite-app graphite-app bot deleted the feat/max-validation-rule-cpu-mem branch September 9, 2024 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:i18n Localization area:ux UI / UX issue. size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants