Bug 2139574: blank vm name in customize #987

upalatucci · 2022-11-28T09:57:05Z

📝 Description

Don't re-process the template just for the VM name. Use the already processed template in the parent component.
If the template is not processable due to required parameters, canQuickCreate is false and the user would not have the name input field. The Customize page would generate the name instead (if generatable).

🎥 Demo

Before

simplescreenrecorder-2022-12-13_14.40.07.mp4

After

correct-2022-12-13_14.41.22.mp4

openshift-ci · 2022-11-28T09:57:17Z

@upalatucci: This pull request references Bugzilla bug 2139574, 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

bug is open, matching expected state (open)
bug target release (4.13.0) matches configured target release for branch (4.13.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2139574: blank vm name in customize

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.

hstastna

Using the already processed template in the parent component is a great way to get the VM name, thanks for this, Ugo! Looks like this is the improvement PR of #787 :)

However, the code changes don't work in one specific case - when WE need to generate the VM name (I mean the name with those 3 human readable words - as we used to do earlier by default no matter the template NAME parameter). For this case, the empty VM name field occurs in the drawer already (the VM name should already be generated - by generateVMName function we have already implemented in our code, this code should be executed in this case):

See how the yaml looks like in this case (we need to support this case, too):

This happens (I mean empty Name field) also when the VM cannot be quick created (and has the yaml as above - orange rectangle):

Note that the buggy case mentioned above worked well without code changes in this PR.

...alog/templatescatalog/components/TemplatesCatalogDrawer/TemplatesCatalogDrawerCreateForm.tsx

upalatucci · 2022-11-28T14:22:54Z

thanks @hstastna i've just fixed that case

hstastna

The issue I've mentioned earlier is fixed! 👍🏽
Now just one little thing and then I think we are good to go, Ugo! 😊

...alog/templatescatalog/components/TemplatesCatalogDrawer/TemplatesCatalogDrawerCreateForm.tsx

hstastna · 2022-11-29T15:28:35Z

@avivtur @metalice @pcbailey @vojtechszocs please review

metalice · 2022-11-29T15:35:19Z

...alog/templatescatalog/components/TemplatesCatalogDrawer/TemplatesCatalogDrawerCreateForm.tsx

-                  onChange={(v) => setStartVM(v)}
-                  label={t('Start this VirtualMachine after creation')}
-                />
+                <Alert variant="danger" title={t('Quick create error')} isInline>


U can use AlertVariant from patternfly

Hmm, what is the preferred component we should use and why? I don't see much difference, at least from the functional point of view, but maybe there is some difference...

done thanks

avivtur

according to your videos I can't understand if you really fixed the bug, it still seems stuck at a loading skeleton after you click on customize, can you please provide a clip including the vm being created?

src/views/catalog/customize/utils.ts

...alog/templatescatalog/components/TemplatesCatalogDrawer/TemplatesCatalogDrawerCreateForm.tsx

upalatucci · 2022-11-30T13:17:29Z

OMG my videos are so bad ahahahah!
I thought they were good but they don't even show the real issue.
The real issue is that when you click really fast the 'Customize' button on the catalog, the vmName does not show up on the next page. I'll update the video soon

hstastna · 2022-11-30T13:28:03Z

according to your videos I can't understand if you really fixed the bug, it still seems stuck at a loading skeleton after you click on customize, can you please provide a clip including the vm being created?

I've tested this PR (when our clusters were working) and I was not able to reproduce the issue anymore. The issue is presented in the bugzilla (video).

upalatucci · 2022-11-30T13:56:24Z

I could reproduce it. You have to be fast or set up a throttling in the network dev tools

openshift-ci · 2022-12-13T13:40:37Z

@upalatucci: This pull request references Bugzilla bug 2139574, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.13.0) matches configured target release for branch (4.13.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2139574: blank vm name in customize

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.

openshift-ci · 2022-12-13T13:42:04Z

@upalatucci: This pull request references Bugzilla bug 2139574, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.13.0) matches configured target release for branch (4.13.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2139574: blank vm name in customize

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.

openshift-ci · 2022-12-13T13:59:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avivtur, hstastna, 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:

~~OWNERS~~ [avivtur]

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

upalatucci · 2022-12-13T20:08:26Z

/retest

openshift-ci · 2022-12-13T21:37:46Z

@upalatucci: All pull requests linked via external trackers have merged:

Bugzilla bug 2139574 has been moved to the MODIFIED state.

In response to this:

Bug 2139574: blank vm name in customize

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.

openshift-ci bot requested review from hstastna and tnisan November 28, 2022 09:57

openshift-ci bot added bugzilla/severity-medium bugzilla/valid-bug labels Nov 28, 2022

openshift-ci bot requested a review from gouyang November 28, 2022 09:57

hstastna suggested changes Nov 28, 2022

View reviewed changes

openshift-ci bot assigned hstastna Nov 28, 2022

metalice reviewed Nov 28, 2022

View reviewed changes

...alog/templatescatalog/components/TemplatesCatalogDrawer/TemplatesCatalogDrawerCreateForm.tsx Outdated Show resolved Hide resolved

upalatucci force-pushed the process-template-customize branch from 9b381b5 to a53054c Compare November 28, 2022 14:21

hstastna reviewed Nov 28, 2022

View reviewed changes

...alog/templatescatalog/components/TemplatesCatalogDrawer/TemplatesCatalogDrawerCreateForm.tsx Outdated Show resolved Hide resolved

upalatucci force-pushed the process-template-customize branch from a53054c to 0de7339 Compare November 29, 2022 09:53

hstastna approved these changes Nov 29, 2022

View reviewed changes

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

metalice reviewed Nov 29, 2022

View reviewed changes

avivtur reviewed Nov 30, 2022

View reviewed changes

src/views/catalog/customize/utils.ts Show resolved Hide resolved

...alog/templatescatalog/components/TemplatesCatalogDrawer/TemplatesCatalogDrawerCreateForm.tsx Outdated Show resolved Hide resolved

upalatucci force-pushed the process-template-customize branch from 0de7339 to 5a6d8c2 Compare November 30, 2022 13:28

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

upalatucci force-pushed the process-template-customize branch from 5a6d8c2 to 2f56236 Compare December 1, 2022 10:09

blank vm name

893005c

upalatucci force-pushed the process-template-customize branch from 2f56236 to 893005c Compare December 1, 2022 10:09

avivtur approved these changes Dec 13, 2022

View reviewed changes

openshift-ci bot assigned avivtur Dec 13, 2022

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

openshift-ci bot added the approved This issue is something we want to fix label Dec 13, 2022

openshift-merge-robot merged commit 2932b63 into kubevirt-ui:main Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2139574: blank vm name in customize #987

Bug 2139574: blank vm name in customize #987

upalatucci commented Nov 28, 2022 •

edited

openshift-ci bot commented Nov 28, 2022

hstastna left a comment •

edited

upalatucci commented Nov 28, 2022

hstastna left a comment

hstastna commented Nov 29, 2022

metalice Nov 29, 2022

hstastna Nov 30, 2022

upalatucci Dec 1, 2022

avivtur left a comment •

edited

upalatucci commented Nov 30, 2022 •

edited

hstastna commented Nov 30, 2022 •

edited

upalatucci commented Nov 30, 2022 •

edited

openshift-ci bot commented Dec 13, 2022

openshift-ci bot commented Dec 13, 2022

openshift-ci bot commented Dec 13, 2022

upalatucci commented Dec 13, 2022

openshift-ci bot commented Dec 13, 2022

Bug 2139574: blank vm name in customize #987

Bug 2139574: blank vm name in customize #987

Conversation

upalatucci commented Nov 28, 2022 • edited

📝 Description

🎥 Demo

openshift-ci bot commented Nov 28, 2022

hstastna left a comment • edited

Choose a reason for hiding this comment

upalatucci commented Nov 28, 2022

hstastna left a comment

Choose a reason for hiding this comment

hstastna commented Nov 29, 2022

metalice Nov 29, 2022

Choose a reason for hiding this comment

hstastna Nov 30, 2022

Choose a reason for hiding this comment

upalatucci Dec 1, 2022

Choose a reason for hiding this comment

avivtur left a comment • edited

Choose a reason for hiding this comment

upalatucci commented Nov 30, 2022 • edited

hstastna commented Nov 30, 2022 • edited

upalatucci commented Nov 30, 2022 • edited

openshift-ci bot commented Dec 13, 2022

openshift-ci bot commented Dec 13, 2022

openshift-ci bot commented Dec 13, 2022

upalatucci commented Dec 13, 2022

openshift-ci bot commented Dec 13, 2022

upalatucci commented Nov 28, 2022 •

edited

hstastna left a comment •

edited

avivtur left a comment •

edited

upalatucci commented Nov 30, 2022 •

edited

hstastna commented Nov 30, 2022 •

edited

upalatucci commented Nov 30, 2022 •

edited