Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Fix rendered disk size <1Gi #452

Merged
merged 5 commits into from
May 16, 2019
Merged

Fix rendered disk size <1Gi #452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c5dc94
Fix rendered disk size <1Gi
mareklibra May 15, 2019
bfb5eac
Wrap "units" to a "context" object in importVmwareVm()
mareklibra May 15, 2019
860deaa
Introduce renderValueConfig()
mareklibra May 16, 2019
193a446
Rename `renderConfig()` to `renderEditConfig()`
mareklibra May 16, 2019
1028c82
Keep using Gi for non-imported disks
mareklibra May 16, 2019
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
42 changes: 26 additions & 16 deletions src/components/Table/EditableDraggableTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,15 +168,15 @@ class EditableDraggableTable extends React.Component {
};

getActionButtons = (additionalData, isEditing) => {
const renderConfig = this.getRenderConfig(additionalData);
const renderEditConfig = this.getRenderEditConfig(additionalData);
let result;
if (renderConfig) {
const id = prefixedId(renderConfig.id, additionalData.rowKey);
if (renderEditConfig) {
const id = prefixedId(renderEditConfig.id, additionalData.rowKey);

result =
isEditing && !renderConfig.visibleOnEdit ? null : (
isEditing && !renderEditConfig.visibleOnEdit ? null : (
<DropdownKebab className="kubevirt-editable-table__row-actions" id={id} key={id} pullRight>
{renderConfig.actions.map((action, idx) =>
{renderEditConfig.actions.map((action, idx) =>
this.getActionButton(action, additionalData, prefixedId(idx, id))
)}
</DropdownKebab>
Expand All @@ -186,25 +186,35 @@ class EditableDraggableTable extends React.Component {
return <td className="editable">{result}</td>;
};

isDropdown = additionalData => get(this.getRenderConfig(additionalData), 'type') === DROPDOWN;
isDropdown = additionalData => get(this.getRenderEditConfig(additionalData), 'type') === DROPDOWN;

getRenderConfig = additionalData => {
const { renderConfig } = additionalData.column;
getRenderEditConfig = additionalData => {
const { renderEditConfig } = additionalData.column;
return renderEditConfig ? renderEditConfig(additionalData.rowData) : null;
};

return renderConfig ? renderConfig(additionalData.rowData) : null;
getRenderValueConfig = additionalData => {
const { renderValueConfig } = additionalData.column;
return renderValueConfig ? renderValueConfig(additionalData.rowData) : null;
};

resolveRenderedValue = (value, additionalData, editable) => {
const renderConfig = this.getRenderConfig(additionalData);
const renderEditConfig = this.getRenderEditConfig(additionalData);
const renderValueConfig = this.getRenderValueConfig(additionalData);

let result;
if (this.isDropdown(additionalData)) {
result = get(renderConfig.choices.find(clazz => clazz.id === value), 'name');
result = get(renderEditConfig.choices.find(clazz => clazz.id === value), 'name');
}

if (!result) {
result = value;
}

if (!editable && renderValueConfig && renderValueConfig.formatter) {
result = renderValueConfig.formatter(result);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather implement this with addendum approach or combine these approaches altogether (addendum with functional approach). So we don't have two approaches for the similar thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it's very similar, the formatter can just do more things than just appending a string.

For flexibility, I am more for the functional approach but refactoring of the addendum is not feasible for the 2.0.0 scope. Can be done later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If implemented using static addendum, the logic would need to be moved to a higher level while affecting other flows and so potentially introducing new bugs. So I decided to go this safer way.

One more idea: we can keep using addendum as it is and optionally assigning a function to it (instead of recent string). The EdditableDraggableTable would check it's type and act accordingly. But such reusing of single property for two different types introduces more confusion, imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, we can leave this for later. Can you please create an issue to unify these two approaches?

Yes better to rewrite it without any magic and use just the functional approach.

if (!editable && additionalData.column.hasAddendum) {
const { addendum } = additionalData.rowData;
if (addendum) {
Expand All @@ -217,7 +227,7 @@ class EditableDraggableTable extends React.Component {

inlineEditFormatter = inlineEditFormatterFactory({
isEditing: additionalData =>
this.inlineEditController.isEditing(additionalData) && this.getRenderConfig(additionalData),
this.inlineEditController.isEditing(additionalData) && this.getRenderEditConfig(additionalData),

renderValue: (value, additionalData) => {
const data = this.resolveRenderedValue(value, additionalData, false);
Expand Down Expand Up @@ -253,17 +263,17 @@ class EditableDraggableTable extends React.Component {
},

renderEdit: (value, additionalData) => {
const renderConfig = this.getRenderConfig(additionalData);
const renderEditConfig = this.getRenderEditConfig(additionalData);
const isDropdown = this.isDropdown(additionalData);

const onChange = v => this.inlineEditController.onChange(v, additionalData);
return (
<td className="editable editing kubevirt-editable-table__cell">
{getFormElement({
...renderConfig,
id: prefixedId(renderConfig.id, additionalData.rowKey),
...renderEditConfig,
id: prefixedId(renderEditConfig.id, additionalData.rowKey),
value: this.resolveRenderedValue(value, additionalData, true),
defaultValue: this.resolveRenderedValue(value, additionalData, true) || renderConfig.initialValue,
defaultValue: this.resolveRenderedValue(value, additionalData, true) || renderEditConfig.initialValue,
onChange: isDropdown ? onChange : null, // onChange for dropdowns
onBlur: isDropdown ? null : onChange, // onBlur for text
})}
Expand Down
3 changes: 2 additions & 1 deletion src/components/Wizard/CreateVmWizard/CreateVmWizard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ export class CreateVmWizard extends React.Component {
vmSettings,
this.state.stepData[NETWORKS_TAB_KEY].value,
this.state.stepData[STORAGE_TAB_KEY].value,
this.props.persistentVolumeClaims
this.props.persistentVolumeClaims,
this.props.units
)
.then(() => getResults(enhancedK8sMethods))
.catch(error => cleanupAndGetResults(enhancedK8sMethods, error))
Expand Down
10 changes: 5 additions & 5 deletions src/components/Wizard/CreateVmWizard/NetworksTab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ export class NetworksTab extends React.Component {
},
},
property: 'name',
renderConfig: () => ({
renderEditConfig: () => ({
id: 'name-edit',
type: TEXT,
}),
Expand All @@ -255,7 +255,7 @@ export class NetworksTab extends React.Component {
},
},
property: 'mac',
renderConfig: row =>
renderEditConfig: row =>
row.networkType === NETWORK_TYPE_POD
? null
: {
Expand All @@ -273,7 +273,7 @@ export class NetworksTab extends React.Component {
},
},
property: 'network',
renderConfig: () => ({
renderEditConfig: () => ({
id: 'network-edit',
type: DROPDOWN,
choices: this.props.networkConfigs
Expand All @@ -294,7 +294,7 @@ export class NetworksTab extends React.Component {
},
},
property: 'binding',
renderConfig: nic => ({
renderEditConfig: nic => ({
id: 'binding-edit',
type: DROPDOWN,
choices: getNetworkBindings(nic.networkType),
Expand All @@ -313,7 +313,7 @@ export class NetworksTab extends React.Component {
},
},
type: ACTIONS_TYPE,
renderConfig: () => ({
renderEditConfig: () => ({
id: 'actions',
actions: [
{
Expand Down
24 changes: 20 additions & 4 deletions src/components/Wizard/CreateVmWizard/StorageTab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ import {

import { canBeBootable, needsBootableDisk } from './utils/storageTabUtils';

import { getValidK8SSize } from '../../../utils';

const initalStorageErrorsArray = () => Array(4).fill(null);

const genericValidator = (storage, validationResolvers) =>
Expand Down Expand Up @@ -390,7 +392,7 @@ export class StorageTab extends React.Component {
},
},
property: 'name',
renderConfig: storage => {
renderEditConfig: storage => {
if (storage.storageType === STORAGE_TYPE_PVC) {
return {
id: 'name-attach-edit',
Expand Down Expand Up @@ -421,13 +423,27 @@ export class StorageTab extends React.Component {
},
},
property: 'size',
renderConfig: storage =>
renderEditConfig: storage =>
storage.storageType === STORAGE_TYPE_DATAVOLUME
? {
id: 'size-edit',
type: POSITIVE_NUMBER,
}
: null,
renderValueConfig: storage => {
if ([STORAGE_TYPE_EXTERNAL_IMPORT].includes(storage.storageType)) {
return {
formatter: value => getValidK8SSize(value, this.props.units, 'Gi').string,
};
}
if ([STORAGE_TYPE_DATAVOLUME, STORAGE_TYPE_PVC, STORAGE_TYPE_EXTERNAL_V2V_TEMP].includes(storage.storageType)) {
// do not convert, keep Gi
return {
formatter: value => `${value} Gi`,
};
}
return null;
},
},
{
header: {
Expand All @@ -440,7 +456,7 @@ export class StorageTab extends React.Component {
},
property: 'storageClass',

renderConfig: storage =>
renderEditConfig: storage =>
[STORAGE_TYPE_DATAVOLUME, STORAGE_TYPE_EXTERNAL_IMPORT, STORAGE_TYPE_EXTERNAL_V2V_TEMP].includes(
storage.storageType
)
Expand All @@ -461,7 +477,7 @@ export class StorageTab extends React.Component {
},
},
type: ACTIONS_TYPE,
renderConfig: storage =>
renderEditConfig: storage =>
storage.rootStorage ||
[STORAGE_TYPE_EXTERNAL_IMPORT, STORAGE_TYPE_EXTERNAL_V2V_TEMP, STORAGE_TYPE_EXTERNAL_V2V_VDDK].includes(
storage.storageType
Expand Down
14 changes: 13 additions & 1 deletion src/components/Wizard/CreateVmWizard/fixtures/CreateVmWizard.fixture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,19 @@ export const storageClasses = [
},
];

export const units = { dehumanize: val => ({ value: val ? val.match(/^[0-9]+/)[0] * 1073741824 : 0 }) };
export const units = {
dehumanize: val => ({ value: val ? val.match(/^[0-9]+/)[0] * 1073741824 : 0 }),
humanize: () => {
// mock implementation
const unit = 'Gi';
const value = 1234;
return {
string: `${value} ${unit}`,
value,
unit,
};
},
};

export default [
{
Expand Down
6 changes: 3 additions & 3 deletions src/components/Wizard/CreateVmWizard/fixtures/StorageTab.fixture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ export const rows = [
name: 'D',
size: '15',
storageClass: 'iscsi',
renderConfig: 0,
renderEditConfig: 0,
storageType: STORAGE_TYPE_DATAVOLUME,
},
{
id: 2,
isBootable: false,
renderConfig: 1,
renderEditConfig: 1,
name: 'disk Two',
size: '15',
storageClass: 'glusterfs',
Expand All @@ -26,7 +26,7 @@ export const rows = [
{
id: 3,
isBootable: false,
renderConfig: 1,
renderEditConfig: 1,
storageType: STORAGE_TYPE_PVC,
},
];
Expand Down
2 changes: 1 addition & 1 deletion src/components/Wizard/CreateVmWizard/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export const ERROR_NO_STORAGE_CLASS_SELECTED = 'Storage Class not selected';
export const ERROR_NO_STORAGE_SELECTED = 'No storage is selected';
export const ERROR_STORAGE_NOT_VALID = 'Selected storage is not valid';
export const ERROR_DISK_NOT_FOUND = 'Disk configuration not found';
export const HEADER_SIZE = 'Size (Gi)';
export const HEADER_SIZE = 'Size';
export const HEADER_STORAGE_CLASS = 'Storage Class';
export const REMOVE_DISK_BUTTON = 'Remove Disk';
export const ATTACH_DISK_BUTTON = 'Attach Disk';
Expand Down
6 changes: 3 additions & 3 deletions src/components/Wizard/CreateVmWizard/tests/NetworksTab.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const pxeRow = {
name: 'pxe-net',
mac: '',
network: NetworksTabFixture.props.networkConfigs[0].metadata.name,
renderConfig: 0,
renderEditConfig: 0,
edit: false,
editable: true,
networkType: NETWORK_TYPE_MULTUS,
Expand All @@ -51,7 +51,7 @@ const podRow = {
name: 'pod-network',
mac: '',
network: POD_NETWORK,
renderConfig: 0,
renderEditConfig: 0,
edit: false,
editable: true,
networkType: NETWORK_TYPE_POD,
Expand Down Expand Up @@ -326,7 +326,7 @@ describe('<NetworksTab />', () => {
name: 'pxe-net2',
mac: '',
network: networkConfig2.metadata.name,
renderConfig: 0,
renderEditConfig: 0,
edit: false,
editable: true,
networkType: NETWORK_TYPE_MULTUS,
Expand Down
10 changes: 5 additions & 5 deletions src/components/Wizard/CreateVmWizard/tests/__snapshots__/NetworksTab.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ exports[`<NetworksTab /> renders correctly 1`] = `
},
},
"property": "name",
"renderConfig": [Function],
"renderEditConfig": [Function],
},
Object {
"header": Object {
Expand All @@ -38,7 +38,7 @@ exports[`<NetworksTab /> renders correctly 1`] = `
},
},
"property": "mac",
"renderConfig": [Function],
"renderEditConfig": [Function],
},
Object {
"header": Object {
Expand All @@ -50,7 +50,7 @@ exports[`<NetworksTab /> renders correctly 1`] = `
},
},
"property": "network",
"renderConfig": [Function],
"renderEditConfig": [Function],
},
Object {
"header": Object {
Expand All @@ -62,7 +62,7 @@ exports[`<NetworksTab /> renders correctly 1`] = `
},
},
"property": "binding",
"renderConfig": [Function],
"renderEditConfig": [Function],
},
Object {
"header": Object {
Expand All @@ -72,7 +72,7 @@ exports[`<NetworksTab /> renders correctly 1`] = `
},
},
},
"renderConfig": [Function],
"renderEditConfig": [Function],
"type": "actions",
},
]
Expand Down
11 changes: 6 additions & 5 deletions src/components/Wizard/CreateVmWizard/tests/__snapshots__/StorageTab.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,20 @@ exports[`<StorageTab /> renders correctly 1`] = `
},
},
"property": "name",
"renderConfig": [Function],
"renderEditConfig": [Function],
},
Object {
"header": Object {
"label": "Size (Gi)",
"label": "Size",
"props": Object {
"style": Object {
"width": "23%",
},
},
},
"property": "size",
"renderConfig": [Function],
"renderEditConfig": [Function],
"renderValueConfig": [Function],
},
Object {
"header": Object {
Expand All @@ -57,7 +58,7 @@ exports[`<StorageTab /> renders correctly 1`] = `
},
},
"property": "storageClass",
"renderConfig": [Function],
"renderEditConfig": [Function],
},
Object {
"header": Object {
Expand All @@ -67,7 +68,7 @@ exports[`<StorageTab /> renders correctly 1`] = `
},
},
},
"renderConfig": [Function],
"renderEditConfig": [Function],
"type": "actions",
},
]
Expand Down
4 changes: 2 additions & 2 deletions src/k8s/objects/pvc/pvc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { PersistentVolumeClaimModel } from '../../../models/index';

export const buildPvc = ({ generateName, namespace, storageType, size, storageClass }) => ({
export const buildPvc = ({ generateName, namespace, size, unit, storageClass }) => ({
apiVersion: PersistentVolumeClaimModel.apiVersion,
kind: PersistentVolumeClaimModel.kind,
metadata: {
Expand All @@ -12,7 +12,7 @@ export const buildPvc = ({ generateName, namespace, storageType, size, storageCl
volumeMode: 'Filesystem',
resources: {
requests: {
storage: `${size}Gi`,
storage: `${size}${unit}`,
},
},
storageClassName: storageClass,
Expand Down
Loading