feat(topology filter): added filter for knative event sources #3774

nemesis09 · 2019-12-12T13:53:24Z

This PR adds functionality for topology filter option "event sources" under the "show" section
Jira issue - https://jira.coreos.com/browse/ODC-2458
this PR depends on #3746
screens

debsmita1 · 2019-12-12T17:15:58Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

@@ -365,7 +365,9 @@ export const transformTopologyData = (

  const knSvcResources: K8sResourceKind[] = _.get(resources, ['ksservices', 'data'], []);
  knSvcResources.length && getKnativeTopologyData(knSvcResources, NodeType.KnService);
-  const knEventSources: K8sResourceKind[] = getKnativeEventSources();
+  const knEventSources: K8sResourceKind[] = filters.display.eventSources


you might want to write it like :

Suggested change

const knEventSources: K8sResourceKind[] = filters.display.eventSources

const knEventSources: K8sResourceKind[] = filters.display.eventSources && getKnativeEventSources();

I caution against this change. && is a sneaky beast in JavaScript. I am unsure if TypeScript properly catches this, but if your left-hand variable is falsey, you get that value. This isn't so much of a problem when rendering React as falsey usually means don't render it.

However if you do it with a JS statement, you can be getting unexpected values into your variable:

1 && 2 // 2 0 && 1 // 0 true && [] // [] false && [] // false undefined && ['hello', 'world'] // undefined

Which can be confusing for your variable (in this case knEventSources). Especially if you want it to be an array.

@andrewballantyne Thank you for this piece of information.
When I executed this PR locally (after pulling in #3746) it worked fine without an error so I went ahead and suggested the change. But I will make a note of the example you just shared

vikram-raj · 2019-12-16T11:10:54Z

/retest

vikram-raj · 2019-12-16T13:57:07Z

@nemesis09 All tests are failing. Can you please rebase your PR with the latest master.

nemesis09 · 2019-12-17T12:06:35Z

@nemesis09 All tests are failing. Can you please rebase your PR with the latest master.

@vikram-raj waiting for PR #3746 to get in.

christianvogt · 2019-12-17T20:56:57Z

/hold

andrewballantyne · 2020-01-02T20:11:05Z

/kind feature

andrewballantyne · 2020-01-03T05:00:41Z

/hold cancel
/retest

andrewballantyne · 2020-01-03T05:24:21Z

@nemesis09 looks like you failed unit tests. Please take a look when you get a chance.

/hold

andrewballantyne · 2020-01-05T22:02:15Z

/hold cancel
/retitle [WIP] feat(topology filter): added filter for knative event sources

Adding the WIP label to distinguish that this is not ready for reviews.

@nemesis09 Please see my previous message

nemesis09 · 2020-01-07T11:38:56Z

@nemesis09 looks like you failed unit tests. Please take a look when you get a chance.

/hold

I've added unit tests for the code change. PTAL

sahil143 · 2020-01-07T12:26:23Z

frontend/packages/dev-console/src/components/topology/__tests__/topology-utils.spec.ts

+    const eventFilter: TopologyFilters = {
+      display: {
+        podCount: true,
+        eventSources: true,
+        knativeServices: true,
+        appGrouping: true,
+        operatorGrouping: true,
+      },
+      searchQuery: '',
+    };


declare it once either in beforeEach or out of describe block. and you should be importing this from redux/const.ts
and do something like this
const topologyFilters = {...DEFAULT_TOPOLOGY_FILTERS};
and change the topology filter in each test case as you need it.

I've added the changes. PTAL

andrewballantyne · 2020-01-07T15:27:29Z

@christianvogt @jeff-phillips-18 This is a bit odd. When I toggle it on / off, the Knative snaps around a bit. Any ideas? I don't think this PR is at fault as it's just filtering.

openshift-bot · 2020-01-09T22:20:47Z