-
Notifications
You must be signed in to change notification settings - Fork 76
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
fix(FR-386): add max value for User/Project setting modal #3050
base: main
Are you sure you want to change the base?
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🔴 | Statements | 4.63% (+0.01% 🔼) |
405/8739 |
🔴 | Branches | 4.01% (-0.02% 🔻) |
240/5984 |
🔴 | Functions | 2.8% | 79/2819 |
🔴 | Lines | 4.56% (+0.01% 🔼) |
390/8550 |
Test suite run success
124 tests passing in 14 suites.
Report generated by 🧪jest coverage report action from 79bd3c3
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.
please resolve the conflicts
…o fix/validate-resource-policy
…lup/backend.ai-webui into fix/validate-resource-policy
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.
There seems to be a conflict in the const-vars.ts file.
I resolved that conflict in latest commit. |
@nowgnuesLee Still I have conflicts. Would you resolve it and re-request review again? |
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.
Please resolve the conflicts.
@@ -1194,6 +1194,7 @@ | |||
"Name": "Name", | |||
"AllowedHosts": "Allowed hosts", | |||
"CannotCreateResourcePolicy": "Cannot create Resource Policy. Please check input values.", | |||
"CannotUpdateResourcePolicy": "Cannot update Resource Policy. Please check input values.", |
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.
We couldn't say "Please check input values" because this PR might display this message even when the user input is correct.
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.
I expect the detailed error message to be provided in err.message
from the response
. If not, I assume it would be an error that occurs in such cases. So, what do you think about simply displaying ‘Cannot update Resource Policy.’? I will check whether the backend provides sufficient error explanations.
message.success(t('resourcePolicy.SuccessfullyCreated')); | ||
onRequestClose(true); | ||
if (errors && errors.length > 0) { | ||
errors.forEach((error) => message.error(error.message, 2.5)); |
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.
Is there any reason to set it to 2.5 only here?
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.
I set it to 2.5 seconds since it was set that way in the UserResourcePolicyList. What would be an appropriate duration?
…o fix/validate-resource-policy
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.
I couldn’t find any conflicts on my side. I have updated the branch, but if you still encounter any conflicts, please let me know.
resolves #3055 (FR-386)
max
value for each time in the setting modal on theResource Policy
pageusePainkilerr
changes
onCompleted
MAX_CPU_QUOTA
,SIGNED_32BIT_MAX_INT
MAX_CPU_QUOTA
, since the CPU type is afloat
, we limited the range to1e16
after consulting with @fregataa.Checklist: (if applicable)