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

refactor v2v, basicSettings and CreateVmDialog + fix bugs #417

Merged
merged 1 commit into from
Apr 27, 2019

Conversation

atiratree
Copy link
Contributor

  • renamed basiceSettings -> vmSettings
  • updated enhancedK8sMethods

- renamed basiceSettings -> vmSettings
- updated enhancedK8sMethods
@atiratree atiratree force-pushed the v2v.refactor-basic branch from 6804d8d to f378eee Compare April 27, 2019 22:58
@coveralls
Copy link

coveralls commented Apr 27, 2019

Pull Request Test Coverage Report for Build 1695

  • 631 of 847 (74.5%) changed or added relevant lines in 55 files are covered.
  • 15 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-3.4%) to 82.793%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/Wizard/CreateVmWizard/providers/VMwareImportProvider/requestedResources.js 6 7 85.71%
src/k8s/objects/v2v/vmware/vmWareSecret.js 5 6 83.33%
src/k8s/requests/v2v/importVmware.js 22 23 95.65%
src/k8s/requests/v2v/utils.js 9 10 90.0%
src/k8s/util/enhancedK8sMethods.js 12 13 92.31%
src/selectors/deployment/selectors.js 1 2 50.0%
src/components/Wizard/CreateVmWizard/utils/vmSettingsTabUtils.js 19 21 90.48%
src/k8s/request.js 12 14 85.71%
src/k8s/util/errors.js 0 2 0.0%
src/selectors/v2v/selectors.js 3 5 60.0%
Files with Coverage Reduction New Missed Lines %
src/selectors/vm/selectors.js 3 77.11%
src/tests/k8s.js 12 53.97%
Totals Coverage Status
Change from base Build 1446: -3.4%
Covered Lines: 3767
Relevant Lines: 4343

💛 - Coveralls

@atiratree
Copy link
Contributor Author

  • tests and additional fixtures will be added in followups

There are many data flows and side effects in vm dialog. Other tabs/extensions(v2v) need to change data as a whole of all dialog. I resolved this by adding custom reducers which react on state updates.

I guess you know what should be the next step :)
Dialog is getting too complicated and this solution is not perfect. Nevertheless I think this implementation should be easily converted to use with single state/store + actions, i.e. by any combination of immutables/redux/saga.

Let's discuss what is the best approach how to continue. @mareklibra @rawagner @vojtechszocs

Currently approach:

  • allows any tab/extension to register isDisabled/isRequired/isHidden metadata on vmSettings (renamed from basicSettings).
  • updates are chained in useful order through reducers where each reducer changes the state only if interesting changes occur on the data the reducer is working with.
  • each tab can declare initial state + validators which are called after the reducers do their work

I tried to separate requests from the ui code but it still needs some work (the best solution would be probably saga)

@atiratree
Copy link
Contributor Author

merging; with future followups in mind

@atiratree atiratree merged commit 32fdb82 into kubevirt:master Apr 27, 2019
@atiratree atiratree mentioned this pull request Apr 27, 2019
Copy link
Contributor

@mareklibra mareklibra left a comment

Choose a reason for hiding this comment

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

This patch is too huge for review. Especially after its merge.

It contains non-v2v changes (refactoring).
The only option we have now is to test it and react on bugs afterwards.

Next time, I would prefer to postpone such huge/unnecessary change after creation of stable branch in project phase like we have these days.

import { VALIDATION_INFO_TYPE } from '../../constants';
import { prefixedId } from '../../utils';

export const FormControlLabel = ({ isRequired, title, help }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow-up: can be placed to a separate source file

import { Wizard } from 'patternfly-react';

import { BasicSettingsTab, onCloseBasic } from './BasicSettingsTab';
import { VmSettingsTab } from './VmSettingsTab';
Copy link
Contributor

Choose a reason for hiding this comment

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

Too late for following comment as this PR is merged already, but let me just mention it - I am not fan of this renaming as all the settings tabs are bout a VM. The "Basic" denotes context better. And if it eventually contains non-basic options, it can be splitted to a new specialized tab (not recent case, IMO)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it because it is mouthful. This way, is easier to navigate in the code. We can discuss this and come up with other name - preferably short

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants