-
Notifications
You must be signed in to change notification settings - Fork 605
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
Refactor VM Configs #5823
Refactor VM Configs #5823
Conversation
@suomiy when you have time, could you take a look please |
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.
Looks pretty good! Much better experience reading the tests now.
import { getRandStr } from '../utils/utils'; | ||
|
||
export abstract class BaseVMBuilder<T extends BaseVMBuilderData> { | ||
protected kind: K8sKind; |
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 know the naming is very confusing and it is not consistent across the codebase, but can we name it model? As we use it in few places.
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.
Yes, I used model in the UIResource, forgot to update this one
public generateName(id?: string) { | ||
const finalId = id || getRandStr(5); | ||
const osId = OSIDLookup[this.data.os]; | ||
this.data.name = [ |
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.
can we defer this action until the build is called?
Some of the fields might not be initialized yet.
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.
Yes, I added in VMBuilder/VMTemplateBuilder
public generateName(id?: string) {
this.generateNameId = id;
return this;
}
and
build() {
super.generateName(this.generateNameId);
return new VirtualMachine(this.getData());
}
So the name is set even when we don't really care about it
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.
ok, but we should still allow for simple setName
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.
Good point... What if we add setName
method and generate random name by default if not set explicitly
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.
can we still create some variable like generateNameID and use it when generating name in build method when name is not filled?
and not run generating name logic here (just from build method) as not all info for it might not be available yet
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 added
generateNameForPrefix(prefix: string) {
this.data.name = `${prefix}-${getRandStr(5)}`;
return this;
{
And don't call generateName anywhere except the build methods
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.
looks good
return this; | ||
} | ||
|
||
public setBuilder(builder: BaseVMBuilder<T>) { |
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.
What is the use case of this method? The naming is confusing as it does not behave exactly as a setter. Meaning, only the fields which the build has will be overwritten.
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 think we thought of it similarly as when we inherit existing VMBuilder when creating new one, except in this case it's for two existing VMBuilders. We're not using it anywhere.
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 use it in generateBuilders. Can we at least rename it a bit or add a doc?
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.
Oh, right, we do use it there. I'm not sure about a fitting name, so added a doc
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.
+1 for doc
what about naming it setBuilderAttributes
?
Object.keys(builder.data) | ||
.filter((key) => builder.data[key] !== undefined) | ||
.forEach((key) => { | ||
this.data[key] = builder.data[key]; |
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.
even in this implementation it is better to deep copy everything
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.
updated
this.data[key] = builder.data[key]; | ||
}); | ||
if (builder.data.networks) { | ||
this.data.networks.push(...builder.data.networks); |
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.
maybe we could filter out the duplicates (same references) first and then deep copy the data.
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.
updated
return newComputedBuilders; | ||
}; | ||
|
||
export const computedVMBuilders = deepFreeze( |
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.
and some doc here? what is the result? You can just write some small example.
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.
are we using the generated builders somewhere?
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're not using these generated builders atm. I'm not sure how to deal with adding polarion IDs for this. We could leave it without an ID and not have a test case in polarion, or create one test case in Polarion and describe the matrix of created VMs there. I'm afraid that having test case in Polarion for each combination is impossible to maintain givan the interface Polarion provides us....
We could use them in v2v suite, which is all about importing differently configured VMs
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 think having one test case with the matrix could serve this purpose quite fine. Can we use it? Or are there some issues with it?
+1 for v2v
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.
Don't see any issue, should I include it in this PR?
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.
IMO you can, but up to you.
}, {}), | ||
); | ||
|
||
export const basicVM = new VMBuilder() |
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.
suppose we want to use two basicVM builders in one test, then these two builders would be the same instance and would mutate themselves.
I know it is always passed to the new VMBuilder constructor so this will never occur. But somebody might want to use the basicVM without this knowledge in the future and it could cause issues
We have few options here:
- make the data inside this VMBuilder immutable (frozen) - we could add freeze method on it
- just use immutable VMBuilderData here instead
- use a function which returns new instance of a VMBuilder
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.
- use a function which returns new instance of a VMBuilder
updated
@@ -77,4 +102,51 @@ export class VirtualMachine extends BaseVirtualMachine { | |||
await this.deleteNodeSelector(key); | |||
}); | |||
} | |||
|
|||
async start(waitForAction = true) { |
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.
+1 easier to use
import { VMBuilderData } from '../types/vm'; | ||
import { VM_ACTION, TAB, VM_STATUS } from '../utils/constants/vm'; | ||
import { MatchLabels } from 'public/module/k8s'; | ||
import { Wizard } from './wizard'; | ||
|
||
const noConfirmDialogActions: VM_ACTION[] = [VM_ACTION.Start, VM_ACTION.Clone]; | ||
|
||
export class VirtualMachine extends BaseVirtualMachine { |
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.
does it make sense to have similar inheritance structure here as the builders have?
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 you suggest to squash the UIResource, kubevirtUIResource together?
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 don't know. I mean mainly the usage of data (VMBuilderData, etc) and having a common class for that with using generics.
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.
ping
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.
Yes, still TODO
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.
updated, is this what you meant?
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.
almost :) replied bellow
private data: VMBuilderData; | ||
|
||
constructor(data: VMBuilderData) { | ||
super({ ...data, model: VirtualMachineModel }); |
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.
can we just cherry pick the name and namespace here and not pass the whole object? It is easier to read that way IMO
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.
Yes
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.
Some places are using vm.detailViewAction(VM_ACTION.Start)
and some are using vm.start()
, Suggest use vm.start
in all places.
@@ -149,13 +149,19 @@ export const waitForCount = (elementArrayFinder: any, targetCount: number) => { | |||
|
|||
export const waitForStringInElement = (elem: any, needle: string) => { | |||
return async () => { | |||
if (!(await elem.isPresent())) { | |||
return false; | |||
} | |||
const content = await elem.getText(); | |||
return content.includes(needle); | |||
}; | |||
}; | |||
|
|||
export const waitForStringNotInElement = (elem: any, needle: 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.
is it necessaty? can it be !waitForStringInElement
instead of waitForStringNotInElement
?
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.
the if (!(await elem.isPresent())) {
is good to check, because if the element doesn't happen to be present, the subsequent await elem.getText();
would throw an exception.
As to whether we could use !waitForStringInElement
, yes we could use it like that, I thought it would be a bit more readable this way. If you want to change the occurences to !waitForStringInElement instead, it can be done
[ProvisionSource.URL]: { | ||
method: ProvisionSource.URL, | ||
source: | ||
'http://cnv-qe-server.rhevdev.lab.eng.rdu2.redhat.com/files/cnv-tests/cirros-images/cirros-0.4.0-x86_64-disk.raw', |
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.
https://download.cirros-cloud.net/0.4.0/cirros-0.4.0-x86_64-disk.img
is workable on 2.4, maybe we should use a public source 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 think I saw some issues with the public one, because our clusters download this image from the same IP address, and sometimes it seems that download.cirros-cloud.net rate limits the requests. I'll change it to public one, if there are issues, we can always use the internal mirror.
}, | ||
[ProvisionSource.CONTAINER]: { | ||
method: ProvisionSource.CONTAINER, | ||
source: 'kubevirt/cirros-registry-disk-demo', |
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.
Test like cloud-init
requires fedora image, l suggest putting fedora image here for all testing using provision source CONTAINER
.
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.
yep, I'll use fedora
@@ -77,4 +102,51 @@ export class VirtualMachine extends BaseVirtualMachine { | |||
await this.deleteNodeSelector(key); | |||
}); | |||
} | |||
|
|||
async start(waitForAction = true) { |
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.
Suggest adding a check whether the VM is running, doing nothing if the VM is running. Same for stop
.
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.
That's interesting point, I can add it in a followup
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.
a small note here: logic for checking if vm is expected running or not is quite complicated: please see isVMExpectedRunning
function
} | ||
|
||
async stop(waitForAction = true) { | ||
await this.action(VM_ACTION.Restart, waitForAction); |
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.
Typo here, restart
should be stop
.
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.
Nice catch!
|
||
/** | ||
* Performs action form list view or detail view | ||
*/ | ||
async action(action: VM_ACTION, waitForAction = true, timeout?: number) { |
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 seems waitForAction
is always true
, if so, can we simplify this function by not adding waitForAction
?
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.
Not always, in non-admin we don't wait, and I'm pretty sure there are more uses.
I would prefer to keep it, it gives up option to save time where waiting for the VM to be Running is not required, for example in permission oriented tests
@gouyang |
@suomiy Alright, I believe I addressed all the comments from the first round :) |
await this.action(VM_ACTION.Delete, waitForAction); | ||
} | ||
|
||
async clone(name?: string, namespace?: string): Promise<VirtualMachine> { |
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.
@suomiy wdyt?
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 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.
looks great and user friendly
One last touch could be a usage of builder here and using setName/setNamespace instead of plain object.
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 wanted to use the VMBuilder, but it creates dependency cycle....
That's why I did it this way
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.
ok np. Looks nice as it is
|
||
protected data: T; | ||
|
||
constructor(kind: K8sKind, builder?: BaseVMBuilder<T>) { |
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.
kind leftover
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.
Fixed
public generateName(id?: string) { | ||
const finalId = id || getRandStr(5); | ||
const osId = OSIDLookup[this.data.os]; | ||
this.data.name = [ |
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.
can we still create some variable like generateNameID and use it when generating name in build method when name is not filled?
and not run generating name logic here (just from build method) as not all info for it might not be available yet
.setStartOnCreation(false) | ||
.generateName() | ||
.setDisks([getDiskToCloneFrom()]) | ||
.generateName('vm1') |
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.
called twice, plus in other cases as well.
maybe we should also remove all instances of generateName calls if we do it by default
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.
Removed all generateName instances
return this; | ||
} | ||
|
||
public setBuilder(builder: BaseVMBuilder<T>) { |
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.
+1 for doc
what about naming it setBuilderAttributes
?
|
||
public appendBuilder(builder: BaseVMBuilder<T>) { | ||
const customAppendKeys = new Set(['networks', 'disks', 'storageClassAttributes']); | ||
const data = _.cloneDeep(builder.getData()); |
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 forgot what was here before and it looks fine now, but getData returns cloned data already so it should be enough.
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.
Right, fixed
await this.action(VM_ACTION.Delete, waitForAction); | ||
} | ||
|
||
async clone(name?: string, namespace?: string): Promise<VirtualMachine> { |
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.
looks great and user friendly
One last touch could be a usage of builder here and using setName/setNamespace instead of plain object.
const vm = new VMBuilder(getBasicVMBuilder()) | ||
.setProvisionSource(provisionSources.URL) | ||
.setDisks([rootDisk]) | ||
.setFlavor(flavorConfigs.Tiny) |
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 are still some instances where specifying Tine is not required due to getBasicVM/VMT builders
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.
Removed all the unneccessary ones
import { VMTemplateBuilder } from '../models/vmtemplateBuilder'; | ||
import { VirtualMachineTemplate } from '../models/virtualMachineTemplate'; | ||
|
||
/** |
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.
+1
.setFlavor(flavorConfigs.Medium) | ||
.setWorkload(Workload.DESKTOP) | ||
.setDisks([hddDisk]) | ||
.setStartOnCreation(false) |
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.
+ in multiple places this is not necessary
import { VMBuilderData } from '../types/vm'; | ||
import { VM_ACTION, TAB, VM_STATUS } from '../utils/constants/vm'; | ||
import { MatchLabels } from 'public/module/k8s'; | ||
import { Wizard } from './wizard'; | ||
|
||
const noConfirmDialogActions: VM_ACTION[] = [VM_ACTION.Start, VM_ACTION.Clone]; | ||
|
||
export class VirtualMachine extends BaseVirtualMachine { |
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.
ping
2babadf
to
4ea5af3
Compare
const additionalBuilder = new VMBuilder().setData(additionalBuilderData); | ||
newComputedBuilders[`config ${idx}-${additionalBuilderId}`] = new VMBuilder(builder) | ||
.setBuilderAttributes(additionalBuilder) | ||
.generateNameForPrefix(`${additionalBuilderId}`); |
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.
except in this case - I am not sure if we don't want to include the appropriate metadata of the configuration in the name
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.
okay, changed back
return this; | ||
} | ||
|
||
public appendBuilder(builder: BaseVMBuilder<T>) { |
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.
thanks for removing the unnecessary storageAttributes - makes it easier to read
import { Disk, Network } from '../types/types'; | ||
import { BaseVMBuilderData } from '../types/vm'; | ||
|
||
export class BaseVirtualMachine<T extends BaseVMBuilderData> extends KubevirtUIResource { |
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.
+1, can we use this also for the VirtualMachineTemplate?
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.
Yes, KubeVirtUIResource
is now generic, from that we inherit BaseVirtualMachine
with VMBuilderData
and VirtualMachineTemplate
with VMTemplateBuilderData
import { BaseVMBuilderData } from '../types/vm'; | ||
|
||
export class BaseVirtualMachine<T extends BaseVMBuilderData> extends KubevirtUIResource { | ||
protected data: T; |
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.
can we create a constructor here and use it in children classes? and pass name/namespace/model to super?
eg
constructor(data: T, model: K8sKind) {
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.
With regards to the comment above, I added this constructor to KubevirtUIResource
6067bc6
to
552d59f
Compare
/test images |
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.
looking pretty good
super({ ...config, model: VirtualMachineModel }); | ||
constructor(data: VMBuilderData) { | ||
super(data, VirtualMachineModel); | ||
this.data = data; |
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 this line needed?
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.
Nope. Removed.
export class VirtualMachineTemplate extends KubevirtUIResource<VMTemplateBuilderData> { | ||
constructor(data: VMTemplateBuilderData) { | ||
super(data, VirtualMachineTemplateModel); | ||
this.data = data; |
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.
same 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.
fixed
|
||
export class KubevirtUIResource extends UIResource { | ||
async navigateToListView() { | ||
export class KubevirtUIResource<T extends BaseVMBuilderData> extends UIResource { |
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.
+1, nice refactoring
let vmTemplate: VirtualMachineTemplate; | ||
const vmTemplate = new VMTemplateBuilder(getBasicVMTBuilder()) | ||
.setProvisionSource(provisionSources.Container) | ||
.setFlavor(flavorConfigs.Tiny) |
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.
+here, superfluous setters
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.
Removed.
}, {} as VMBuilderData); | ||
const additionalBuilder = new VMBuilder().setData(additionalBuilderData); | ||
newComputedBuilders[`config ${idx}-${additionalBuilderId}`] = new VMBuilder(builder) | ||
.setBuilderAttributes(additionalBuilder) |
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 know I am getting annoying with this, but can we have ${idx}-${additionalBuilderId}
and all the metadata in the name?
This could help with debugging when having large number of combinations.
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.
Fixed. Added Workload to the name as well
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.
thanks
Good job! This is a great enhancement for kubevirt tests. /lgtm |
Awesome! Thanks for your patient guidance with this. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rawagner, 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. |
Adding VMBuilder and VMTemplateBuilder classes to instantiate VirtualMachines and VirtualMachineTemplate and the data for creating them via VM Wizard.
What's missing: