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

Create VM Wizard: conditionally rendered Import provision source #519

Merged
merged 1 commit into from
Jul 19, 2019

Conversation

mareklibra
Copy link
Contributor

The V2V Import VMware option is supposed to be rendered just in case
that related infrastructure is deployed on the cluster.

In other words, check for presence of V2VVMware CRD is introduced.

As we have just a single provider (VMware) so far, the higher-level
"Import"-provision type is not rendered if the CRD is missing.

@coveralls
Copy link

coveralls commented Jul 18, 2019

Pull Request Test Coverage Report for Build 2160

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 82.482%

Files with Coverage Reduction New Missed Lines %
src/components/Wizard/CreateVmWizard/initialState/vmSettingsTabInitialState.js 1 91.67%
Totals Coverage Status
Change from base Build 2156: -0.02%
Covered Lines: 4006
Relevant Lines: 4650

💛 - Coveralls

Copy link
Contributor

@atiratree atiratree left a comment

Choose a reason for hiding this comment

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

This approach is okay for now due to having only one import provider, but it should be changed in the future if other providers get included.

@@ -49,7 +49,7 @@ export const getVmSettingsInitialState = props => ({
});

export const getInitialVmSettings = (props = { createTemplate: false }) => {
Copy link
Contributor

@atiratree atiratree Jul 18, 2019

Choose a reason for hiding this comment

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

please add the isV2vVmwareCrd with false as a default. This could break things in the future if somebody for example chose to use it for asHidden which takes distinction of undefined and false into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but the default false is recently handled via CreateVmWizard default prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, just to be sure to have a better contract

@mareklibra
Copy link
Contributor Author

This approach is okay for now due to having only one import provider, but it should be changed in the future if other providers get included.

That's the plan. But so far the potentially empty list of providers would be confusing. In addition, this is meant as a workaround for potential project planing or cluster mis-configuration issues.

The V2V Import VMware option is supposed to be rendered just in case
that related infrastructure is deployed on the cluster.

In other words, check for presence of V2VVMware CRD is introduced.

As we have just a single provider (VMware) so far, the higher-level
"Import"-provision type is not rendered if the CRD is missing.
@atiratree atiratree merged commit ded7e83 into kubevirt:master Jul 19, 2019
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.

4 participants