Refactor VM node selector scenario #5742
Conversation
rhrazdil
commented
Jun 15, 2020
rhrazdil
commented
- Created model for Node Selector modal dialog
- updated node-selector.scenario.ts
openshift-ci-robot
added
component/kubevirt
rhrazdil
force-pushed
the
node-selector-model
branch
from
5384557
to
b5e9cf7
Compare
|
|
||
| describe('KubeVirt VM detail - edit Node Selector', () => { | ||
| const testVM = getVMManifest('Container', testName, `node-selector-vm-${getRandStr(5)}`); | ||
| const vm = new VirtualMachine(testVM.metadata); | ||
| const vm: VirtualMachine = new VirtualMachine(testVM.metadata); | ||
| const labels: MatchLabels = { |
There was a problem hiding this comment.
I would suggest adding at least two labels for this tests to make sure it works well. And then refactor deleteLabel to deleteLabels.
There was a problem hiding this comment.
updated, adding and removing 2 key:value labels, added deleteNodeSelectors method.
rhrazdil
force-pushed
the
node-selector-model
branch
from
b5e9cf7
to
db0d569
Compare
rhrazdil
force-pushed
the
node-selector-model
branch
from
db0d569
to
bdd9ae9
Compare
| @@ -39,4 +53,28 @@ export class VirtualMachine extends BaseVirtualMachine { | |||
| timeout, | |||
| ); | |||
| } | |||
|
|
|||
| async addNodeSelectors(labels: MatchLabels) { | |||
There was a problem hiding this comment.
Is it better to move addNodeSelectors and deleteNodeSelectors to kubevirtUIResource.ts?
There was a problem hiding this comment.
No, scheduling is relevant for Virtual Machine only, it cannot be edited in VMI or Virtual Machine Templates
| * Adds new row if needed. | ||
| * Returns string with index of next empty row. | ||
| */ | ||
| async addRow(): Promise<string> { |
There was a problem hiding this comment.
Other place like Tolerations and affinity may use addRow as well, then we can move it to utils or console-shared.
There was a problem hiding this comment.
Better place would probably be some scheduling.dialog.view.ts file, utils and console-shared are for more general purpose utility methods
There was a problem hiding this comment.
okay, if you decide not to do it in this one, then it looks good to me.
|
@suomiy can you review please? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhrazdil, suomiy 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
