Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -289,16 +289,15 @@ export class CreateVMWizardComponent extends React.Component<CreateVMWizardCompo
const storages = immutableListToShallowJS(
iGetIn(this.props.stepData, [VMWizardTab.STORAGE, 'value']),
);
const templates = immutableListToShallowJS<TemplateKind>(
concatImmutableLists(
iGetLoadedData(this.props[VMWizardProps.commonTemplates]),
iGetLoadedData(this.props[VMWizardProps.userTemplates]),
),
);

const iUserTemplates = iGetLoadedData(this.props[VMWizardProps.userTemplates]);
const iCommonTemplates = iGetLoadedData(this.props[VMWizardProps.commonTemplates]);
let promise;

if (isProviderImport) {
const templates = immutableListToShallowJS<TemplateKind>(
concatImmutableLists(iUserTemplates, iCommonTemplates),
);
const { interOPVMSettings, interOPNetworks, interOPStorages } = await kubevirtInterOP({
vmSettings,
networks,
Expand All @@ -323,7 +322,8 @@ export class CreateVMWizardComponent extends React.Component<CreateVMWizardCompo
vmSettings,
networks,
storages,
templates,
iUserTemplates,
iCommonTemplates,
namespace: activeNamespace,
openshiftFlag,
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { TemplateValidations } from 'packages/kubevirt-plugin/src/utils/validations/template/template-validations';
import {
CloudInitField,
VMImportProvider,
Expand Down Expand Up @@ -208,4 +209,11 @@ export const vmWizardInternalActions: VMWizardInternalActions = {
},
type: InternalActionType.SetResults,
}),
[InternalActionType.SetTemplateValidations]: (id, value: TemplateValidations[]) => ({
payload: {
id,
value,
},
type: InternalActionType.SetTemplateValidations,
}),
};
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,11 @@ export default (state, action: WizardInternalAction) => {
[dialogID, 'tabs', VMWizardTab.VM_SETTINGS, 'value'],
fromJS(payload.value),
);
case InternalActionType.SetTemplateValidations:
return state.setIn(
[dialogID, 'commonData', 'dataIDReferences', 'templateValidations'],
Copy link
Member

Choose a reason for hiding this comment

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

just a note: dataIDReferences is for storing references of already present data in the redux store only. Not the original data.

payload.value,
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be fromJS(payload.value), ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because the payload isn't an immutable object but a TemplateValidations[] object.

);
default:
break;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ export const prefillVmTemplateUpdater = ({ id, dispatch, getState }: UpdateOptio
[VMSettingsField.WORKLOAD_PROFILE]: { value: null },
[VMSettingsField.PROVISION_SOURCE_TYPE]: { value: isProviderImport ? undefined : null },
[VMSettingsField.HOSTNAME]: { value: null },
[VMSettingsField.CPU]: { value: null },
[VMSettingsField.MEMORY]: { value: null },
};

// filter out oldTemplates
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { FLAGS } from '@console/shared';
import { TemplateValidations } from '../../../../../utils/validations/template/template-validations';
import { isWinToolsImage, getVolumeContainerImage } from '../../../../../selectors/vm';
import {
hasVmSettingsChanged,
Expand All @@ -20,6 +21,7 @@ import { ProvisionSource } from '../../../../../constants/vm/provision-source';
import { getProviders } from '../../../provider-definitions';
import { windowsToolsStorage } from '../../initial-state/storage-tab-initial-state';
import { getStorages } from '../../../selectors/selectors';
import { getTemplateValidations } from '../../validations/utils/templates-validations';
import { prefillVmTemplateUpdater } from './prefill-vm-template-state-update';

export const selectedUserTemplateUpdater = (options: UpdateOptions) => {
Expand Down Expand Up @@ -144,6 +146,25 @@ export const nativeK8sUpdater = ({ id, dispatch, getState, changedCommonData }:
);
};

export const templateValidationsUpdater = (options: UpdateOptions) => {
const { id, prevState, dispatch, getState } = options;
const state = getState();
if (
!hasVmSettingsChanged(
prevState,
state,
id,
VMSettingsField.OPERATING_SYSTEM,
VMSettingsField.FLAVOR,
VMSettingsField.WORKLOAD_PROFILE,
)
) {
return;
}
const validations: TemplateValidations[] = getTemplateValidations(options);
Copy link
Member

Choose a reason for hiding this comment

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

not needing const state = getState(); in this function is strange ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

I do not think it is needed to store validations in the redux.

There are problems with this approach:

  • redux should not store duplicate data
  • this function is missing all the dependencies for this to work correctly
  • this might be another place for performance issues in the future

Copy link
Contributor Author

@irosenzw irosenzw Jan 19, 2020

Choose a reason for hiding this comment

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

@suomiy , I don't understand this comment at all. Can you elaborate ?
why the validations consider duplicate data ?
what are the dependencies I missed?
what performance issues you are referring to? can you give an example ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@suomiy, how do you suggest to get the validations to the disk-modal without using Redux store ?

Copy link
Member

Choose a reason for hiding this comment

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

@suomiy , I don't understand this comment at all. Can you elaborate ?
why the validations consider duplicate data ?

because they are already stored in the template

what are the dependencies I missed?
what performance issues you are referring to? can you give an example ?

usertemplate and templates and that is why it could be a performance issue (we could still use it if we decide to, but it is better to be careful)

Copy link
Member

Choose a reason for hiding this comment

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

@suomiy, how do you suggest to get the validations to the disk-modal without using Redux store ?

from the templates

dispatch(vmWizardInternalActions[InternalActionType.SetTemplateValidations](id, validations));
};

export const updateVmSettingsState = (options: UpdateOptions) =>
[
...(iGetCommonData(options.getState(), options.id, VMWizardProps.isProviderImport)
Expand All @@ -154,6 +175,7 @@ export const updateVmSettingsState = (options: UpdateOptions) =>
flavorUpdater,
osUpdater,
nativeK8sUpdater,
templateValidationsUpdater,
].forEach((updater) => {
updater && updater(options);
});
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export enum InternalActionType {
SetNetworks = 'KubevirtVMWizardSetNetworks',
SetStorages = 'KubevirtVMWizardSetStorages',
SetResults = 'KubevirtVMWizardSetResults',
SetTemplateValidations = 'TemplateValidations',
}

export type WizardInternalAction = {
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { UpdateOptions } from '../../types';
import {
iGetVmSettingAttribute,
iGetVmSettingValue,
} from '../../../selectors/immutable/vm-settings';
import { iGetLoadedCommonData } from '../../../selectors/immutable/selectors';
import { VMSettingsField, VMWizardProps } from '../../../types';
import { iGetTemplateValidations } from '../../../../../selectors/immutable/template/selectors';
import { iGetRelevantTemplates } from '../../../../../selectors/immutable/template/combined';
import { TemplateValidations } from '../../../../../utils/validations/template/template-validations';

const getValidationsFromTemplates = (templates): TemplateValidations[] =>
templates.map(
(relevantTemplate) => new TemplateValidations(iGetTemplateValidations(relevantTemplate)),
);

export const getTemplateValidations = (options: UpdateOptions): TemplateValidations[] => {
const { getState, id } = options;
const state = getState();

const userTemplateName = iGetVmSettingValue(state, id, VMSettingsField.USER_TEMPLATE);
const os = iGetVmSettingAttribute(state, id, VMSettingsField.OPERATING_SYSTEM);
const flavor = iGetVmSettingAttribute(state, id, VMSettingsField.FLAVOR);
const workload = iGetVmSettingAttribute(state, id, VMSettingsField.WORKLOAD_PROFILE);

const iUserTemplates = iGetLoadedCommonData(state, id, VMWizardProps.userTemplates);
const iCommonTemplates = iGetLoadedCommonData(state, id, VMWizardProps.commonTemplates);

const templates = iGetRelevantTemplates(iUserTemplates, iCommonTemplates, {
userTemplateName,
os,
workload,
flavor,
});

if (templates.size > 0 && os && workload) {
// templates are sorted by relevance if only flavor is missing
return getValidationsFromTemplates(templates.toArray());
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reason why this is necessary. Please update to the newest version

}

return getValidationsFromTemplates([templates.first()]);
};
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,10 @@ import {
import { validatePositiveInteger } from '../../../../utils/validations/common';
import { pluralize } from '../../../../utils/strings';
import { vmSettingsOrder } from '../initial-state/vm-settings-tab-initial-state';
import { TemplateValidations } from '../../../../utils/validations/template/template-validations';
import { combineIntegerValidationResults } from '../../../../utils/validations/template/utils';
import { getValidationUpdate } from './utils';
import {
getTemplateValidations,
runValidation,
} from './common-template-validations/common-templates-validations';
import { getTemplateValidations } from './utils/templates-validations';

const validateVm: VmSettingsValidator = (field, options) => {
const { getState, id } = options;
Expand Down Expand Up @@ -101,32 +100,22 @@ export const validateOperatingSystem: VmSettingsValidator = (field) => {

const memoryValidation: VmSettingsValidator = (field, options): ValidationObject => {
const memValueGB = iGetFieldValue(field);
if (!memValueGB) {
if (memValueGB == null || memValueGB === '') {
return null;
}
const memValueBytes = memValueGB * 1024 ** 3;
const validations = getTemplateValidations(options, VMSettingsField.MEMORY);
const validations = getTemplateValidations(options);
if (validations.length === 0) {
return null;
validations.push(new TemplateValidations()); // add empty validation for positive integer if it is missing one
}

const validationResult = runValidation(validations, memValueBytes);

if (!validationResult.isValid) {
// Must have failed all validations, including first one:
const validation = validations[0];
let customMessage = validationResult.errorMsg;

if ('min' in validation && 'max' in validation) {
customMessage = `Memory must be between ${validation.min / 1024 ** 3}GB and ${validation.max /
1024 ** 3} GB`;
} else if ('min' in validation) {
customMessage = `Memory must be above ${validation.min / 1024 ** 3}GB`;
} else if ('max' in validation) {
customMessage = `Memory must be below ${validation.max / 1024 ** 3}GB`;
}
const validationResults = validations
.map((v) => v.validateMemory(memValueBytes))
.filter(({ isValid }) => !isValid);

return asValidationObject(customMessage, ValidationErrorType.Error);
if (validationResults.length === validations.length) {
// every template failed its validations - we cannot choose one
return combineIntegerValidationResults(validationResults, 0);
}

return null;
Expand Down Expand Up @@ -177,6 +166,7 @@ const validationConfig: VMSettingsValidationConfig = {
VMSettingsField.OPERATING_SYSTEM,
VMSettingsField.WORKLOAD_PROFILE,
],
detectCommonDataChanges: [VMWizardProps.userTemplates, VMWizardProps.commonTemplates],
validator: memoryValidation,
},
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { get } from 'lodash';
import { Map } from 'immutable';
import { TemplateValidations } from 'packages/kubevirt-plugin/src/utils/validations/template/template-validations';
import { iGetIn, immutableListToShallowJS } from '../../../utils/immutable';
import {
VMWizardNetwork,
Expand Down Expand Up @@ -50,3 +51,6 @@ export const getStoragesWithWrappers = (state, id: string): VMWizardStorageWithW
persistentVolumeClaim,
...rest,
}));

export const getTemplateValidations = (state, id: string): TemplateValidations[] =>
iGetIn(getCreateVMWizards(state), [id, 'commonData', 'dataIDReferences', 'templateValidations']);
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export const titleResolver: VMSettingsRenderableFieldResolver = {
[VMSettingsField.IMAGE_URL]: 'URL',
[VMSettingsField.OPERATING_SYSTEM]: 'Operating System',
[VMSettingsField.FLAVOR]: 'Flavor',
[VMSettingsField.MEMORY]: 'Memory (GB)',
[VMSettingsField.MEMORY]: 'Memory (GiB)',
[VMSettingsField.CPU]: 'CPUs',
[VMSettingsField.WORKLOAD_PROFILE]: 'Workload Profile',
[VMSettingsField.START_VM]: 'Start virtual machine on creation',
Expand Down
Loading