Bug 1840958: Source can be empty in general step #6270

yaacov · 2020-08-09T12:27:02Z

URL and Container source are not required in the general step, they are part of the storage step.

Screenshots:
Edit storage "rootdisk" URL source:

Edit storage "rootdisk" Container source:

Edit storage "rootdisk" "any" source:

Edit network "any" net interface

openshift-ci-robot · 2020-08-09T12:27:09Z

@yaacov: This pull request references Bugzilla bug 1840958, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1840958: Source can be empty in general step

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-08-09T12:30:33Z

@yaacov: This pull request references Bugzilla bug 1840958, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1840958: Source can be empty in general step

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yaacov · 2020-08-09T12:31:00Z

@matthewcarleton please review

irosenzw · 2020-08-09T13:05:14Z

...ges/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.tsx

+            ProvisionSource.URL.toString(),
+            ProvisionSource.CONTAINER.toString(),
+            ProvisionSource.DISK.toString(),
+          ].includes(provisionSourceValue) && getProvisionSourceDiskHelpMsg()}


can getProvisionSourceDiskHelpMsg be a component rather than a function ?

+1, changed to consts

irosenzw · 2020-08-09T13:06:14Z

...ges/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.tsx

+          onClick={() => goToStep(VMWizardTab.STORAGE)}
+          variant={ButtonVariant.link}
+        >
+          <strong>storage</strong>


please capitalize

irosenzw · 2020-08-09T13:06:40Z

...ges/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.tsx

+          onClick={() => goToStep(VMWizardTab.NETWORKING)}
+          variant={ButtonVariant.link}
+        >
+          <strong>networking</strong>


also capitalize here

irosenzw · 2020-08-09T13:39:26Z

/lgtm

openshift-ci-robot · 2020-08-09T13:39:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~frontend/packages/kubevirt-plugin/OWNERS~~ [yaacov]

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

yaacov · 2020-08-09T15:01:51Z

/retest

yaacov · 2020-08-09T18:34:28Z

/retest

openshift-ci-robot · 2020-08-09T20:23:02Z

@yaacov: All pull requests linked via external trackers have merged: openshift/console#6270. Bugzilla bug 1840958 has been moved to the MODIFIED state.

In response to this:

Bug 1840958: Source can be empty in general step

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

atiratree

@yaacov PTAL

atiratree · 2020-08-10T13:02:14Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

@@ -110,11 +110,9 @@ const provisioningSourceUpdater = ({ id, prevState, dispatch, getState }: Update
  dispatch(
    vmWizardInternalActions[InternalActionType.UpdateVmSettings](id, {
      [VMSettingsField.CONTAINER_IMAGE]: {


the fields were kept required so it would be apparent from redux which fields have to be filled.

for this special use case skipValidation was introduced into vm-settings-tab-initial-state.

Please revert this change or give a good reason why it shouldn't be visible in redux and skipValidation should then be removed from initial-state?

the fields were kept required so it would be apparent from redux which fields have to be filled.

this fields are are not required in the user view, a user can leave them empty and continue filling up the form.
the redux viewer should reflect that.

or give a good reason why it shouldn't be visible in redux and skipValidation should then be removed from initial-state?

This fields are optional, users can fill this information later in the storage tab, we should remove the skipValidation, I can make a folow up PR to fix that.

+1 let's remove the skipValidation then

atiratree · 2020-08-10T13:03:52Z

...ges/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.tsx

@@ -53,6 +62,52 @@ export class VMSettingsTabComponent extends React.Component<VMSettingsTabCompone
      goToStep,
    } = this.props;

+    const provisionSourceValue = this.getFieldValue(VMSettingsField.PROVISION_SOURCE_TYPE);


there are many fields in General Tab and we try to keep this component short as possible. Please either make a new component for this message or make a new component for provision source which will include this functionality.

makes sense to me too 👍

I can make a follow up PR to move the "source" input into a separate component.

openshift-ci-robot requested review from jelkosz and rhrazdil August 9, 2020 12:27

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2020

yaacov force-pushed the source-can-be-empty-in-general-step branch from 9d55028 to a96b6dc Compare August 9, 2020 12:29

source can be empty in general step

c702494

yaacov force-pushed the source-can-be-empty-in-general-step branch from a96b6dc to c702494 Compare August 9, 2020 12:51

irosenzw reviewed Aug 9, 2020

View reviewed changes

capitalize

8910a55

yaacov force-pushed the source-can-be-empty-in-general-step branch from 72af112 to 8910a55 Compare August 9, 2020 13:20

openshift-ci-robot assigned irosenzw Aug 9, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2020

openshift-merge-robot merged commit 20bdbe2 into openshift:master Aug 9, 2020

atiratree reviewed Aug 10, 2020

View reviewed changes

This was referenced Aug 10, 2020

Bug 1840958: Dont skip validation of url and container sources #6277

Merged

Bug 1840958: Move provision source to component #6278

Merged

spadgett added this to the v4.6 milestone Aug 11, 2020

Bug 1840958: Source can be empty in general step #6270

Bug 1840958: Source can be empty in general step #6270

yaacov commented Aug 9, 2020 •

edited

openshift-ci-robot commented Aug 9, 2020

openshift-ci-robot commented Aug 9, 2020

yaacov commented Aug 9, 2020

irosenzw Aug 9, 2020

yaacov Aug 9, 2020

irosenzw Aug 9, 2020

yaacov Aug 9, 2020

irosenzw Aug 9, 2020

yaacov Aug 9, 2020

irosenzw commented Aug 9, 2020

openshift-ci-robot commented Aug 9, 2020

yaacov commented Aug 9, 2020

yaacov commented Aug 9, 2020

openshift-ci-robot commented Aug 9, 2020

atiratree left a comment

atiratree Aug 10, 2020

yaacov Aug 10, 2020

atiratree Aug 10, 2020

atiratree Aug 10, 2020

yaacov Aug 10, 2020

atiratree Aug 10, 2020

Bug 1840958: Source can be empty in general step #6270

Bug 1840958: Source can be empty in general step #6270

Conversation

yaacov commented Aug 9, 2020 • edited

openshift-ci-robot commented Aug 9, 2020

openshift-ci-robot commented Aug 9, 2020

yaacov commented Aug 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irosenzw commented Aug 9, 2020

openshift-ci-robot commented Aug 9, 2020

yaacov commented Aug 9, 2020

yaacov commented Aug 9, 2020

openshift-ci-robot commented Aug 9, 2020

atiratree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Aug 9, 2020 •

edited