Bug 1871103: Remove 'Template' field from VM wizard #6402

irosenzw · 2020-08-23T08:41:15Z

When creating a VM from template the wizard title will be:
Create Virtual Machine from <template name> template

Signed-off-by: Ido Rosenzwig irosenzw@redhat.com

openshift-ci-robot · 2020-08-23T08:41:22Z

@irosenzw: This pull request references Bugzilla bug 1871103, 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.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1871103: Remove 'Template' field from VM wizard

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.

irosenzw · 2020-08-23T10:40:43Z

/retest

atiratree · 2020-08-24T10:34:02Z

...ges/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.tsx

@@ -73,14 +71,6 @@ export const VMSettingsTabComponent: React.FC<VMSettingsTabComponentProps> = ({
          />
        </FormField>
      </FormFieldMemoRow>


how will the user know which template was selected in virtualization/templates ?

what do you propose ? keep the template's dropdown or add a label (or another component) to show the chosen template ?

I guess having a label should be enough. Although there will probably be some logic , when the template doesn't exist, etc. So having a component might be beneficial.

openshift-ci-robot · 2020-08-26T12:33:00Z

@irosenzw: This pull request references Bugzilla bug 1871103, which is valid.

3 validation(s) were run on this bug

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

In response to this:

Bug 1871103: Remove 'Template' field from VM wizard

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-robot · 2020-08-26T12:33:16Z

@irosenzw: This pull request references Bugzilla bug 1871103, which is valid.

3 validation(s) were run on this bug

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

In response to this:

Bug 1871103: Remove 'Template' field from VM wizard

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.

irosenzw · 2020-08-26T12:35:07Z

atiratree · 2020-08-26T12:39:16Z

...ages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/user-templates.tsx

-import { FormField, FormFieldType } from '../../form/form-field';
-import { ignoreCaseSort } from '../../../../utils/sort';
-import { VMSettingsField } from '../../types';
-import {


what about all these strings?

atiratree · 2020-08-26T12:41:23Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard.tsx

@@ -290,6 +291,7 @@ class CreateVMWizardComponent extends React.Component<CreateVMWizardComponentPro
 const wizardStateToProps = (state, { reduxID }) => ({
  isLastTabErrorFatal: isLastStepErrorFatal(state, reduxID),
  tabsMetadata: getStepsMetadata(state, reduxID),
+  userTemplateName: iGetCommonData(state, reduxID, VMWizardProps.userTemplateName),
  // fetch data from store to detect and fire changes
  ...[...DetectCommonDataChanges]
    .filter((v) => v !== VMWizardProps.storageClassConfigMap) // passed directly


we don't have to load all the userTemplates anymore. Just the one is enough.

plus we don't have to load it at all when we are not in the userTemplateMode

atiratree · 2020-08-26T12:44:52Z

...ges/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.tsx

@@ -73,14 +71,6 @@ export const VMSettingsTabComponent: React.FC<VMSettingsTabComponentProps> = ({
          />
        </FormField>
      </FormFieldMemoRow>
-      <UserTemplates
-        userTemplateField={getField(VMSettingsField.USER_TEMPLATE)}


we can remove VMSettingsField.USER_TEMPLATE field as it is not used anymore

plus all the not needed logic

atiratree · 2020-08-26T12:48:42Z

@matthewcarleton what is your opinion on this design and what should we do when the template is missing? (somebody has wrong url)

matthewcarleton · 2020-08-26T14:57:16Z

@matthewcarleton what is your opinion on this design and what should we do when the template is missing? (somebody has wrong url)

Having the template name in the title of the wizard works. We could also have
Template
template-name-here
as the first label - read only input
WDYT @irosenzw ?

For when the template is missing
for the short term I think we'll have to have an error in the wizard that alerts them of the issue and how to fix it. More long term we are talking about Template status so that could help avoid selecting a template that has this issue.
This is if I follow you correctly. I wasn't aware of this type of issue.

irosenzw · 2020-08-26T15:28:39Z

@matthewcarleton can you please attach a design proposal so we can all be on the same page of what you are suggesting ?

atiratree · 2020-08-26T16:01:54Z

For when the template is missing
for the short term I think we'll have to have an error in the wizard that alerts them of the issue and how to fix it. More long term we are talking about Template status so that could help avoid selecting a template that has this issue.
This is if I follow you correctly. I wasn't aware of this type of issue.

I don't expect anything to be wrong template. I think the only issue will be that the user saved a template creation url and is trying to use, while that template has been already deleted

matthewcarleton · 2020-08-26T16:17:58Z

For when the template is missing
for the short term I think we'll have to have an error in the wizard that alerts them of the issue and how to fix it. More long term we are talking about Template status so that could help avoid selecting a template that has this issue.
This is if I follow you correctly. I wasn't aware of this type of issue.

I don't expect anything to be wrong template. I think the only issue will be that the user saved a template creation url and is trying to use, while that template has been already deleted
This would happen in the UI?

yes, but not that likely - you go to create from a template and somebody deletes it

nevertheless we should add a guard against it

Yes in the case where someone is trying to create from a template and then another user deletes that template it would be nice if they are aware right away and not have to wait for the creation to fail.

Template no longer available
It appears this template has been removed. Please contact your system administrator for more information.

Not sure if we can provide any additional info around the user that deleted it can we?

atiratree · 2020-08-26T16:25:36Z

yes, but not that likely - you go to create from a template and somebody deletes it

nevertheless we should add a guard against it

matthewcarleton · 2020-09-01T00:44:50Z

@irosenzw just want to make sure you saw my last comment. It posted strangely - but there is a response there to @suomiy latest comment.

atiratree · 2020-09-04T12:02:01Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard-footer.tsx

@@ -89,6 +94,8 @@ const CreateVMWizardFooterComponent: React.FC<CreateVMWizardFooterComponentProps
        const areMainTabsHidden = VM_WIZARD_DIFFICULT_TABS.some((tab) => steps[tab].isHidden);
        const isLastStepValid = steps[VMWizardTab.RESULT].isValid;

+        const isValidUserTemplate = !userTemplateName || (userTemplateName && isUserTemplateValid);


it would make more sense to call this isInvalidUserTemplate IMO

atiratree · 2020-09-04T12:19:38Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard.tsx

+          userTemplateName: {
+            value: props.userTemplateName,
+            initialized: false,
+            isValid: true,


we should not load all the userTemplates, but just the one we need.

In that case we don't need isValid value here, because the validity is already present in FirehoseResult object

It is a fluid value. It could easily happen that we open wizard and then somebody deletes the userTemplate - and in that case the wizard stops being valid

atiratree · 2020-09-04T12:20:41Z

...es/kubevirt-plugin/src/components/create-vm-wizard/error-components/resource-load-errors.tsx

@@ -10,6 +10,32 @@ import { K8sKind } from '@console/internal/module/k8s';
 import { TemplateModel } from '@console/internal/models';
 import { DataVolumeModel, VirtualMachineModel } from '../../../models';
 import { getExtraWSQueries } from '../selectors/selectors';
+import { iGetUserTemplateName, iGetUserTemplateValidity } from '../selectors/immutable/template';
+
+const userTemplateError = (state: any, wizardReduxID: string): Error => {


if we load one use template in that case we don't need a special handler like this one but a generic handler like VMWizardProps.userTemplates have right now

atiratree · 2020-09-04T12:23:11Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/redux/internal-actions.ts

@@ -37,6 +37,27 @@ export const vmWizardInternalActions: VMWizardInternalActions = {
    },
    type: InternalActionType.Update,
  }),
+  [InternalActionType.UpdateUserTemplateName]: (id, value) => ({


that is too many actions and it will not scale. Please create just a generic action like setCommonDataDataValue

and it seems you are dispatching it in just one place, so you don't need such granularity

atiratree · 2020-09-04T12:28:15Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

 };

 const selectedUserTemplateUpdater = (options: UpdateOptions) => {
  const { id, prevState, dispatch, getState } = options;
  const state = getState();
-  if (!hasVmSettingsChanged(prevState, state, id, VMSettingsField.USER_TEMPLATE)) {
+
+  if (!hasCommonDataChanged(prevState, state, id, VMWizardProps.userTemplateName)) {


we don't expect the name to change so the logic has to be redone.

atiratree · 2020-09-04T12:28:30Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

-      VMSettingsField.USER_TEMPLATE,
-    )
+    ) &&
+    !hasCommonDataChanged(prevState, state, id, VMWizardProps.userTemplateName)


no need to have it here

matthewcarleton · 2020-09-08T18:05:27Z

Create a VM from an existing template:

Create a VM from a non-existing template:

What if we said

Could not load requested template
requested template vm-templates-example-5 can not be found. It may have been removed.
Please contact your admin for more information or choose another template (this could link back to the template list)

atiratree · 2020-09-08T17:59:21Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard-footer.tsx

-});
+const stateToProps = (state, { wizardReduxID }) => {
+  const iUserTemplate = iGetCommonData(state, wizardReduxID, VMWizardProps.userTemplate);
+  const isInvalidUserTemplate = iUserTemplate ? !!iGetLoadError(iUserTemplate) : false;


it should be prob also disabled when it is loading

atiratree · 2020-09-08T18:09:04Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard.tsx

        getResource(TemplateModel, {
          namespace: 'openshift',
          prop: VMWizardProps.commonTemplates,
          matchLabels: { [TEMPLATE_TYPE_LABEL]: TEMPLATE_TYPE_BASE },
        }),
+        getResource(TemplateModel, {


this websocket should only run when isCreateTemplate

atiratree · 2020-09-08T18:15:22Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/selectors/combined.ts

    : operatingSystemsNative;

+  if (openshiftFlag) {
+    operatingSystems.push(iUserTemplate.toJS());


can fail on NPE

can we refactor this function so that we gather info only from iUserTemplate when we have it. Otherwise from iCommonTemplates?

atiratree · 2020-09-08T18:21:15Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard-footer.tsx

@@ -117,11 +120,12 @@ const CreateVMWizardFooterComponent: React.FC<CreateVMWizardFooterComponentProps
        const isFinishingStep = [VMWizardTab.REVIEW, VMWizardTab.RESULT].includes(activeStepID);
        const isLastStep = activeStepID === VMWizardTab.RESULT;

-        const isNextButtonDisabled = isAnyStepLocked;
-        const isReviewButtonDisabled = isAnyStepLocked;
+        const isNextButtonDisabled = isAnyStepLocked || isInvalidUserTemplate;


after some consideration. These metadata are used in multiple places - that is why this will not disable left nav when needed. We should track this invalidity in redux. I think a good place for this would be finalizeTabResolver. You can play with the logic there and get inspired with finalizeImportProviderStateUpdate.

first, I don't understand why it won't disable the left nav when needed, can you provide an example.
second, can we maybe add the functionality as a followup ?

delete the template why you are in a wizard. IMO it is better to do it here, as these inconsistencies add up and are hard to follow up.

another valid option IMO is to disable just the review and create button and let the wizard be navigable with the error on the top. But the navigation would be at least consistent

when the template gets deleted all the nav buttons are disabled (next, back, review and create)
and when the template gets create while in the wizard they are enabled again.
I don't see what is wrong with the logic here

the nav is enabled and it is possible to jump to other tabs

atiratree · 2020-09-08T18:49:18Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

 };

 const selectedUserTemplateUpdater = (options: UpdateOptions) => {
  const { id, prevState, dispatch, getState } = options;
  const state = getState();
-  if (!hasVmSettingsChanged(prevState, state, id, VMSettingsField.USER_TEMPLATE)) {
+
+  if (!hasCommonDataChanged(prevState, state, id, VMWizardProps.isUserTemplateInitialized)) {


why should here be something instead when the logic gets executed there?

atiratree · 2020-09-08T19:04:15Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx

    const params = {
-      userTemplate,
+      userTemplateName,


can we remove it from params. It does not seem necessary to have it there.

atiratree · 2020-09-08T19:04:57Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx

+    const vanillaTemplates = immutableListToShallowJS(iGetLoadedData(commonTemplates));
+
+    if (isUserTemplateValid) {
+      vanillaTemplates.push(userTemplate);


lets just use the one userTemplate when resolving os/flavor/workload

atiratree · 2020-09-08T19:06:32Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/workload-profile.tsx

-      concatImmutableLists(iGetLoadedData(commonTemplates), iGetLoadedData(userTemplates)),
-    );
+    const isUserTemplateLoaded = iGetIsLoaded(iUserTemplate);
+    const userTemplateName: string = isUserTemplateLoaded


same - no need to resolve the name here

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/types.ts

  | ChangedCommonDataProp;

 export type ChangedCommonData = Set<ChangedCommonDataProp>;

 export const DetectCommonDataChanges = new Set<ChangedCommonDataProp>([
  VMWizardProps.activeNamespace,
  VMWizardProps.openshiftFlag,
+  VMWizardProps.userTemplate,
  VMWizardProps.virtualMachines,


atiratree · 2020-09-08T19:09:43Z

frontend/packages/kubevirt-plugin/src/selectors/vm-template/combined-dependent.ts

@@ -45,12 +45,14 @@ export const getOperatingSystems = (templates: TemplateKind[], userTemplate: str

 export const getWorkloadProfiles = (
  templates: TemplateKind[],
-  { flavor, os, userTemplate }: { flavor: string; os: string; userTemplate: string },
+  { flavor, os, userTemplateName }: { flavor: string; os: string; userTemplateName: string },


repeating myself, but let's not pass the name but just the [template] for userTemplates

atiratree · 2020-09-08T19:14:46Z

What if we said

Could not load requested template
requested template vm-templates-example-5 can not be found. It may have been removed.
Please contact your admin for more information or choose another template (this could link back to the template list)

@matthewcarleton

We could do this, but I am against it at this time. This component handles all types of errors (of all resources and casuses) and if we start adding customization like this one on top of each other iit will become a mess.

We have to come either with a better generic solution to these errors or add a good extension framework.
If we try to do it here, this PR will get more out of scope.

atiratree · 2020-09-09T10:42:44Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/selectors/combined.ts

+  let operatingSystems = [];
+
+  if (openshiftFlag && iUserTemplate) {
+    operatingSystems.push(getTemplateOperatingSystems([iUserTemplate.toJS()]));


please use toJSON here or toShallowJS

atiratree · 2020-09-09T10:49:53Z

frontend/packages/kubevirt-plugin/src/selectors/vm-template/combined-dependent.ts

    ]);
  }
  return getTemplateOperatingSystems(getTemplatesOfLabelType(templates, TEMPLATE_TYPE_BASE));
 };

 export const getWorkloadProfiles = (
  templates: TemplateKind[],
-  { flavor, os, userTemplate }: { flavor: string; os: string; userTemplate: string },
+  { flavor, os, isUserTemplateValid }: { flavor: string; os: string; isUserTemplateValid: boolean },


why do we need isUserTemplateValid variable? can we just pass templates and that is all?

atiratree · 2020-09-09T10:50:33Z

frontend/packages/kubevirt-plugin/src/selectors/vm-template/combined-dependent.ts

@@ -34,48 +34,46 @@ export const getFlavorLabel = (flavor: string) => {
  return undefined;
 };

-export const getOperatingSystems = (templates: TemplateKind[], userTemplate: string) => {
-  if (userTemplate) {
+export const getOperatingSystems = (templates: TemplateKind[], userTemplateName: string) => {


same, why do we need to pass userTemplateName?

@irosenzw please take a look. This should work the same way as getWorkloadProfiles.

atiratree · 2020-09-09T10:50:39Z

frontend/packages/kubevirt-plugin/src/selectors/vm-template/combined-dependent.ts

+    workload,
+    os,
+    isUserTemplateValid,
+  }: { workload: string; os: string; isUserTemplateValid: boolean },


atiratree · 2020-09-09T10:52:01Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/workload-profile.tsx

+    const isUserTemplateValid = iGetIsLoaded(iUserTemplate) && !iGetLoadError(iUserTemplate);
+
+    const templates = isUserTemplateValid
+      ? [iGetLoadedData(iUserTemplate).toJS()]


please prefer toShallowJS here

atiratree · 2020-09-09T13:32:02Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/selectors/combined.ts

+  let operatingSystems = [];
+
+  if (openshiftFlag && iUserTemplate) {
+    operatingSystems.push(getTemplateOperatingSystems([toShallowJS(iUserTemplate)]));


array in an array. better to have an assignment?

atiratree · 2020-09-09T13:36:58Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/workload-profile.tsx

@@ -43,7 +45,6 @@ export const WorkloadProfile: React.FC<WorkloadProps> = React.memo(
          field={workloadProfileField}
          fieldType={FormFieldType.SELECT}
          loadingResources={{


it should be in loading resources if the wizard is in userTemplate mode

atiratree · 2020-09-09T13:37:30Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx


-    const workloadProfiles = getWorkloadProfiles(vanillaTemplates, params);
+    const workloadProfiles = getWorkloadProfiles(templates, params);

    const loadingResources = openshiftFlag
      ? {


loadingResources: same here

matthewcarleton · 2020-09-09T17:29:39Z

What if we said

Could not load requested template
requested template vm-templates-example-5 can not be found. It may have been removed.
Please contact your admin for more information or choose another template (this could link back to the template list)

@matthewcarleton

We could do this, but I am against it at this time. This component handles all types of errors (of all resources and casuses) and if we start adding customization like this one on top of each other iit will become a mess.

We have to come either with a better generic solution to these errors or add a good extension framework.
If we try to do it here, this PR will get more out of scope.

+1 to improving this in general - I think errors are most helpful when they are specific. These generic errors don't help the user take action to fix the problem.

atiratree · 2020-09-09T19:41:56Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/workload-profile.tsx

            commonTemplates,
+            iUserTemplate,


this should be only assigned in userTemplate mode

atiratree · 2020-09-09T20:01:47Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

 };

 const selectedUserTemplateUpdater = (options: UpdateOptions) => {
  const { id, prevState, dispatch, getState } = options;
  const state = getState();
-  if (!hasVmSettingsChanged(prevState, state, id, VMSettingsField.USER_TEMPLATE)) {
+
+  if (!hasCommonDataChanged(prevState, state, id, VMWizardProps.isUserTemplateInitialized)) {


I don't see any issue with it. It seems pretty simple:

const selectUserTemplateOnLoadedUpdater = (options: UpdateOptions) => { const { id, dispatch, getState } = options; const state = getState(); if ( iGetCommonData(state, id, VMWizardProps.isUserTemplateInitialized) || !iGetLoadedCommonData(state, id, VMWizardProps.userTemplate) ) { return; } dispatch( vmWizardInternalActions[InternalActionType.UpdateCommonDataValue]( id, [VMWizardProps.isUserTemplateInitialized], true, ), ); const iUserTemplate = iGetLoadedCommonData(state, id, VMWizardProps.userTemplate); const isDisabled = asDisabled(iUserTemplate != null, VMWizardProps.userTemplate); dispatch( vmWizardInternalActions[InternalActionType.UpdateVmSettings](id, { [VMSettingsField.PROVISION_SOURCE_TYPE]: { isDisabled }, [VMSettingsField.CONTAINER_IMAGE]: { isDisabled }, [VMSettingsField.IMAGE_URL]: { isDisabled }, [VMSettingsField.OPERATING_SYSTEM]: { isDisabled }, [VMSettingsField.CLONE_COMMON_BASE_DISK_IMAGE]: { isHidden: asHidden(iUserTemplate != null, VMWizardProps.userTemplate), }, }), ); prefillVmTemplateUpdater(options); };

atiratree · 2020-09-09T20:02:26Z

frontend/packages/kubevirt-plugin/src/selectors/vm-template/combined-dependent.ts

@@ -34,48 +34,46 @@ export const getFlavorLabel = (flavor: string) => {
  return undefined;
 };

-export const getOperatingSystems = (templates: TemplateKind[], userTemplate: string) => {
-  if (userTemplate) {
+export const getOperatingSystems = (templates: TemplateKind[], userTemplateName: string) => {


@irosenzw please take a look. This should work the same way as getWorkloadProfiles.

irosenzw · 2020-09-10T09:27:18Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard.tsx

    if (isCreateTemplate) {
      return CREATE_VM_TEMPLATE;
    }
    if (isProviderImport) {
      return IMPORT_VM;
    }
-    return CREATE_VM;
+    return iUserTemplate ? `${CREATE_VM} from ${iGetName(iUserTemplate)} template` : CREATE_VM;


already changed it

atiratree · 2020-09-10T09:02:41Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/workload-profile.tsx

-    );
+    const isUserTemplateValid = iGetIsLoaded(iUserTemplate) && !iGetLoadError(iUserTemplate);
+
+    const templates = isUserTemplateValid


There are 3 options here

if iUserTemplate { if isUserTemplateValid { use iUserTemplate } else { use [] } } else { use commonTemplates }

atiratree · 2020-09-10T09:04:09Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx

-    const vanillaTemplates = immutableListToShallowJS(
-      concatImmutableLists(iGetLoadedData(commonTemplates), iGetLoadedData(userTemplates)),
-    );
+    const templates = isUserTemplateValid


same here, there are 3 options

atiratree · 2020-09-10T09:04:49Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx

          commonTemplates,
          cnvBaseImages,
        }
      : {};

+    if (openshiftFlag && iUserTemplate) {
+      Object.assign(loadingResources, { [iUserTemplate]: iUserTemplate });


Suggested change

Object.assign(loadingResources, { [iUserTemplate]: iUserTemplate });

Object.assign(loadingResources, { iUserTemplate: iUserTemplate });

the key is just called iUserTemplate (this notation would resolve in a key called [object Object])

atiratree · 2020-09-10T09:05:10Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/workload-profile.tsx

+    };
+
+    if (iUserTemplate) {
+      Object.assign(loadingResources, { [iUserTemplate]: iUserTemplate });


Suggested change

Object.assign(loadingResources, { [iUserTemplate]: iUserTemplate });

Object.assign(loadingResources, { iUserTemplate: iUserTemplate });

atiratree · 2020-09-10T09:17:39Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx

@@ -144,7 +147,7 @@ export const OSFlavor: React.FC<OSFlavorProps> = React.memo(
          checkboxDescription: '',
        };

-        if (!userTemplate) {
+        if (!isUserTemplateValid) {


Suggested change

if (!isUserTemplateValid) {

if (!iUserTemplate) {

like this?

atiratree · 2020-09-10T09:19:04Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx

    let operatingSystemValidation;
    let flavorValidation;

    if (
-      iGetIsLoaded(commonTemplates) &&
-      iGetIsLoaded(userTemplates) &&
+      (iGetIsLoaded(commonTemplates) || isUserTemplateValid) &&


just loaded not valid and &&

atiratree · 2020-09-10T10:29:02Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

@@ -88,7 +56,7 @@ const selectedUserTemplateUpdater = (options: UpdateOptions) => {
      [VMSettingsField.IMAGE_URL]: { isDisabled },
      [VMSettingsField.OPERATING_SYSTEM]: { isDisabled },


I suppose we should disable also workload profile since it cannot be changed for the user template

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard-footer.tsx

@@ -118,7 +121,7 @@ const CreateVMWizardFooterComponent: React.FC<CreateVMWizardFooterComponentProps
        const isLastStep = activeStepID === VMWizardTab.RESULT;

        const isNextButtonDisabled = isAnyStepLocked;
-        const isReviewButtonDisabled = isAnyStepLocked;
+        const isReviewButtonDisabled = isAnyStepLocked || isInvalidUserTemplate;


atiratree · 2020-09-10T11:53:25Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard-footer.tsx

@@ -118,7 +121,8 @@ const CreateVMWizardFooterComponent: React.FC<CreateVMWizardFooterComponentProps
        const isLastStep = activeStepID === VMWizardTab.RESULT;

        const isNextButtonDisabled = isAnyStepLocked;
-        const isReviewButtonDisabled = isAnyStepLocked;
+        const isReviewButtonDisabled = isAnyStepLocked || isInvalidUserTemplate;
+        const isCreateButtonDisabled = isInvalidUserTemplate;


Suggested change

const isCreateButtonDisabled = isInvalidUserTemplate;

const isCreateButtonDisabled = isNextButtonDisabled || isInvalidUserTemplate;

When creating a VM from template the title will be: `Create Virtual Machine from <template name>` Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

atiratree · 2020-09-10T12:22:26Z

/lgtm

openshift-ci-robot · 2020-09-10T12:22:45Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~frontend/packages/kubevirt-plugin/OWNERS~~ [suomiy]

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

openshift-bot · 2020-09-10T14:02:57Z

/retest

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

openshift-bot · 2020-09-10T14:15:56Z

/retest

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

openshift-ci-robot · 2020-09-10T22:39:32Z

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

openshift/console#6402

Bugzilla bug 1871103 has been moved to the MODIFIED state.

In response to this:

Bug 1871103: Remove 'Template' field from VM wizard

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-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Aug 23, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 23, 2020

openshift-ci-robot requested review from jelkosz and rawagner August 23, 2020 08:41

atiratree reviewed Aug 24, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch from 307b29b to d469b72 Compare August 26, 2020 12:32

atiratree suggested changes Aug 26, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch 2 times, most recently from 6857641 to f57e0ba Compare September 3, 2020 17:24

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2020

irosenzw force-pushed the remove-template-field-vm-wizard branch 2 times, most recently from 8f7938e to 60793ca Compare September 3, 2020 17:34

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2020

irosenzw force-pushed the remove-template-field-vm-wizard branch 3 times, most recently from fd79b5a to 668c01f Compare September 3, 2020 18:04

atiratree suggested changes Sep 4, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch from 668c01f to e3edc7f Compare September 7, 2020 14:06

atiratree suggested changes Sep 8, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch from 2251e43 to 6de9114 Compare September 9, 2020 09:48

atiratree suggested changes Sep 9, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch from 6de9114 to fec2f7d Compare September 9, 2020 12:59

atiratree suggested changes Sep 9, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch from fec2f7d to 0804db0 Compare September 9, 2020 14:46

atiratree suggested changes Sep 9, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch 2 times, most recently from 81a5144 to 4be8724 Compare September 10, 2020 09:03

atiratree suggested changes Sep 10, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch 2 times, most recently from 7656c41 to c3f7ea3 Compare September 10, 2020 10:11

atiratree suggested changes Sep 10, 2020

View reviewed changes

irosenzw force-pushed the remove-template-field-vm-wizard branch from c3f7ea3 to 9660223 Compare September 10, 2020 11:36

atiratree suggested changes Sep 10, 2020

View reviewed changes

Bug 1871103: Remove 'Template' dropdown field from VM wizard

b7c474c

When creating a VM from template the title will be: `Create Virtual Machine from <template name>` Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

irosenzw force-pushed the remove-template-field-vm-wizard branch from 9660223 to b7c474c Compare September 10, 2020 12:20

openshift-ci-robot assigned atiratree Sep 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2020

openshift-merge-robot merged commit 8338461 into openshift:master Sep 10, 2020

spadgett added this to the v4.6 milestone Sep 11, 2020

	Object.assign(loadingResources, { [iUserTemplate]: iUserTemplate });
	Object.assign(loadingResources, { iUserTemplate: iUserTemplate });

		@@ -88,7 +56,7 @@ const selectedUserTemplateUpdater = (options: UpdateOptions) => {
		[VMSettingsField.IMAGE_URL]: { isDisabled },
		[VMSettingsField.OPERATING_SYSTEM]: { isDisabled },

	const isCreateButtonDisabled = isInvalidUserTemplate;
	const isCreateButtonDisabled = isNextButtonDisabled \|\| isInvalidUserTemplate;

Bug 1871103: Remove 'Template' field from VM wizard #6402

Bug 1871103: Remove 'Template' field from VM wizard #6402

Conversation

irosenzw commented Aug 23, 2020 • edited

openshift-ci-robot commented Aug 23, 2020

irosenzw commented Aug 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020

irosenzw commented Aug 26, 2020

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

atiratree commented Aug 26, 2020

matthewcarleton commented Aug 26, 2020

irosenzw commented Aug 26, 2020

atiratree commented Aug 26, 2020

matthewcarleton commented Aug 26, 2020 • edited

atiratree commented Aug 26, 2020

matthewcarleton commented Sep 1, 2020

Choose a reason for hiding this comment

atiratree Sep 4, 2020 • edited

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

matthewcarleton commented Sep 8, 2020

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

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

This comment was marked as resolved.

Choose a reason for hiding this comment

atiratree commented Sep 8, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcarleton commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

atiratree commented Sep 10, 2020

openshift-ci-robot commented Sep 10, 2020

openshift-bot commented Sep 10, 2020

openshift-bot commented Sep 10, 2020

openshift-ci-robot commented Sep 10, 2020

irosenzw commented Aug 23, 2020 •

edited

matthewcarleton commented Aug 26, 2020 •

edited

atiratree Sep 4, 2020 •

edited