Fix for helm application filters, refactor topology utils #4778

jeff-phillips-18 · 2020-03-19T18:31:48Z

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

Analysis / Root cause:
Workloads were being filtered out during data creation time and the helm workloads do not contain the application label so they were not being created.

Solution Description:
Updated the mechanism for application filtering to create all the nodes and hide those not in the filtered application.

Large refactor of the data creation functions to simplify parameters and to break it up into the different types (knative, helm, operators, base). Eventually this should be changed to use plugins for each to make it more extensible as more types are added.

Screen shots / Gifs for design review:
No visible changes

Unit test coverage report:

Browser conformance:

Chrome
Firefox
Safari
Edge

jeff-phillips-18 · 2020-03-19T19:15:31Z

/retest

andrewballantyne · 2020-03-20T18:05:37Z

/kind bug
/kind cleanup

andrewballantyne

There are a lot of references from knative-plugin to dev-console ... which we need to make use of the plugin infrastructure. cc @christianvogt

You are also directly using Redux Store getState... which is not a great idea as this breaks the paradigm of Redux. Not sure your refactors in this case should be done. They appear to have used the proper connect statement which is the appropriate way to do it. There are hooks and HoCs that can be used to help streamline it likely, but we shouldn't do import store; store.getState();.

andrewballantyne · 2020-03-23T14:18:29Z

frontend/packages/console-shared/src/utils/resource-utils.ts

+    resourceList.forEach((resource) => {
+      utils = [...utils, resource.properties.utils];
+    });


Suggested change

resourceList.forEach((resource) => {

utils = [...utils, resource.properties.utils];

});

utils = resourceList.map((resource) => resource.properties.utils);

andrewballantyne · 2020-03-23T14:18:57Z

frontend/packages/console-shared/src/utils/resource-utils.ts

+
+export const getResourceUtils = (resList?: any): Function[] => {
+  let utils = [];
+  const state = store.getState();


This is deprecated. If you need the feature flags you'll have to provide it somehow. This is bad Redux and never should have been done in the first place by Console.

cc @christianvogt not sure if we have a better mechanism to getting the feature flags though 😞

global access to redux store is a no go. We need to use connect or hooks.

andrewballantyne · 2020-03-23T14:29:33Z

frontend/packages/console-shared/src/utils/resource-utils.ts

@@ -649,8 +671,11 @@ export class TransformResourceData {
    });
  };

-  public createDeploymentConfigItems = (deploymentConfigs: K8sResourceKind[]): OverviewItem[] => {
-    return _.map(deploymentConfigs, (dc) => {
+  public createDeploymentConfigItems = (


You'll need to expand the tests for these 4 methods to cover the new param you added.

andrewballantyne · 2020-03-23T14:44:32Z

frontend/packages/dev-console/src/components/topology/actions/edgeActions.ts

-  TYPE_TRAFFIC_CONNECTOR,
-} from '../const';
-import { moveConnectionModal } from '../components/MoveConnectionModal';
+} from '@console/knative-plugin/src/topology/const';


Please reorder the import and put with the rest of the package imports.

cc @christianvogt Do we have a better mechanism for getting constants than a direct import from a package?

andrewballantyne · 2020-03-23T14:55:20Z

frontend/packages/dev-console/src/components/topology/components/MoveConnectionModal.tsx

-
-const mapStateToProps = (state: RootState): StateProps => {
-  return {
-    serviceBinding: getServiceBindingStatus(state),


Not sure we can get rid of this imo. This is kind of important functionality of Redux.

andrewballantyne · 2020-03-23T16:09:09Z

frontend/packages/knative-plugin/src/topology/components/groups/KnativeService.scss

@@ -1,4 +1,4 @@
-@import '../../topology-utils';
+@import '../../../../../dev-console/src/components/topology/topology-utils';


😕 I am not sure we have a plugin infrastructure for SCSS.

cc @christianvogt

andrewballantyne · 2020-03-23T16:10:14Z

frontend/packages/knative-plugin/src/topology/components/groups/KnativeService.tsx

@@ -9,7 +9,7 @@ import {
 } from '@console/topology';
 import { modelFor, referenceFor } from '@console/internal/module/k8s';
 import { useAccessReview } from '@console/internal/components/utils';
-import { getTopologyResourceObject } from '../../topology-utils';
+import { getTopologyResourceObject } from '@console/dev-console/src/components/topology/topology-utils';


Definitely need a plugin infrastructure for utilities -- I do not currently see one in place.

cc @christianvogt

Correct. That is the longer term goal. This PR is a stepping stone towards that.

andrewballantyne · 2020-03-23T16:12:00Z

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

@@ -13,9 +13,12 @@ import {
  createSvgIdUrl,
 } from '@console/topology';
 import { getKnativeEventSourceIcon } from '@console/knative-plugin';


Probably doesn't need to be package referenced anymore.

andrewballantyne · 2020-03-23T16:16:12Z

frontend/packages/knative-plugin/src/topology/data-transformer.ts

+  TopologyDataResources,
+} from '@console/dev-console/src/components/topology/topology-types';
+import { addToTopologyDataModel } from '@console/dev-console/src/components/topology/data-transforms/transform-utils';
+import { getOperatorBackedServiceKindMap, OperatorBackedServiceKindMap } from '@console/shared/src';


Suggested change

import { getOperatorBackedServiceKindMap, OperatorBackedServiceKindMap } from '@console/shared/src';

import { getOperatorBackedServiceKindMap, OperatorBackedServiceKindMap } from '@console/shared';

andrewballantyne · 2020-03-23T16:16:22Z

frontend/packages/knative-plugin/src/topology/data-transformer.ts

@@ -0,0 +1,112 @@
+import * as _ from 'lodash';
+import { DeploymentKind, K8sResourceKind } from '@console/internal/module/k8s';
+import { ClusterServiceVersionKind } from '@console/operator-lifecycle-manager/src';


Suggested change

import { ClusterServiceVersionKind } from '@console/operator-lifecycle-manager/src';

import { ClusterServiceVersionKind } from '@console/operator-lifecycle-manager';

jeff-phillips-18 · 2020-03-23T16:44:28Z

The idea here was to simplify the calls and avoid passing state through to all the lower levels. Using store.getState() is not necessarily a bad thing and is used in other sections of console code. Currently we get the state from connect to pass it down and down and down until eventually needed. This seems worse to me as it just seems to be a continuously growing list of parameters.

The plan would be to follow up with a plugin mechanism but wanted to get the simplification in first to make that process easier.

andrewballantyne · 2020-03-23T20:54:46Z

The idea here was to simplify the calls and avoid passing state through to all the lower levels. Using store.getState() is not necessarily a bad thing and is used in other sections of console code. Currently we get the state from connect to pass it down and down and down until eventually needed. This seems worse to me as it just seems to be a continuously growing list of parameters.

The plan would be to follow up with a plugin mechanism but wanted to get the simplification in first to make that process easier.

Let's put aside what console code already does... there were a lot of early learning problems they have yet to correct. Getting access to store state directly is bad form as you're not subscribing for changes. Sure, in your case, it doesn't matter a ton as you're just asking at a "point in time" what the feature flag states are... but the functions you use it in are no longer testable (eg. pure functions).

You're doing impure things inside a function for convenience, this is a poor model to grow code on. Passing variables down as they are relevant is perfectly fine for a functional programming methodology. If your path is growing in design and needs the world of params, that's likely an indication that it should be broken up / address from a different angle; like computed in parts and combined in another.

In this case you've removed a simple "get me the state of a single feature flag" and a few threading of the variable to making an impure method that every time it's invoked can result in a different outcome. I don't think we should give up good design for convenience... what is short term gains will hurt us in the long run.

For the case of using plugins as well, we have developed hooks for this that were added in #3383. We should be able to keep a proper plugin infrastructure and redux connectivity without resorting to impure global accessors.

christianvogt

Lots of good changes here.
I think grouping related files (ie data transformers, components, utils etc) in a single folder such as helm, knative, operator-backed, etc... would be good in future to better support an extension model.

christianvogt · 2020-03-23T21:54:24Z

frontend/packages/knative-plugin/src/topology/layouts/getColaLayoutConstraints.ts

+const nodeSorter = (node1: ColaNode, node2: ColaNode) =>
+  getNodeTimeStamp(node1) > getNodeTimeStamp(node2) ? -1 : 1;
+
+export const getColaLayoutConstraints = (


Can we rename file to layoutConstraints instead.

christianvogt · 2020-03-23T21:58:47Z

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

+import KnativeService from './components/groups/KnativeService';
+import { nodeContextMenu } from '@console/dev-console/src/components/topology/nodeContextMenu';
+import RevisionNode from './components/nodes/RevisionNode';
+import TrafficLink from '@console/dev-console/src/components/topology/components/edges/TrafficLink';


Shouldn't this move to the knative plugin as well?

christianvogt · 2020-03-23T22:09:11Z

frontend/packages/dev-console/src/components/topology/data-transforms/topology-model.ts

+  application: string = ALL_APPLICATIONS_KEY,
+  filters?: TopologyFilters,
+): Model => {
+  const groupNodes: NodeModel[] = dataModel.graph.groups.map((d) => {


We should look to remove the intermediate data model and handle groups as nodes in the future. Probably too much to do as part of this refactor.

andrewballantyne · 2020-03-25T16:54:25Z

frontend/packages/dev-console/src/components/topology/Topology.tsx

+  if (!knativeComponentFactoryRef.current) {
+    knativeComponentFactoryRef.current = new KnativeComponentFactory(serviceBinding);
+  }
+  if (!helmComponentFactoryRef.current) {
+    helmComponentFactoryRef.current = new HelmComponentFactory(serviceBinding);
+  }
+  if (!operatorsComponentFactoryRef.current) {
+    operatorsComponentFactoryRef.current = new OperatorsComponentFactory(serviceBinding);
+  }


Curious. I think you can just do React.useRef<Type>(new MyItem(value)). I think that achieves the same net result.

@jeff-phillips-18 any consideration to the above? It'll save you the use of the if statements.

andrewballantyne · 2020-03-25T16:55:41Z

frontend/packages/dev-console/src/components/topology/Topology.tsx

@@ -260,10 +296,13 @@ const Topology: React.FC<TopologyProps> = ({ data, serviceBinding, filters, acti
  );
 };

+const getServiceBindingStatus = ({ FLAGS }: RootState): boolean => FLAGS.get(ALLOW_SERVICE_BINDING);


Last I reviewed (forgot to mention this - recalled when i saw this line) this exists already. Can you do a quick search for ALLOW_SERVICE_BINDING and see if it's implemented multiple times. Think we can probably reuse here.

andrewballantyne · 2020-03-25T16:58:28Z

frontend/packages/dev-console/src/components/topology/TopologyDataController.tsx

-}) =>
-  render({
+}) => {
+  return render({


I assume a debugging refactor? Not sure this change is needed, but no skin off my nose to leave it.

andrewballantyne · 2020-03-25T17:01:23Z

frontend/packages/dev-console/src/components/topology/TopologyDataController.tsx

      />
    </Firehose>
  );
 };

+const getServiceBindingStatus = ({ FLAGS }: RootState): boolean => FLAGS.get(ALLOW_SERVICE_BINDING);


Ah, I wonder if this is the one I saw... Can we find a common spot for these?

andrewballantyne · 2020-03-26T19:36:16Z

frontend/packages/dev-console/src/components/topology/components/groups/Application.scss

-@import '~@patternfly/patternfly/sass-utilities/colors';
+@import '../../../../../../../node_modules/@patternfly/patternfly/sass-utilities/colors';


Yikes... what happened here? Is this really required? This looks horrible lol.

not sure how that happened, works fine the original way. Sorry I missed that.

andrewballantyne

Okay, my eyes hurt... but I think I am done. Just a couple disjointed comments I have posted above. Feel free to mark our conversations resolved when you are good with them @jeff-phillips-18 might be easier to re-review after the last set of changes (I think) you need.

andrewballantyne

/lgtm

Big refactor, let's try to get it merged and iterate over it.

andrewballantyne · 2020-03-26T21:50:14Z

More conflicts 🙈

andrewballantyne · 2020-03-27T12:39:48Z

/lgtm

christianvogt · 2020-03-27T19:27:51Z

/approve

christianvogt · 2020-03-27T19:28:02Z

@jeff-phillips-18 rebase

Fix to set nodes filtered by application to be not visible

andrewballantyne · 2020-03-27T20:06:04Z

/lgtm

openshift-ci-robot · 2020-03-27T20:06:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

openshift-ci-robot added component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin component/shared Related to console-shared labels Mar 19, 2020

openshift-ci-robot requested review from divyanshiGupta and vikram-raj March 19, 2020 18:32

jeff-phillips-18 force-pushed the refactor-topology-utils branch from 1e25202 to 19ea3de Compare March 19, 2020 20:07

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

openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Mar 20, 2020

jeff-phillips-18 force-pushed the refactor-topology-utils branch from 19ea3de to d30d3f3 Compare March 20, 2020 18:46

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

andrewballantyne suggested changes Mar 23, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Mar 23, 2020

christianvogt reviewed Mar 23, 2020

View reviewed changes

jeff-phillips-18 force-pushed the refactor-topology-utils branch from d30d3f3 to 0a8d45d Compare March 25, 2020 16:16

andrewballantyne reviewed Mar 25, 2020

View reviewed changes

jeff-phillips-18 force-pushed the refactor-topology-utils branch from 0a8d45d to c205af3 Compare March 25, 2020 20:46

andrewballantyne reviewed Mar 26, 2020

View reviewed changes

jeff-phillips-18 force-pushed the refactor-topology-utils branch from c205af3 to 29daec6 Compare March 26, 2020 20:57

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

andrewballantyne approved these changes Mar 26, 2020

View reviewed changes

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

jeff-phillips-18 force-pushed the refactor-topology-utils branch from 29daec6 to 30d1ce9 Compare March 27, 2020 10:34

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 27, 2020

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

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

jeff-phillips-18 added 2 commits March 27, 2020 15:58

Refactor topology utils, simplify imports, restructure topology code

d526bc9

Fix to set nodes filtered by application to be not visible

Address review comments

1f14278

jeff-phillips-18 force-pushed the refactor-topology-utils branch from 30d1ce9 to 1f14278 Compare March 27, 2020 20:05

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020

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

openshift-merge-robot merged commit 26a372a into openshift:master Mar 27, 2020

spadgett added this to the v4.5 milestone Mar 31, 2020

jeff-phillips-18 deleted the refactor-topology-utils branch December 2, 2020 13:35

		@@ -1,4 +1,4 @@
		@import '../../topology-utils';
		@import '../../../../../dev-console/src/components/topology/topology-utils';

	import { getOperatorBackedServiceKindMap, OperatorBackedServiceKindMap } from '@console/shared/src';
	import { getOperatorBackedServiceKindMap, OperatorBackedServiceKindMap } from '@console/shared';

	import { ClusterServiceVersionKind } from '@console/operator-lifecycle-manager/src';
	import { ClusterServiceVersionKind } from '@console/operator-lifecycle-manager';

		@import '~@patternfly/patternfly/sass-utilities/colors';
		@import '../../../../../../../node_modules/@patternfly/patternfly/sass-utilities/colors';

Fix for helm application filters, refactor topology utils #4778

Fix for helm application filters, refactor topology utils #4778

Conversation

jeff-phillips-18 commented Mar 19, 2020

jeff-phillips-18 commented Mar 19, 2020

andrewballantyne commented Mar 20, 2020

andrewballantyne 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

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 Mar 23, 2020

andrewballantyne commented Mar 23, 2020

christianvogt 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

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

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Mar 26, 2020

andrewballantyne commented Mar 27, 2020

christianvogt commented Mar 27, 2020

christianvogt commented Mar 27, 2020

andrewballantyne commented Mar 27, 2020

openshift-ci-robot commented Mar 27, 2020