Bug 1829885: Snapshot code cleanup #5204

a2batic · 2020-04-27T21:11:28Z

The PR:

Removes snap scheduler code
Removes Labels column for snapshot list
Changes the format of Time using Timestamp component

Fixes: https://issues.redhat.com/browse/RHSTOR-1057
Signed-off-by: Kanika kmurarka@redhat.com

afreen23 · 2020-04-28T06:57:00Z

...es/ceph-storage-plugin/src/components/modals/volume-snapshot-modal/volume-snapshot-modal.tsx

+    handlePromise(k8sCreate(VolumeSnapshotModel, snapshotTemplate))
+      .then(close)
+      .catch((error) => {
+        // eslint-disable-next-line no-console
+        console.error(error);
+      });


IMO logging can be avoided. Internally handlePromise will throw error.

Suggested change

handlePromise(k8sCreate(VolumeSnapshotModel, snapshotTemplate))

.then(close)

.catch((error) => {

// eslint-disable-next-line no-console

console.error(error);

});

// eslint-disable-next-line promise/catch-or-return

handlePromise(k8sCreate(VolumeSnapshotModel, snapshotTemplate))

.then(close)

afreen23 · 2020-04-28T06:57:55Z

...es/ceph-storage-plugin/src/components/modals/volume-snapshot-modal/volume-snapshot-modal.tsx

 } from '@console/internal/components/factory';
-import { SnapshotScheduleModel, VolumeSnapshotModel } from '../../../models';
-import { cronLink, numberOfSnapshot, snapshotTypes } from './volume-snapshot';
+import { VolumeSnapshotModel } from '../../../models';
+import { snapshotTypes } from './volume-snapshot';
 import { getName, getNamespace } from '@console/shared';

 import { PersistentVolumeClaimModel } from '@console/internal/models';


console/shared and internal could be moved up.

afreen23 · 2020-04-28T07:03:39Z

frontend/packages/ceph-storage-plugin/src/components/volume-snapshot/volume-snapshot.tsx

@@ -169,7 +162,7 @@ export const VolumeSnapshotTableRow: RowFunction<K8sResourceKind> = ({
 export const VolumeSnapshotList: React.FC<TableProps> = (props) => (
  <Table
    {...props}
-    aria-label="Volume Snapshot"
+    aria-label={VolumeSnapshotModel.labelPlural}


Should it not be more elaborate like Volume Snapshot List ?

afreen23 · 2020-04-28T07:05:02Z

.../packages/ceph-storage-plugin/src/components/modals/volume-snapshot-modal/volume-snapshot.ts

+  // CronJob: 'CronJob',
+  // Monthly: 'Monthly',
+  // Weekly: 'Weekly',


A comment for commenting would be nice or rather remove it.

afreen23 · 2020-04-28T07:06:14Z

frontend/packages/ceph-storage-plugin/src/components/volume-snapshot/volume-snapshot.tsx

    </TableData>
    <TableData className={snapshotTableColumnClasses[3]}>
-      {obj?.status?.restoreSize || 'No Data'}
+      {obj.status.restoreSize || 'No Data'}


In console if I am not wrong, if some data is not available then a dash is used - for lists.

gnehapk · 2020-04-30T06:12:56Z

.../packages/ceph-storage-plugin/src/components/modals/volume-snapshot-modal/volume-snapshot.ts

 export const snapshotTypes = {
  Single: 'Single',
+  /* Other ways to create snapshot 


Since you have removed all the code for snapshot other than single, it makes sense to remove these comments too. WDYT?

afreen23

One minor comment.

afreen23 · 2020-04-30T08:18:01Z

frontend/packages/ceph-storage-plugin/src/components/volume-snapshot/volume-snapshot.tsx

@@ -169,7 +160,7 @@ export const VolumeSnapshotTableRow: RowFunction<K8sResourceKind> = ({
 export const VolumeSnapshotList: React.FC<TableProps> = (props) => (
  <Table
    {...props}
-    aria-label="Volume Snapshot"
+    aria-label={'Volume Snapshot List'}


Suggested change

aria-label={'Volume Snapshot List'}

aria-label='Volume Snapshot List'

Fixes: https://issues.redhat.com/browse/RHSTOR-1057 Signed-off-by: Kanika <kmurarka@redhat.com>

a2batic · 2020-04-30T14:34:11Z

/retitle Bug 1829885: Snapshot code cleanup

openshift-ci-robot · 2020-04-30T14:34:32Z

@a2batic: This pull request references Bugzilla bug 1829885, 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.5.0) matches configured target release for branch (4.5.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 1829885: Snapshot code cleanup

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.

a2batic · 2020-04-30T16:26:23Z

/bugzilla refresh

openshift-ci-robot · 2020-04-30T16:26:27Z

@a2batic: This pull request references Bugzilla bug 1829885, which is valid.

3 validation(s) were run on this bug

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

In response to this:

/bugzilla refresh

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.

afreen23 · 2020-04-30T16:27:19Z

/lgtm

bipuladh · 2020-04-30T16:33:20Z

/approve

openshift-ci-robot · 2020-04-30T16:37:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, afreen23, bipuladh

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/ceph-storage-plugin/OWNERS~~ [afreen23,bipuladh]

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

openshift-ci-robot · 2020-04-30T19:21:32Z

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

In response to this:

Bug 1829885: Snapshot code cleanup

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 requested review from bipuladh and shyRozen April 27, 2020 21:12

afreen23 suggested changes Apr 28, 2020

View reviewed changes

a2batic force-pushed the scheduling-cleanup branch from cb1c9b8 to e4ee7e6 Compare April 29, 2020 15:29

gnehapk reviewed Apr 30, 2020

View reviewed changes

afreen23 approved these changes Apr 30, 2020

View reviewed changes

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

Bug 1829885: Snapshot code cleanup

91ee365

Fixes: https://issues.redhat.com/browse/RHSTOR-1057 Signed-off-by: Kanika <kmurarka@redhat.com>

a2batic force-pushed the scheduling-cleanup branch from e4ee7e6 to 91ee365 Compare April 30, 2020 14:25

openshift-ci-robot changed the title ~~Snapshot code cleanup~~ Bug 1829885: Snapshot code cleanup Apr 30, 2020

openshift-ci-robot added the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Apr 30, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 30, 2020

a2batic requested review from afreen23 and gnehapk April 30, 2020 14:55

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. and removed bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. labels Apr 30, 2020

openshift-ci-robot assigned afreen23 Apr 30, 2020

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

bipuladh approved these changes Apr 30, 2020

View reviewed changes

openshift-merge-robot merged commit 474f0e6 into openshift:master Apr 30, 2020

spadgett added this to the v4.5 milestone May 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1829885: Snapshot code cleanup #5204

Bug 1829885: Snapshot code cleanup #5204

a2batic commented Apr 27, 2020 •

edited

afreen23 Apr 28, 2020

afreen23 Apr 28, 2020

afreen23 Apr 28, 2020

afreen23 Apr 28, 2020

afreen23 Apr 28, 2020

gnehapk Apr 30, 2020

afreen23 left a comment

afreen23 Apr 30, 2020

a2batic commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

a2batic commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

afreen23 commented Apr 30, 2020

bipuladh commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

	aria-label={'Volume Snapshot List'}
	aria-label='Volume Snapshot List'

Bug 1829885: Snapshot code cleanup #5204

Bug 1829885: Snapshot code cleanup #5204

Conversation

a2batic commented Apr 27, 2020 • edited

afreen23 Apr 28, 2020

Choose a reason for hiding this comment

afreen23 Apr 28, 2020

Choose a reason for hiding this comment

afreen23 Apr 28, 2020

Choose a reason for hiding this comment

afreen23 Apr 28, 2020

Choose a reason for hiding this comment

afreen23 Apr 28, 2020

Choose a reason for hiding this comment

gnehapk Apr 30, 2020

Choose a reason for hiding this comment

afreen23 left a comment

Choose a reason for hiding this comment

afreen23 Apr 30, 2020

Choose a reason for hiding this comment

a2batic commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

a2batic commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

afreen23 commented Apr 30, 2020

bipuladh commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

a2batic commented Apr 27, 2020 •

edited