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
Bug 1782818: Added requested storage in textbox by default. #5768
Bug 1782818: Added requested storage in textbox by default. #5768
Conversation
@cloudbehl: This pull request references Bugzilla bug 1782818, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
a4693cd
to
bea1e30
Compare
@cloudbehl: This pull request references Bugzilla bug 1782818, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
@cloudbehl: This pull request references Bugzilla bug 1782818, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @spadgett @benjaminapetersen @rawagner |
import { K8sResourceKind } from '@console/internal/module/k8s'; | ||
|
||
export const getPvcStorageSize = (pvc: K8sResourceKind): string => | ||
pvc.spec.resources.requests.storage; |
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.
This is a good place for optional chaining.
pvc.spec.resources.requests.storage; | |
pvc?.spec?.resources?.requests?.storage; |
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.
ack
// Modal for expanding persistent volume claims | ||
class ExpandPVCModal extends PromiseComponent { | ||
constructor(props) { | ||
super(props); | ||
const defaultSize = validate.split(getPvcStorageSize(props.resource)); |
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.
Do we need to handle missing values? It looks like these fields are optional in the API.
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.
Do we want to prefill the requested capacity or the actual capacity from status
?
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.
It's the requested size. Because what you pass in the pvc object is requested capacity. So even if the requested capacity is not fulfilled out it will not throw error next time we try to expand.
That's the same we did with OCP 3.11 also but this was the debt that got left.
/unassign @benjaminapetersen |
@@ -0,0 +1,4 @@ | |||
import { K8sResourceKind } from '@console/internal/module/k8s'; | |||
|
|||
export const getPvcStorageSize = (pvc: K8sResourceKind): string => |
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.
Console naming conventions would have PVC
as uppercase. I would make it clear in the name that it's the request.
export const getPvcStorageSize = (pvc: K8sResourceKind): string => | |
export const getRequestedPVCSize = (pvc: K8sResourceKind): string => |
// Modal for expanding persistent volume claims | ||
class ExpandPVCModal extends PromiseComponent { | ||
constructor(props) { | ||
super(props); | ||
const defaultSize = validate.split(getPvcStorageSize(props.resource)); |
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.
Do we want to prefill the requested capacity or the actual capacity from status
?
Signed-off-by: Ankush Behl <cloudbehl@gmail.com>
bea1e30
to
6cbf2b4
Compare
/retest |
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cloudbehl, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@cloudbehl: All pull requests linked via external trackers have merged: openshift/console#5768. Bugzilla bug 1782818 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Earlier If the PVC was created for 1Mi/Ti and you open the expansion model the value used to be empty in text & dropdown used to be Gi by default. Which was getting a bit confusing for the user. With the default set to the requested storage now, it makes a bit easy for the user to expand.
Signed-off-by: Ankush Behl cloudbehl@gmail.com
Before
After