Skip to content
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

Optional initial template for create vm wizard #4811

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Mar 24, 2020

Set initial template for create vm wizard

a - move name and description inputs to top of form.
b - add optional initial template to the vm wizard.
c - adjust links to the virtualization path.
d - add create vm link to the vm-template list.

Design: openshift/openshift-origin-design#350

Screenshot:
Peek 2020-03-25 18-30

@openshift-ci-robot openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Mar 24, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2020
@yaacov
Copy link
Member Author

yaacov commented Mar 24, 2020

@suomiy @glekner please revierw

/assigne @suomiy

@yaacov
Copy link
Member Author

yaacov commented Mar 24, 2020

/assign @suomiy

@yaacov
Copy link
Member Author

yaacov commented Mar 24, 2020

/test e2e-gcp-console
/test images

@yaacov yaacov changed the title kubevirt: set init template for vm wizard Optional initial template for create vm wizard Mar 25, 2020
@yaacov yaacov force-pushed the vm-wizard-move-name-and-description-to-top branch 3 times, most recently from 5d45e66 to 10edf2e Compare March 25, 2020 05:42
@yaacov
Copy link
Member Author

yaacov commented Mar 25, 2020

/test e2e-gcp-console

@yaacov yaacov force-pushed the vm-wizard-move-name-and-description-to-top branch 2 times, most recently from 095a114 to b678e44 Compare March 25, 2020 08:13
@yaacov
Copy link
Member Author

yaacov commented Mar 25, 2020

/test e2e-gcp-console

@yaacov yaacov force-pushed the vm-wizard-move-name-and-description-to-top branch 2 times, most recently from 836f63d to 1419d14 Compare March 25, 2020 09:39
@yaacov
Copy link
Member Author

yaacov commented Mar 25, 2020

@yfrimanm @matthewcarleton please review

@yaacov
Copy link
Member Author

yaacov commented Mar 25, 2020

/test e2e-gcp-console
/test analyze

@yaacov
Copy link
Member Author

yaacov commented Mar 25, 2020

/test e2e-gcp-console

1 similar comment
@yaacov
Copy link
Member Author

yaacov commented Mar 25, 2020

/test e2e-gcp-console

const getCreateLink = (
namespace: string,
itemName: 'yaml' | 'wizard' = 'wizard',
mode: 'template' | 'vm' = 'template',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make an enum of that?

],
tableColumnClasses,
);

VMTemplateTableHeader.displayName = 'VMTemplateTableHeader';

const getCreateLink = (
namespace: string,
itemName: 'yaml' | 'wizard' = 'wizard',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and from that?

@@ -576,6 +576,9 @@ export const CreateVMWizardPageComponent: React.FC<CreateVMWizardPageComponentPr
);
}

const userMode = new URLSearchParams(search).get('mode');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable can be extracted here

@@ -584,8 +587,9 @@ export const CreateVMWizardPageComponent: React.FC<CreateVMWizardPageComponentPr
return (
<Firehose resources={resources} doNotConnectToState>
<CreateVMWizard
isCreateTemplate={!path.includes('/virtualmachines/')}
isProviderImport={new URLSearchParams(search).get('mode') === 'import'}
isCreateTemplate={userMode === 'template'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use this enum here

],
tableColumnClasses,
);

VMTemplateTableHeader.displayName = 'VMTemplateTableHeader';

const getCreateLink = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please make a reusable funtion which includes also import and basic vm.

Basically this funtion + createLink from vm.tsx + enums for all the stuff

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could live probably under constants?

namespace: string,
itemName: 'yaml' | 'wizard' = 'wizard',
mode: 'template' | 'vm' = 'template',
template?: string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably better to have one object as parameters

[VMSettingsField.WORKLOAD_PROFILE]: 9,
[VMSettingsField.NAME]: 10,
[VMSettingsField.DESCRIPTION]: 11,
[VMSettingsField.NAME]: 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name returns to the the top again! :)


if (
iGetCommonData(state, id, VMWizardProps.isCreateTemplate) ||
iGetVmSettingAttribute(state, id, VMSettingsField.USER_TEMPLATE, 'initialized')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should set the initialized property according to the VMWizardProps.userTemplateName and VMWizardProps.isCreateTemplate in vm-settings-tab-initial-state.ts

so we would not need this check here VMWizardProps.isCreateTemplate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plus this should also break when there was no change to the templates

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about the initial state - shouldn't we set the value of template in the beggining so the user sees it? and maybe disable it for good for this wizard instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about the initial state - shouldn't we set the value of template in the beggining so the user sees it? and maybe disable it for good for this wizard instance?

done 🍰

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎂 🎉 fixed

@@ -576,6 +576,9 @@ export const CreateVMWizardPageComponent: React.FC<CreateVMWizardPageComponentPr
);
}

const userMode = new URLSearchParams(search).get('mode');
const userTemplateName = new URLSearchParams(search).get('template');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should make a null of this if the mode is something different than the template

@@ -54,6 +55,7 @@ const tableColumnClasses = [
tableColumnClassHiddenOnSmall,
tableColumnClassHiddenOnSmall,
tableColumnClassHiddenOnSmall,
tableColumnClass,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thses should be max 12 together

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed :-)

@matthewcarleton
Copy link
Contributor

@yaacov I'm not sure we want to allow them to switch the template in this flow do we?
When the work that @yfrimanm is finishing up comes in we'll have the "select template" as the first step in the wizard so if they really want to go back and choose a different one they can.
Providing it in a dropdown doesn't give them much info on the template so it's really not that helpful.

@yaacov
Copy link
Member Author

yaacov commented Mar 25, 2020

@yaacov I'm not sure we want to allow them to switch the template in this flow do we?

@matthewcarleton 👍
It's locked, will update the screen shot ...

@yaacov
Copy link
Member Author

yaacov commented Mar 25, 2020

@matthewcarleton I've updated the screen-shot in the description.
It shows the disabled template drop-down.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2020
@@ -40,6 +41,18 @@ export enum VMWizardProps {

export const ALL_VM_WIZARD_TABS = getStringEnumValues<VMWizardTab>(VMWizardTab);

export enum VMWizardName {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these enums are the interface - they should be probably in ./src/constants

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

@@ -576,6 +577,10 @@ export const CreateVMWizardPageComponent: React.FC<CreateVMWizardPageComponentPr
);
}

const searchParams = new URLSearchParams(search);
const userMode = searchParams.get('mode');
const userTemplateName = userMode === VMWizardMode.VM ? searchParams.get('template') || '' : '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const userTemplateName = userMode === VMWizardMode.VM ? searchParams.get('template') || '' : '';
const userTemplateName = userMode === VMWizardMode.VM && searchParams.get('template') || '';

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks !

@@ -44,6 +44,8 @@ export const getInitialVmSettings = (data: CommonData): VMSettings => {
[VMSettingsField.DESCRIPTION]: {},
[VMSettingsField.USER_TEMPLATE]: {
isHidden: hiddenByProviderOrTemplate,
initialized: isProviderImport || isCreateTemplate || !userTemplateName,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this better?

Suggested change
initialized: isProviderImport || isCreateTemplate || !userTemplateName,
initialized: !(isCreateTemplate && userTemplateName),

we don't need to check the other cases

const { id, dispatch, getState } = options;
const state = getState();

if (iGetVmSettingAttribute(state, id, VMSettingsField.USER_TEMPLATE, 'initialized')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please proceed also only if options.changedCommonData.has(VMWizardProps.userTemplates)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please use changedCommonData - it is more clear indicator and one can see the function prerequisites in the beginning of the function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

vmWizardInternalActions[InternalActionType.UpdateVmSettings](id, {
[VMSettingsField.USER_TEMPLATE]: {
initialized: true,
value: userTemplateName,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value is already set

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although I worry a bit that it might cause bugs in the future if somebody is listening only on the string value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets leave it for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

/>
);

const optionsList = sortedNames.map((name) => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no good reason to have this extracted IMO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right ! fixed

({ userTemplateField, userTemplates, commonTemplates, onChange, openshiftFlag }) => {
({
userTemplateField,
userTemplateName,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is very confusing to read a variable like this in this context.
Can we rename it here?

like: forceSingleUserTemplateName ?

@@ -163,19 +165,7 @@ const getCreateProps = ({ namespace }: { namespace: string }) => {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also use the constants to create the object keys above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -51,3 +52,20 @@ export const getVMLikeModelDetailPath = (
namespace: string,
name: string,
) => `${getVMLikeModelListPath(isCreateTemplate, namespace)}/${name}`;

export const getVMWizardCreateLink = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this live rather in ./url.ts or ./endpoints.ts or something like that

utils/utils.ts should only for the most random stuff we can't place anywhere else

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

const wizardMode = itemName === VMWizardName.IMPORT ? VMWizardMode.IMPORT : mode;

const type = itemName === VMWizardName.YAML ? '~new' : '~new-wizard';
const args = `${wizardMode ? `mode=${wizardMode}` : ''}${
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use URLSearchParams here? as it is less prone to mistakes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

@yaacov yaacov force-pushed the vm-wizard-move-name-and-description-to-top branch from f59741b to e09bbd0 Compare March 25, 2020 18:21
@yaacov
Copy link
Member Author

yaacov commented Mar 26, 2020

/test frontend

@yaacov
Copy link
Member Author

yaacov commented Mar 26, 2020

/test analyze
/test frontend
/test backend

5 similar comments
@yaacov
Copy link
Member Author

yaacov commented Mar 26, 2020

/test analyze
/test frontend
/test backend

@yaacov
Copy link
Member Author

yaacov commented Mar 26, 2020

/test analyze
/test frontend
/test backend

@yaacov
Copy link
Member Author

yaacov commented Mar 26, 2020

/test analyze
/test frontend
/test backend

@yaacov
Copy link
Member Author

yaacov commented Mar 26, 2020

/test analyze
/test frontend
/test backend

@yaacov
Copy link
Member Author

yaacov commented Mar 26, 2020

/test analyze
/test frontend
/test backend

@yaacov
Copy link
Member Author

yaacov commented Mar 26, 2020

ci/prow/e2e-gcp-console


const userTemplates = iGetLoadedCommonData(state, id, VMWizardProps.userTemplates);
if (!userTemplates) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for this return

export const selectUserTemplateOnLoadedUpdater = (options: UpdateOptions) => {
const { id, dispatch, getState } = options;
const state = getState();
if (options.changedCommonData.has(VMWizardProps.userTemplates)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would this even work?

if user templates changed
    do not do anything 

please look at the other updaters for examples on how these guards should work

const userTemplate = iGetLoadedCommonData(state, id, VMWizardProps.userTemplates, List()).find(
(template) => iGetName(template) === userTemplateName,
);

if (!userTemplate) {
return asValidationObject(
'Template is missing, check template name',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should ask somebody from UX to have a better message :)

it is not that the template name is wrong - it is just the template is missing for unknown reason (probably got deleted right now)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to be less instructive :-)


const optionsList = !forceSingleUserTemplateName && hasUserTemplates && (
<>
{optionNoTemplate}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we inline the variable here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, I like it 👍

params.append('template', template);
}

return `/k8s/ns/${namespace || 'default'}/virtualization/${type}${params ? `?${params}` : ''}`; // covers 'yaml', new-wizard and default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the params is an object here so it is always true - we should first make it a string and then do the comparison for appending "?...."

@@ -88,7 +88,7 @@ const validateUserTemplate: VmSettingsValidator = (field, options) => {

if (!userTemplate) {
return asValidationObject(
'Template is missing, check template name',
"Can't verify tempalte, template is missing",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-) thanks !

@@ -35,10 +35,6 @@ export const selectUserTemplateOnLoadedUpdater = (options: UpdateOptions) => {
}

const userTemplateName = iGetCommonData(state, id, VMWizardProps.userTemplateName);
if (!userTemplateName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong return removed ^^ this return is quite useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ :-)

@@ -26,7 +26,7 @@ import { prefillVmTemplateUpdater } from './prefill-vm-template-state-update';
export const selectUserTemplateOnLoadedUpdater = (options: UpdateOptions) => {
const { id, dispatch, getState } = options;
const state = getState();
if (options.changedCommonData.has(VMWizardProps.userTemplates)) {
if (!options.changedCommonData.has(VMWizardProps.userTemplates)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we check for the initialized first? As this updater will be checked only few seconds after opening and for the rest of life of the wizard the initialized prop should be enough

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@atiratree
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@openshift-merge-robot openshift-merge-robot merged commit 71a4851 into openshift:master Mar 26, 2020
@spadgett spadgett added this to the v4.5 milestone Mar 31, 2020
@atiratree
Copy link
Member

Btw, shouldn't we have the Create Virtual Machine option in the template detail as well, in the actions kebab? Shouldn't we also move this action into the template list kebab for consistency as well?

@yaacov @yfrimanm @matthewcarleton

@matthewcarleton
Copy link
Contributor

Btw, shouldn't we have the Create Virtual Machine option in the template detail as well, in the actions kebab? Shouldn't we also move this action into the template list kebab for consistency as well?

@yaacov @yfrimanm @matthewcarleton

+1 it was in the design :)

@yfrimanm
Copy link

yfrimanm commented Apr 6, 2020 via email

@yaacov
Copy link
Member Author

yaacov commented Apr 6, 2020

shouldn't we have the Create Virtual Machine option in the template detail as well,

+1 it was in the design :)

@yfrimanm @matthewcarleton hi, thanks for the feedback, are you sure we have a design for chanes in the template detail page, I'm looking at this design[1] what am I missing ?

[1] http://openshift.github.io/openshift-origin-design/designs/virtualization/4.x/Templates/Templates.html

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Apr 6, 2020

ah ya I'm looking at the list view kebab for the "create virtual machine" and it's not there and should be according to the design.
image
Looking at the design we don't specifically show the template design although I'd assume the actions from that view would be the same as the actions from the list view of the templates right?

It looks like maybe the confusion came from the inline action for creating a vm from the template in the list view.

@yaacov
Copy link
Member Author

yaacov commented Apr 6, 2020

@matthewcarleton the design suggest two options [1]:
a - A link (button) to "create vm".
b - Adds a "create vm" action to the drop down.

If we want to add both action and link (button), we will need to update the design.

[1] http://openshift.github.io/openshift-origin-design/designs/virtualization/4.x/Templates/Templates.html#virtual-machine-templates-tab

@matthewcarleton
Copy link
Contributor

sorry for the confusion @yaacov
When I initially read this line in the design:

From the Virtualization left nav item the user goes to the Virtual machine templates tab where they can see the full list of templates. From this point, we suggest 2 options to create a VM

I took it as both options are included and the user can choose either to create a VM. I think what happened was with the second design I didn't notice the action was gone b/c the kebab menu covered it.
I agree we need to update the design to make it more clear. @yfrimanm Do you mind putting up a new PR to do this?

@atiratree
Copy link
Member

IMO we should have at least kebab option in both places, so that the user can expect the same behavior in the list and in the detail.

@matthewcarleton
Copy link
Contributor

IMO we should have at least kebab option in both places, so that the user can expect the same behavior in the list and in the detail.

agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants