Bug 2187971: guard for template parameter on disk moda #1270
Conversation
|
@upalatucci: This pull request references Bugzilla bug 2187971, 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
Requesting review from QA contact: 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. |
|
@upalatucci: This pull request references Bugzilla bug 2187971, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
| `${vm?.metadata?.name}-${diskState.diskName}`, | ||
| resultDisk, | ||
| ); | ||
| const isNameTemplateParameter = /[${}]/.test(vm?.metadata?.name); |
There was a problem hiding this comment.
Are you sure this kind of check is gonna work well everywhere we use DiskModal? I can see we deal with disks when dealing with VMs and also Templates.. And when you get here data of a template that has been already processed, there's no any chars like ${} in the name even when the name was generated according to the NAME param.
Buut... maybe this is not a problem? I am not sure because this PR lacks more info about the problem in its description. Maybe the problem is just we don't wanna get ${} chars into the pvcName?
There was a problem hiding this comment.
As you can see from the bugzilla ticket, there is also a screenshot of the problem.
The description is explanatory to me. The Diskmodal is trying to create a disk with ${NAME}. $, { and } cannot be used as names so whatever name it is, if contains those characters, the name contains a parameter.
The template at this point is not processed.
There was a problem hiding this comment.
Thanks for the explanation, Ugo. So are you sure it's gonna work well for both VM and Templates Disks pages? Did you test it in both places?
upalatucci
force-pushed
the
disk-modal-template-name-parameter
branch
from
fe67e06
to
046d941
Compare
upalatucci
force-pushed
the
disk-modal-template-name-parameter
branch
2 times, most recently
from
7f06463
to
4358690
Compare
upalatucci
force-pushed
the
disk-modal-template-name-parameter
branch
from
4358690
to
3c106e2
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metalice, upalatucci 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 |
|
@upalatucci: All pull requests linked via external trackers have merged: Bugzilla bug 2187971 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. |
📝 Description
No UI diff.
Do not use
vm.metadata.nameif contains a parameter to create the DV NAME. Like ${NAME}Disk name is provided by the user so no need to guard on that.