Cleanup OverviewItem removing unnecessary fields, retrieve data as necessary #7207

jeff-phillips-18 · 2020-11-13T14:19:24Z

Fixes:
https://issues.redhat.com/browse/ODC-5088

Solution Description:
Remove pod information from OverviewItem. Create separate type for knative overview items. Cleanup resources fetched during topology load and fetch pod data when necessary.

Non-Functional Change

Browser conformance:

Chrome
Firefox
Safari
Edge

/kind cleanup

christianvogt · 2020-11-13T19:36:31Z

frontend/packages/console-shared/src/hooks/__tests__/usePluginsOverviewTabSection.spec.ts

@@ -1,6 +1,7 @@
 import * as React from 'react';
 import { Button } from '@patternfly/react-core';
 import { useExtensions, OverviewTabSection, LazyLoader } from '@console/plugin-sdk';
+import { KnativeServiceOverviewItem } from '@console/knative-plugin/src/topology/topology-types';


Can we do the test without knative dependency?

christianvogt · 2020-11-13T19:39:24Z

frontend/packages/dev-console/src/components/monitoring/overview/MonitoringTab.tsx

+  React.useEffect(() => {
+    if (!loadError && loaded) {
+      const updatedPods = podData.pods as PodKind[];
+      setPods(updatedPods);
+    }
+  }, [podData, loadError, loaded]);


This doesn't need to be done inside an effect. Instead derive in place or use useMemo.

christianvogt · 2020-11-13T19:42:01Z

frontend/packages/dev-console/src/components/topology/components/nodes/WorkloadNode.tsx

+  const donutStatus: PodRCData = React.useMemo(() => {
+    if (!loadError && loaded) {
+      return podData;
+    }
+    return null;
+  }, [loadError, loaded, podData]);


You don't need useMemo here because there isn't savings on efficiency:

Suggested change

const donutStatus: PodRCData = React.useMemo(() => {

if (!loadError && loaded) {

return podData;

}

return null;

}, [loadError, loaded, podData]);

const donutStatus: PodRCData = !loadError && loaded ? podData : null;

christianvogt · 2020-11-13T19:44:57Z

frontend/packages/knative-plugin/src/components/overview/EventSinkServicesOverviewList.tsx

+  const deploymentData = React.useMemo(
+    () => podData?.current?.obj?.metadata?.ownerReferences?.[0],
+    [podData],
+  );


Why would we need useMemo here instead of:

Suggested change

const deploymentData = React.useMemo(

() => podData?.current?.obj?.metadata?.ownerReferences?.[0],

[podData],

);

const deploymentData = podData?.current?.obj?.metadata?.ownerReferences?.[0];

christianvogt · 2020-11-13T19:49:43Z

frontend/packages/knative-plugin/src/components/overview/KnativeOverview.tsx

+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  const revisions = React.useMemo(() => [obj], [obj.metadata.uid]);


Whenever you disable any lint rules, please add a comment as to why.
This is a bit odd. If usePodsForRevisions only needs the uid's of the revisions, the API should probably be changed.

christianvogt · 2020-11-13T19:51:31Z

frontend/packages/knative-plugin/src/components/overview/KnativeServiceResources.tsx

+  const revisionIds = useDeepCompareMemoize(
+    revisions?.map((r) => r.metadata.uid).sort((a, b) => a.localeCompare(b)),
+  );
+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  const revisionResources = React.useMemo(() => [...revisions], [revisionIds]);


Same comment as before.
The issue is more noticeable now.

christianvogt · 2020-11-13T19:51:46Z

frontend/packages/knative-plugin/src/components/overview/KnativeServiceResources.tsx

+  React.useEffect(() => {
+    if (loaded) {
+      const revisionsPods = [];
+      pods.forEach((pod) => {
+        if (pod.pods) {
+          revisionsPods.push(
+            ...pod.pods.filter((p) => podPhase(p as PodKind) !== AllPodStatus.AutoScaledTo0),
+          );
+        }
+      });
+      setServicePods(revisionsPods);
+    }
+  }, [loaded, pods]);


use useMemo instead

christianvogt · 2020-11-13T19:52:28Z

frontend/packages/knative-plugin/src/topology/components/nodes/RevisionNode.tsx

+  React.useEffect(() => {
+    if (loaded) {
+      const [current, previous] = pods;
+      const isRollingOut = !!current && !!previous;
+      setDonutStatus({
+        obj: resource,
+        current,
+        previous,
+        isRollingOut,
+        pods: [...(current?.pods || []), ...(previous?.pods || [])],
+      });
+    }
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [loaded, pods]);


use useMemo instead

christianvogt · 2020-11-13T19:52:34Z

frontend/packages/knative-plugin/src/topology/components/nodes/RevisionNode.tsx

+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  const revisions = React.useMemo(() => [resource], [resource.metadata.uid]);


Same comment as before.

christianvogt · 2020-11-13T19:55:38Z

frontend/packages/knative-plugin/src/topology/knative-topology-utils.ts

@@ -951,7 +936,7 @@ export const transformKnNodeData = (
          item,
          type,
          getImageForIconClass(`icon-openshift`),
-        );
+        ) as any;


Why cast too any ?

jeff-phillips-18 · 2020-11-16T17:42:50Z

@christianvogt updated per comments. Moved test data around a to remove some of the undesirable dependencies.

christianvogt · 2020-11-17T17:22:23Z

frontend/packages/knative-plugin/src/components/overview/KnativeRevisionResources.tsx

+    if (loaded && !loadError) {
+      return pods.reduce((acc, pod) => {
+        if (pod.pods) {
+          acc.push(...pod.pods.filter((p) => podPhase(p as PodKind) !== AUTOSCALED));


Suggested change

acc.push(...pod.pods.filter((p) => podPhase(p as PodKind) !== AUTOSCALED));

acc.push(...pod.pods.filter((p) => podPhase(p as PodKind) !== AllPodStatus.AutoScaledTo0));

christianvogt · 2020-11-17T17:39:10Z

lost the deployment and donut on revisions:

before:

…essary

christianvogt · 2020-11-17T20:21:59Z

/lgtm
/approve

openshift-ci-robot · 2020-11-17T20:22:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

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/OWNERS~~ [christianvogt]

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

jeff-phillips-18 · 2020-11-17T20:54:10Z

/retest

jeff-phillips-18 · 2020-11-18T00:45:40Z

/test e2e-gcp-console

openshift-bot · 2020-11-18T04:24:19Z

/retest

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

openshift-bot · 2020-11-18T04:37:13Z

/retest

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

jeff-phillips-18 requested a review from jerolimov November 13, 2020 14:19

openshift-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. component/core Related to console core functionality labels Nov 13, 2020

openshift-ci-robot requested review from debsmita1 and glekner November 13, 2020 14:19

openshift-ci-robot added component/dev-console Related to dev-console component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared labels Nov 13, 2020

jeff-phillips-18 force-pushed the overview-item branch from 5f7907c to 462f3da Compare November 13, 2020 16:54

christianvogt reviewed Nov 13, 2020

View reviewed changes

jeff-phillips-18 force-pushed the overview-item branch from 357fc55 to 3ec42ab Compare November 16, 2020 17:41

christianvogt reviewed Nov 17, 2020

View reviewed changes

jeff-phillips-18 force-pushed the overview-item branch 2 times, most recently from fa10167 to 6a52b81 Compare November 17, 2020 18:28

Cleanup OverviewItem removing unecessary fields, retrieve data as nec…

a8048a6

…essary

jeff-phillips-18 force-pushed the overview-item branch from 6a52b81 to a8048a6 Compare November 17, 2020 19:58

openshift-ci-robot assigned christianvogt Nov 17, 2020

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

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

openshift-merge-robot merged commit d70027e into openshift:master Nov 18, 2020

spadgett added this to the v4.7 milestone Nov 30, 2020

jeff-phillips-18 deleted the overview-item branch December 2, 2020 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup OverviewItem removing unnecessary fields, retrieve data as necessary #7207

Cleanup OverviewItem removing unnecessary fields, retrieve data as necessary #7207

jeff-phillips-18 commented Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

christianvogt Nov 13, 2020

jeff-phillips-18 commented Nov 16, 2020

christianvogt Nov 17, 2020

christianvogt commented Nov 17, 2020

christianvogt commented Nov 17, 2020

openshift-ci-robot commented Nov 17, 2020

jeff-phillips-18 commented Nov 17, 2020

jeff-phillips-18 commented Nov 18, 2020

openshift-bot commented Nov 18, 2020

openshift-bot commented Nov 18, 2020

		// eslint-disable-next-line react-hooks/exhaustive-deps
		const revisions = React.useMemo(() => [obj], [obj.metadata.uid]);

		// eslint-disable-next-line react-hooks/exhaustive-deps
		const revisions = React.useMemo(() => [resource], [resource.metadata.uid]);

	acc.push(...pod.pods.filter((p) => podPhase(p as PodKind) !== AUTOSCALED));
	acc.push(...pod.pods.filter((p) => podPhase(p as PodKind) !== AllPodStatus.AutoScaledTo0));

Cleanup OverviewItem removing unnecessary fields, retrieve data as necessary #7207

Cleanup OverviewItem removing unnecessary fields, retrieve data as necessary #7207

Conversation

jeff-phillips-18 commented Nov 13, 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

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

jeff-phillips-18 commented Nov 16, 2020

Choose a reason for hiding this comment

christianvogt commented Nov 17, 2020

christianvogt commented Nov 17, 2020

openshift-ci-robot commented Nov 17, 2020

jeff-phillips-18 commented Nov 17, 2020

jeff-phillips-18 commented Nov 18, 2020

openshift-bot commented Nov 18, 2020

openshift-bot commented Nov 18, 2020