Use getTemplateVirtualMachineObject in template utils #678

hstastna · 2022-06-27T19:02:38Z

📝 Description

This is a small refactoring PR related to:
using getTemplateVirtualMachineObject function to get the VM object where needed, instead of hardcoced template.objects[0], in the template utils/selectors.

hstastna · 2022-06-27T19:04:48Z

@glekner @avivtur @metalice @pcbailey @yzamir @vojtechszocs please review

yaacov · 2022-06-28T05:21:22Z

/lgtm

nice !

yaacov · 2022-06-28T05:22:00Z

/retest

openshift-bot · 2022-06-28T05:25:32Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

yaacov · 2022-06-28T05:27:15Z

src/views/templates/utils/selectors.ts


 export const getTemplateProviderName = (template: V1Template): string =>
  getAnnotation(template, ANNOTATIONS.providerName, null) ||
  getAnnotation(template, ANNOTATIONS.providerDisplayName, null);

-export const getTemplateWorkload = (template: V1Template): WORKLOADS =>
-  template?.objects[0]?.spec?.template?.metadata?.annotations?.[VM_WORKLOAD_ANNOTATION];
+export const getTemplateWorkload = (template: V1Template): string =>


Suggested change

export const getTemplateWorkload = (template: V1Template): string =>

export const getTemplateWorkload = (template: V1Template): WORKLOADS =>

you should not change the signature of a common lib method
( if you do, you need to go over the code and make sure it's ok )

I changed that because it didn't work, compilation was failing which is obvious if you think deeper about the change we did here. Tried many things and wasn't able to use the better type. But I haven't tried the force cast yet...

openshift-bot · 2022-06-28T05:37:31Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

yaacov · 2022-06-28T16:32:37Z

src/views/templates/utils/selectors.ts

+  getTemplateVirtualMachineObject(template)?.spec?.template?.metadata?.annotations?.[
+    VM_WORKLOAD_ANNOTATION
+  ] as WORKLOADS;


Suggested change

getTemplateVirtualMachineObject(template)?.spec?.template?.metadata?.annotations?.[

VM_WORKLOAD_ANNOTATION

] as WORKLOADS;

{

const workload = getTemplateVirtualMachineObject(template)?.spec?.template?.metadata?.annotations?.[

VM_WORKLOAD_ANNOTATION

];

return workload in WORKLOADS ? workload as WORKLOADS : null;

}

I'd prefer the actual version, as it is shorter and self explanatory enough. Returns undefined if other workload profile than one of the three values present, and then "Other" is displayed in this case.

Returns undefined if other workload

can you check, as does not check the value is actually a workload [1].

another option to force WORKLOADS is to do

return WORKLOADS[workload as WORKLOADS]

[1] https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#type-assertions

Okay. And so this leads me to ask about the correct behavior for editing workload profile:
What do we want to display in the Edit Workload profile modal, if there is other workload profile, other than one of those 3, present in the template's yaml? (I can create such a template easily, when editing the yaml when creating a template)

we currently do 'Other', which is fine ...
do you think showing the verbatim value from the yaml makes more sense ?

Why is it "not nice"? What does it mean? TS is fine with that. The return value is a string which is what we want, plus WORKLOADS is giving an extra and useful information for the other developers about what we expect there in most of the cases.

Why is it "not nice"?

it's not correct, it will create a variable with type A but the value will be of type B, this is not correct,
types are used to help you assert that the value is of a type, if you use as without checking the value
you break the typing mechanism.

for example:

enum WORKLOADS { desktop = 'desktop', server = 'server', highperformance = 'highperformance', } const a = 'hi' const b = 'desktop' const returnsWorlads = (input: string): WORKLOADS => input as WORKLOADS; const aWorkload: WORKLOADS = returnsWorlads(a); // do something that assume aWorkload is of type WORKLOADS // and it may crash or not depending on the input console.log( returnsWorlads(aWorkload) ) // hi const bWorkload: WORKLOADS = returnsWorlads(b); // do something that assume aWorkload is of type WORKLOADS // and it may crash or not depending on the input console.log( returnsWorlads(bWorkload) ) // desktop

Yes it works like that and IMO this is the behavior we want here now. I don't see here any conflict with what I've written earlier.
Btw, originally, at the beginning of this conversation, there was string return type, but you didn't like it...

there was string return type, but you didn't like it...

:-) i liked it, but the tests failed

The tests failed because I forgot to change type to string in some related places, but you had a problem with not using WORKLOADS type, originally.

Use getTemplateVirtualMachineObject function to get the VM object where needed, instead of hardcoced template.objects[0].

yaacov · 2022-06-30T12:49:24Z

/lgtm

openshift-ci · 2022-06-30T12:49:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hstastna, yaacov

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:

~~OWNERS~~ [yaacov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-06-30T20:23:32Z

@hstastna: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/kubevirt-e2e-aws	`9e89b60`	link	unknown	`/test kubevirt-e2e-aws`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot requested review from metalice and tnisan June 27, 2022 19:02

openshift-ci bot assigned yaacov Jun 28, 2022

openshift-ci bot added lgtm Passed code review, ready for merge approved This issue is something we want to fix labels Jun 28, 2022

yaacov reviewed Jun 28, 2022

View reviewed changes

yaacov removed the lgtm Passed code review, ready for merge label Jun 28, 2022

hstastna force-pushed the Use_getTemplateVirtualMachineObject_template_utils branch 4 times, most recently from e5a836d to de61088 Compare June 28, 2022 15:30

yaacov reviewed Jun 28, 2022

View reviewed changes

hstastna force-pushed the Use_getTemplateVirtualMachineObject_template_utils branch from de61088 to 1e278e1 Compare June 29, 2022 13:48

hstastna requested a review from yaacov June 29, 2022 18:50

hstastna force-pushed the Use_getTemplateVirtualMachineObject_template_utils branch from 1e278e1 to 4baeee2 Compare June 30, 2022 12:31

Use getTemplateVirtualMachineObject in template utils

9e89b60

Use getTemplateVirtualMachineObject function to get the VM object where needed, instead of hardcoced template.objects[0].

hstastna force-pushed the Use_getTemplateVirtualMachineObject_template_utils branch from 4baeee2 to 9e89b60 Compare June 30, 2022 12:38

openshift-ci bot added the lgtm Passed code review, ready for merge label Jun 30, 2022

openshift-ci bot merged commit 97894c0 into kubevirt-ui:main Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use getTemplateVirtualMachineObject in template utils #678

Use getTemplateVirtualMachineObject in template utils #678

hstastna commented Jun 27, 2022

hstastna commented Jun 27, 2022

yaacov commented Jun 28, 2022

yaacov commented Jun 28, 2022

openshift-bot commented Jun 28, 2022

yaacov Jun 28, 2022

hstastna Jun 28, 2022

openshift-bot commented Jun 28, 2022

yaacov Jun 28, 2022

hstastna Jun 29, 2022

yaacov Jun 29, 2022

hstastna Jun 29, 2022 •

edited

Loading

yaacov Jun 29, 2022

hstastna Jun 30, 2022

yaacov Jun 30, 2022

hstastna Jun 30, 2022

yaacov Jun 30, 2022

hstastna Jun 30, 2022

yaacov commented Jun 30, 2022

openshift-ci bot commented Jun 30, 2022

openshift-ci bot commented Jun 30, 2022

	export const getTemplateWorkload = (template: V1Template): string =>
	export const getTemplateWorkload = (template: V1Template): WORKLOADS =>

Use getTemplateVirtualMachineObject in template utils #678

Use getTemplateVirtualMachineObject in template utils #678

Conversation

hstastna commented Jun 27, 2022

📝 Description

hstastna commented Jun 27, 2022

yaacov commented Jun 28, 2022

yaacov commented Jun 28, 2022

openshift-bot commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hstastna Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Jun 30, 2022

openshift-ci bot commented Jun 30, 2022

openshift-ci bot commented Jun 30, 2022

hstastna Jun 29, 2022 •

edited

Loading