Show csv icon in topology for operator backed services #3787

rohitkrai03 · 2019-12-15T18:30:54Z

Jira Issue - https://issues.redhat.com/browse/ODC-2455

This PR -

Add support to show icons from CSV data for an operator backed service.
Refactors the topology-utils code to include CSV data along with other resources.
Refactors the code to get icon for the node. It now also checks if the node is operator backed service and gets the image from CSV spec.
Adds a utility function for getting icon from CSV data in @console/shared.
Adds new unit tests for checking the node icon in topology.
Also fixes failing tests and updates the snapshot.

Screenshot @openshift/team-devconsole-ux -

rohitkrai03 · 2019-12-15T18:34:15Z

/assign @christianvogt

invincibleJai · 2019-12-16T15:03:13Z

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

+
+export const getImageForCSVIcon = (csv) =>
+  _.get(csv, 'spec.icon')
+    ? `data:${_.get(csv, 'spec.icon', [])[0].mediatype};base64,${


assigning default as [] so in case of default it may break as [0] would be undefined and we would access mediatype on that

invincibleJai · 2019-12-16T15:04:25Z

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

+    expect((topologyTransformedData[keys[0]].data as WorkloadData).builderImage).toBe(nodejsIcon);
+  });
+
+  it('should return builder image icon for nodejs', () => {


nit: it would be good to use different description in it block for this one and above

Sorry, that was because of copy paste job. Updated the description.

christianvogt · 2019-12-17T04:14:40Z

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

@@ -23,4 +23,8 @@ export type OverviewItem<T = K8sResourceKind> = {
  revisions?: K8sResourceKind[];
 };

+export type OperatorBackedServiceKindMap = {


Why is this in shared?
Seems very specific to topology.

Its being used by knative-plugin as well that's why I had to add it to console-shared.

But knative-plugin depends on dev-console for contributing to topology.
You should be enhancing the TopologyDataObject.

It is not actually part of TopologyDataObject. So, moved the type definition to topology-types.ts file.

You're right. I misread where it was. But topology-types.ts is a better home for it.

divyanshiGupta · 2019-12-17T10:31:45Z

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

+import * as operatorLogo from '../images/operator.svg';
+
+export const getImageForCSVIcon = (csv) =>
+  _.get(csv, 'spec.icon')


Instead of using get at three places, probably store it as a const. And to address what Jai said you can do this or something similar

const icon = _.get(csv, 'spec.icon', []); !_.isEmpty(icon) ? `data:${icon[0].mediatype};base64,${icon[0].base64data}` : operatorLogo

rohitkrai03 · 2019-12-17T14:01:51Z

/retest

rohitkrai03 · 2019-12-17T20:43:55Z

/retest

christianvogt · 2019-12-17T20:54:08Z

frontend/packages/console-shared/src/images/operator.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 258.51 258.51"><defs><style>.cls-1{fill:#d1d1d1;}.cls-2{fill:#8d8d8f;}</style></defs><title>Asset 4</title><g id="Layer_2" data-name="Layer 2"><g id="Layer_1-2" data-name="Layer 1"><path class="cls-1" d="M129.25,20A109.1,109.1,0,0,1,206.4,206.4,109.1,109.1,0,1,1,52.11,52.11,108.45,108.45,0,0,1,129.25,20m0-20h0C58.16,0,0,58.16,0,129.25H0c0,71.09,58.16,129.26,129.25,129.26h0c71.09,0,129.26-58.17,129.26-129.26h0C258.51,58.16,200.34,0,129.25,0Z"/><path class="cls-2" d="M177.54,103.41H141.66L154.9,65.76c1.25-4.4-2.33-8.76-7.21-8.76H102.93a7.32,7.32,0,0,0-7.4,6l-10,69.61c-.59,4.17,2.89,7.89,7.4,7.89h36.9L115.55,197c-1.12,4.41,2.48,8.55,7.24,8.55a7.58,7.58,0,0,0,6.47-3.48L184,113.85C186.86,109.24,183.29,103.41,177.54,103.41Z"/></g></g></svg>


There are 2 other instances of what looks to be the same file:

public/imgs

packages/operator-lifecycle-management/src

Can we consolidate and settle on a single instance.

I have another PR that touches the same area. That's why I didn't make changes there, I plan to do that in another PR. Or if the other PR gets in first I can add that change in this one.

andrewballantyne · 2019-12-18T00:05:32Z

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

+import * as operatorLogo from '../images/operator.svg';
+
+export const getImageForCSVIcon = (csv) => {
+  const icon = _.get(csv, 'spec.icon', []);


FWIW, I think you can do this now:

Suggested change

const icon = _.get(csv, 'spec.icon', []);

const icon = csv?.spec?.icon || [];

Thanks for the suggestion. I was not aware that typescript has been upgraded in console.

Well, we also need to update prettier to be able to use this feature.

Ah interesting, I turned off my watcher for my review version of OpenShift... does it to something crazy?

Its throwing an error saying expression expected after the ?.

Hmmm ... I didn't see any inline eslint errors when I wrote it :/ ah well, maybe for a future time then.

Now, i've broken eslint-prettier-plugin trying to update prettier. Should look at this in another PR. I can raise one after this.

Don't bother with it. There's already a PR up to upgrade eslint. We cannot use the feature yet.

andrewballantyne · 2019-12-18T00:11:21Z

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

+import * as _ from 'lodash';
+import * as operatorLogo from '../images/operator.svg';
+
+export const getImageForCSVIcon = (csv) => {


Type csv? It's a K8sResourceKind in your tests, is there a definition shared we can use? Something like ClusterServiceVersionKind? Can we import from the operator-lifecycle-manager? Is this the right type?

Yes, that is the correct type. Updated it..

So after talking to Christian, he suggests we do not make this dependency. It pollutes what the shared package is trying to do. So in order to keep this, he suggested putting it into dev-console. Does that seem viable?

andrewballantyne · 2019-12-18T00:15:49Z

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

@@ -125,25 +131,33 @@ export const createTopologyNodeData = (
  const deploymentsLabels = _.get(deploymentConfig, 'metadata.labels', {});
  const deploymentsAnnotations = _.get(deploymentConfig, 'metadata.annotations', {});
  const nodeResourceKind = _.get(deploymentConfig, 'metadata.ownerReferences[0].kind');
+  const operatorBackedService = nodeResourceKind in operatorBackedServiceKindMap;
+  const getNodeIcon = () => {


Any reason this was made a function? Seems like it's only used once and lazy computing isn't helping in this case.

No specific reason for making it a function. Just didn't want to write a big ternary operator expression.

I don't think you need to go to either extreme... you can just create the variables inline (instead of wrapped in a function) and just use the result from the return in the spot where you call it now.

andrewballantyne · 2019-12-18T00:32:21Z

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

-      );
-    });
+    operatorBackedServiceKindMap = installedOperators.reduce((kindMap, csv) => {
+      _.get(csv, 'spec.customresourcedefinitions.owned').forEach((crd) => {


Is there any concern that an installed operator would not have CRDs? Is this somehow guaranteed by the spec never to be missing?

Based on the Operator SDK - https://github.com/operator-framework/operator-sdk#workflow, there can be operators which do not provide any CRDs.

I guess my question is more about your _.get doesn't handle a fallback if it is missing.

But, I am yet to find an operator from OperatorHub which is not providing any APIs.

I suppose I am being overly cautious... defaulting it to an array saves us from a NPE if it happens. 🤷‍♂ It probably isn't needed one way or the other though. Just a recommendation for being "defensive" in programming.

Added default array. It's good to be cautious. :)

christianvogt · 2019-12-19T03:42:34Z

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

+export const getImageForCSVIcon = (csv: ClusterServiceVersionKind) => {
+  const icon: ClusterServiceVersionIcon = _.get(csv, 'spec.icon', []);
+  return !_.isEmpty(icon) ? `data:${icon[0].mediatype};base64,${icon[0].base64data}` : operatorLogo;


Importing '@console/operator-lifecycle-manager' from @console-shared is bad.
To remove this dependency, perhaps you type the param as much simpler type to match the CSV icon:

type Icon = { base64data: string; mediatype: string }; export const getOperatorIcon = (icon: Icon[]) => {

andrewballantyne · 2020-01-02T19:46:14Z

/kind feature

andrewballantyne · 2020-01-02T20:56:15Z

/hold

andrewballantyne

Just some weirdness with the API

andrewballantyne · 2020-01-02T20:54:21Z

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

+
+type CSVIcon = { base64data: string; mediatype: string };
+
+export const getImageForCSVIcon = (icon: CSVIcon[]) => {


Suggested change

export const getImageForCSVIcon = (icon: CSVIcon[]) => {

export const getImageForCSVIcons = (icons: CSVIcon[]) => {

More importantly, why are you passing an array only to grab the first one?

andrewballantyne · 2020-01-02T21:01:53Z

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

+
+  const csvIcon =
+    operatorBackedService &&
+    getImageForCSVIcon(_.get(operatorBackedServiceKindMap[nodeResourceKind], 'spec.icon', []));


Per my other comment ... Perhaps [0] here to simplify the utility api?

andrewballantyne · 2020-01-02T23:41:33Z

/hold cancel
/retest
#3847 has fixed the e2e tests

andrewballantyne · 2020-01-03T04:58:51Z

@rohitkrai03 needs rebase.

rohitkrai03 · 2020-01-06T02:49:55Z

/assign

@rohitkrai03 I am getting an error only when I checkout your branch (not visible on master)... it's an odd one that I don't know how you could have caused it:

I debugged it a bit but I'm still quite confused... essentially allModels is not defined and this is the error that gets printed out. I think this is a webpack bundle issue, but it may also be a circular reference or something. Do you experience this issue? Just loading up any cluster with this code causes it (and whitescreens the app)

@andrewballantyne Yeah, this was because of circular dependency. I encountered this on Friday and reverted back the OLM package changes. But seems like I forgot to force push the changes. SHould work now. I've updated the PR.

andrewballantyne

/lgtm

I don't have the powers to give the approve label in olm and shared, so @christianvogt will have to weigh in there. But looks good to me.

christianvogt · 2020-01-06T14:42:22Z

frontend/packages/console-shared/src/utils/__tests__/icon-utils.spec.ts

+    const mockCSV = _.cloneDeep(sampleClusterServiceVersions.data[0]);
+    const icon = _.get(mockCSV, 'spec.icon.0');


You don't need topology test data for this unit test.
Please update to use an object that matches type CSVIcon.

Looks like there is a topology specific test added later on that captures this same data flow.

christianvogt · 2020-01-06T15:16:51Z

/lgtm
/approved

openshift-ci-robot · 2020-01-06T15:17:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, rohitkrai03

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-bot · 2020-01-06T15:38:50Z

/retest