PVC clone workflow #3927

gnehapk · 2020-01-13T09:01:27Z

Will add the feature flag(ceph) with my extension point in a separate PR, once #3383 gets merged as this PR is adding the support for adding feature flag to extension.

gnehapk · 2020-01-14T07:24:41Z

@rawagner @bipuladh @afreen23 @a2batic Please review.

...end/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/_clone-pvc-modal.scss

bipuladh · 2020-01-14T07:24:42Z

...end/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/_clone-pvc-modal.scss

standard spacings?? Maybe one from patternfly global css?

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

bipuladh · 2020-01-14T07:26:38Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Is there a reason why you are input Form from patternfly?

I didn't get you. Can you explain a bit?

Why you are not using the Form component from PF?

bipuladh · 2020-01-14T07:27:10Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

not required. handlePromiseProps should have this

bipuladh · 2020-01-14T07:28:54Z

frontend/packages/ceph-storage-plugin/src/plugin.ts

use PersistentVolumeClaimModel.kind

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

bipuladh · 2020-01-14T07:29:38Z

frontend/packages/ceph-storage-plugin/src/plugin.ts

Suggested change

.then((m) => m.clonePVCModal(clusterObject))

.then((m) => m.default(clusterObject))

frontend/public/components/persistent-volume-claim.jsx

...end/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/_clone-pvc-modal.scss

frontend/public/components/persistent-volume-claim.jsx

bipuladh · 2020-01-14T10:27:09Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Suggested change

prometheusResults,

prometheusResults,

errorMessage,

inProgress

bipuladh · 2020-01-14T10:27:23Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Suggested change

setProgress(false);

handlePromise does it internally

bipuladh · 2020-01-14T10:27:37Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Remove these two.

Its used in the ModalSubmitFooter component.

handlePromiseProps gives your these values. You don't need to handle them.

Yes, thats what this HOC does, it handles all for you.

bipuladh · 2020-01-14T10:29:48Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Suggested change

setProgress(true);

not required

bipuladh · 2020-01-14T10:30:15Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

isRequired should be enough

bipuladh · 2020-01-14T10:32:03Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Suggested change

kind?: any;

kind?: any;

why any? Isn't kind a string?

bipuladh · 2020-01-14T10:33:21Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Perhaps use ModalComponentProps??

frontend/public/components/persistent-volume-claim.jsx

bipuladh · 2020-01-14T11:59:57Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Suggested change

export const ClonePVCModal = withHandlePromise((props: ClonePVCModalProps) => {

const ClonePVCModal = withHandlePromise((props: ClonePVCModalProps) => {

gnehapk · 2020-01-15T08:18:24Z

/assign @spadgett

rawagner · 2020-01-15T11:18:39Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

withDashboardResources should not be used for non-dashboard components

...end/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/_clone-pvc-modal.scss

afreen23 · 2020-01-15T10:27:05Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Nit: Either freeze the object or capitalize the name, to distinguish it as a constant map.

afreen23 · 2020-01-15T10:56:25Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Nit: you might forget to use PersistentVolumeClaimModel.kind here.

afreen23 · 2020-01-15T10:57:43Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Yes, thats what this HOC does, it handles all for you.

afreen23 · 2020-01-15T11:00:25Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

What if there is an error ? I think you should handle that otherwise it will be blank.
Also, you might need to add loading state too.

afreen23 · 2020-01-15T11:03:04Z

frontend/packages/ceph-storage-plugin/src/constants/queries.ts

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

gnehapk · 2020-01-15T15:33:42Z

/test analyze

spadgett · 2020-01-15T15:28:08Z

...end/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/_clone-pvc-modal.scss

We shouldn't need the nesting here since we're using BEM

spadgett · 2020-01-15T15:28:35Z

...end/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/_clone-pvc-modal.scss

Why do we need to adjust the font-weight?

these texts(pvc details title) needs to be bold as per UXD. Hence, I have used this.

spadgett · 2020-01-15T15:29:48Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

If resource is never null, _.get isn't needed.

spadgett · 2020-01-15T15:31:19Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Why is as needed? Is the return type wrong in usePrometheusPoll?

spadgett · 2020-01-15T15:32:28Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Consistently use the values from the PersistentVolumeClaimModel if you're going to use it.

Suggested change

apiVersion: 'v1',

apiVersion: PersistentVolumeClaimModel.apiVersion,

spadgett · 2020-01-15T15:33:53Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

You shouldn't need _.get

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

spadgett · 2020-01-15T15:35:38Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

Suggested change

const [buttonDisabled, setButton] = React.useState(false);

const [submitDisabled, setSubmitDisabled] = React.useState(false);

spadgett · 2020-01-15T15:36:28Z

frontend/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/clone-pvc-modal.tsx

This helper just wraps setClonePVCName with no other changes. Just call setClonePVCName directly

spadgett · 2020-01-15T15:38:32Z

frontend/packages/ceph-storage-plugin/src/plugin.ts

We should add an RBAC check

This should be

Suggested change

type: 'OCSKebabActions',

type: 'PersistentVolumeClaim/Action',

to match ClusterServiceVersion/Action (with a similar variable names throughout)

I will address the RBAC feature in a separate PR as I want to get this PR merged soon as this is dependent for snapshot PR.

spadgett · 2020-01-15T15:42:54Z

@gnehapk Can you add a screenshot? Thanks!

frontend/packages/ceph-storage-plugin/src/plugin.ts

a2batic · 2020-01-23T20:34:13Z

/test analyze

gnehapk · 2020-01-23T22:55:38Z

/test analyze

gnehapk · 2020-01-23T22:55:47Z

/test e2e-gcp-console

cloudbehl · 2020-01-24T13:04:12Z

/test analyze
/test e2e-gcp-console

cloudbehl · 2020-01-24T13:05:29Z

@gnehapk @a2batic can you remove the conflicts.

a2batic · 2020-01-24T16:14:39Z

/test analyze

spadgett · 2020-01-24T16:24:26Z

frontend/packages/ceph-storage-plugin/src/utils/clone-workflow.ts

name should not be here since you're creating a new resource with a different name

a2batic · 2020-01-24T16:44:06Z

@spadgett addressed review comments, please review.

a2batic · 2020-01-24T16:57:10Z

/retest

spadgett

/approve
/lgtm

openshift-ci-robot · 2020-01-24T17:33:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, cloudbehl, gnehapk, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

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

openshift-bot · 2020-01-24T18:28:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-24T18:54:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-24T19:59:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2020-01-24T20:48:11Z

/hold
for merge queue fix #4065

spadgett · 2020-01-25T04:25:24Z

/hold cancel
/retest

openshift-bot · 2020-01-25T04:26:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-25T05:44:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from bipuladh and christianvogt January 13, 2020 09:02

gnehapk force-pushed the clone-workflow branch from fb8cda7 to baf07df Compare January 14, 2020 07:23

gnehapk changed the title ~~[WIP] PVC clone workflow~~ PVC clone workflow Jan 14, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2020

bipuladh suggested changes Jan 14, 2020

View reviewed changes

afreen23 suggested changes Jan 14, 2020

View reviewed changes

...end/packages/ceph-storage-plugin/src/components/modals/clone-pvc-modal/_clone-pvc-modal.scss Outdated Show resolved Hide resolved

gnehapk force-pushed the clone-workflow branch 2 times, most recently from 98a3cb6 to c6fb0ce Compare January 14, 2020 10:11

bipuladh suggested changes Jan 14, 2020

View reviewed changes

gnehapk force-pushed the clone-workflow branch from c6fb0ce to f89af96 Compare January 14, 2020 10:52

gnehapk commented Jan 14, 2020

View reviewed changes

frontend/public/components/persistent-volume-claim.jsx Outdated Show resolved Hide resolved

bipuladh reviewed Jan 14, 2020

View reviewed changes

openshift-ci-robot assigned spadgett Jan 15, 2020

rawagner suggested changes Jan 15, 2020

View reviewed changes

afreen23 suggested changes Jan 15, 2020

View reviewed changes

gnehapk force-pushed the clone-workflow branch 2 times, most recently from f2f2fdf to c212bd1 Compare January 15, 2020 14:52

spadgett requested changes Jan 15, 2020

View reviewed changes

afreen23 reviewed Jan 16, 2020

View reviewed changes

frontend/packages/ceph-storage-plugin/src/plugin.ts Outdated Show resolved Hide resolved

gnehapk force-pushed the clone-workflow branch from c212bd1 to 874b3ae Compare January 16, 2020 05:45

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020

a2batic force-pushed the clone-workflow branch from b3bf856 to 42df545 Compare January 24, 2020 15:42

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020

spadgett requested changes Jan 24, 2020

View reviewed changes

PVC clone workflow

8d38c00

a2batic force-pushed the clone-workflow branch from 42df545 to 8d38c00 Compare January 24, 2020 16:43

spadgett approved these changes Jan 24, 2020

View reviewed changes

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

spadgett added this to the v4.4 milestone Jan 24, 2020

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2020

openshift-merge-robot merged commit d3e7b80 into openshift:master Jan 25, 2020

gnehapk mentioned this pull request Apr 15, 2020

OCS installation changes for baremetal infra #5016

Merged

	.then((m) => m.clonePVCModal(clusterObject))
	.then((m) => m.default(clusterObject))

-    prometheusResults,
+    prometheusResults,
+    errorMessage,
+    inProgress

	export const ClonePVCModal = withHandlePromise((props: ClonePVCModalProps) => {
	const ClonePVCModal = withHandlePromise((props: ClonePVCModalProps) => {

	apiVersion: 'v1',
	apiVersion: PersistentVolumeClaimModel.apiVersion,

	const [buttonDisabled, setButton] = React.useState(false);
	const [submitDisabled, setSubmitDisabled] = React.useState(false);

	type: 'OCSKebabActions',
	type: 'PersistentVolumeClaim/Action',

PVC clone workflow #3927

PVC clone workflow #3927

Uh oh!

Conversation

gnehapk commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnehapk commented Jan 14, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bipuladh Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnehapk commented Jan 15, 2020

Uh oh!

rawagner Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnehapk commented Jan 13, 2020 •

edited

Loading

bipuladh Jan 14, 2020 •

edited

Loading

rawagner Jan 15, 2020 •

edited

Loading