add dynamic sdk support for topology extensions #9055

glekner · 2021-05-26T14:31:38Z

No description provided.

glekner · 2021-05-27T09:49:13Z

/retest

christianvogt · 2021-05-31T17:16:29Z

frontend/packages/topology/src/components/graph-view/Topology.tsx

  const componentFactoryExtensions = useExtensions<TopologyComponentFactory>(
    isTopologyComponentFactory,
  );
+  const [dynamicComponentFactoryExtensions, isFactoryComponentsResolved] = useResolvedExtensions<
+    DynamicTopologyComponentFactory
+  >(isDynamicTopologyComponentFactory);


Can we move away from useExtensions for the old extension and make use of useResolvedExtensions?
Then combine the results two variables: one for the resolved state and one for an array of factories.

This way it'll be two code paths for setup but consumption will be simple.

christianvogt · 2021-05-31T17:17:21Z

frontend/packages/topology/src/components/graph-view/Topology.tsx

+        .catch(() => {});
+    }
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [componentFactoriesPromises, isFactoryComponentsResolved]);

  React.useEffect(() => {
    if (componentFactoriesPromises.length && !componentFactories.length) {


Since you've separated the static and dynamic extensions into different variables, this check is no longer valid as it only applies to one.

See other comment about using useResolvedExtensions for both static and dynamic plugins.

christianvogt · 2021-05-31T17:19:51Z

frontend/packages/topology/src/data-transforms/DataModelProvider.tsx

  const modelFactories = useExtensions<TopologyDataModelFactory>(isTopologyDataModelFactory);
+  const [dynamicModelFactories, isModelResolved] = useResolvedExtensions<
+    DynamicTopologyDataModelFactory
+  >(isDynamicTopologyDataModelFactory);


Can both paths use useResolvedExtensions so that we don't have one path doing manual resolution.

christianvogt · 2021-05-31T17:20:47Z

frontend/packages/topology/src/components/dropdowns/ApplicationDropdown.tsx

  const modelFactories = useExtensions<TopologyDataModelFactory>(isTopologyDataModelFactory);
+  const [dynamicModelFactories] = useResolvedExtensions<DynamicTopologyDataModelFactory>(
+    isDynamicTopologyDataModelFactory,
+  );


Can we update the static extension to make use of CodeRef then simplified the resolution with useResolvedExtensions

We probably don't want to render the firehose component until all extensions have been resolved.

glekner · 2021-06-01T08:47:30Z

@christianvogt modified extensions, now consuming all with useResolvedExtensions

christianvogt

Operator backed services aren't showing up correctly. Use the dev catalog
Before:

After:

There's also flashing of the old value when switching from one namespace to another.

christianvogt · 2021-06-01T21:32:40Z

frontend/packages/helm-plugin/src/topology/helmFilters.ts

@@ -35,3 +36,14 @@ export const applyHelmDisplayOptions = (model: Model, filters: DisplayFilters):
 };

 export const applyDisplayOptions = () => applyHelmDisplayOptions;
+
+export const getHelmWatchedResources = (namespace: string): WatchK8sResources<any> => {


Why was this moved to a filters module?

christianvogt · 2021-06-01T21:35:40Z

frontend/packages/rhoas-plugin/src/topology/createConnector.ts

@@ -19,3 +22,16 @@ export const getCreateConnector = (createHints: string[], source: Node, target:
  }
  return null;
 };
+
+export const getRhoasWatchedResources = (


Moving to createConnector doesn't seem like a logical home.

christianvogt · 2021-06-01T21:39:02Z

frontend/packages/topology/src/operators/operatorFilters.ts

@@ -35,3 +39,25 @@ export const applyOperatorDisplayOptions = (model: Model, filters: DisplayFilter
 };

 export const applyDisplayOptions = () => applyOperatorDisplayOptions;
+
+export const getOperatorWatchedResources = (namespace: string): WatchK8sResources<any> => {


Same comment about location.

christianvogt · 2021-06-01T21:50:32Z

Display filters are missing:

Before:

After:

christianvogt · 2021-06-01T21:52:14Z

Knative service, helm chart and channel:

Before:

After (channel doesn't show up):

christianvogt · 2021-06-02T15:53:39Z

@glekner the latest changes fixed the missing visuals.

One outstanding issue remains. Flashing of the old value when switching from one namespace to another.
In the following gif, the visual of the operator backed service disappears when we change namespaces (expected), then then reappears (unexpected) before the proper topology for the new namespace is displayed.

glekner · 2021-06-06T08:32:28Z

hey again,
the flashing is fixed by reverting ModelFactory to use useExtensions.
since you can’t pass a function without a CodeRef for the dynamic extensions, instead we are passing the resources that need to be watched, and we build a function for them in the consuming of the extensions. please review @christianvogt

glekner · 2021-06-06T09:43:28Z

/test e2e-gcp-console

glekner · 2021-06-07T13:24:03Z

/retest

rohitkrai03 · 2021-06-09T11:10:38Z

/cc @rohitkrai03

glekner · 2021-06-09T12:25:36Z

/retest

rohitkrai03 · 2021-06-09T11:27:51Z

frontend/packages/console-dynamic-plugin-sdk/src/extensions/topology.ts

@@ -88,7 +88,7 @@ export const isTopologyCreateConnector = (e: Extension): e is TopologyCreateConn
 export const isTopologyDataModelFactory = (e: Extension): e is TopologyDataModelFactory =>
  e.type === 'console.topology/data/factory';

-export const isTopologyDisplayFilters = (e: Extension): e is TopologyDisplayFilters =>
+export const isTopologyDisplayFilter = (e: Extension): e is TopologyDisplayFilters =>


Why this change?

to match the static extension

Okay, I guess it was a typo in the static extensions. But we can keep it like this for now.

I suggest changing the old static one instead of the new. That being said we will end up revisiting these extensiioons.

rohitkrai03 · 2021-06-09T12:40:32Z

frontend/packages/topology/src/components/dropdowns/ApplicationDropdown.tsx

@@ -56,7 +69,9 @@ const ApplicationDropdown: React.FC<ApplicationDropdownProps> = ({ namespace, ..
      ...watchedBaseResources[key],
      prop: key,
    }));
-  }, [modelFactories, namespace]);
+
+    return [];


Is this needed? Its unreachable.

thanks, removed

rohitkrai03 · 2021-06-09T12:42:01Z

frontend/packages/console-dynamic-plugin-sdk/src/extensions/topology.ts

@@ -39,7 +39,7 @@ export type TopologyDataModelFactory = ExtensionDeclaration<
    /** Priority for the factory */
    priority: number;
    /** Resources to be fetched from useK8sWatchResources hook. */
-    resources?: CodeRef<(namespace: string) => WatchK8sResources<any>>;
+    resources?: WatchK8sResources<any>;


Do we not need namespaced resources anymore?

since you cant set a function without a CodeRef, i'm building the namespaced resources here
https://github.com/openshift/console/pull/9055/files#diff-281f5462e39247e7a7894ee5ad688563cf18b46d97e7707da0b660b79c236f41R17

So why not make it a coderef?

I talked to @glekner about this too. I don't see an issue with this change unless the extension wants to hard code a namespace. In which case we can make sure to not set the namespace if one is already provided.

I don't think we have such a use case at this time that will be negatively affected by this.

Note that we still need to review the topology extensions for their exposed API and at that time may change things.

glekner · 2021-06-13T07:57:24Z

/retest

glekner · 2021-06-14T14:49:09Z

/test e2e-gcp-console

glekner · 2021-06-16T10:40:24Z

/test frontend

glekner · 2021-06-16T13:02:49Z

/test e2e-gcp-console

glekner · 2021-06-16T21:20:21Z

/retest

glekner · 2021-06-17T12:17:35Z

/test kubevirt-plugin

glekner · 2021-06-17T13:36:12Z

/test kubevirt-plugin

glekner · 2021-06-18T09:41:46Z

/retest

christianvogt · 2021-06-18T16:02:43Z

frontend/packages/console-dynamic-plugin-sdk/src/extensions/topology.ts

@@ -88,7 +88,7 @@ export const isTopologyCreateConnector = (e: Extension): e is TopologyCreateConn
 export const isTopologyDataModelFactory = (e: Extension): e is TopologyDataModelFactory =>
  e.type === 'console.topology/data/factory';

-export const isTopologyDisplayFilters = (e: Extension): e is TopologyDisplayFilters =>
+export const isTopologyDisplayFilter = (e: Extension): e is TopologyDisplayFilters =>


I suggest changing the old static one instead of the new. That being said we will end up revisiting these extensiioons.

christianvogt · 2021-06-18T17:54:58Z

frontend/packages/topology/src/data-transforms/DataModelProvider.tsx

+        Object.assign(
+          {},
+          ...Object.entries(properties.resources).map(([k, v]) => ({
+            [k]: { ...v, namespace: ns },


We should probably accept the namespace provided before overriding? Not sure we have a use case for this yet but if an extension wanted to pull resources from a specific namespace we'd have to do:

Suggested change

[k]: { ...v, namespace: ns },

[k]: { namespace: ns, ...v },

rohitkrai03 · 2021-06-21T13:47:08Z

/assign @rohitkrai03

rohitkrai03

/lgtm
/approve

openshift-ci · 2021-06-21T14:30:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, 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/packages/console-dynamic-plugin-sdk/OWNERS~~ [rohitkrai03]
~~frontend/packages/helm-plugin/OWNERS~~ [rohitkrai03]
~~frontend/packages/knative-plugin/OWNERS~~ [rohitkrai03]
~~frontend/packages/kubevirt-plugin/OWNERS~~ [glekner,rohitkrai03]
~~frontend/packages/pipelines-plugin/OWNERS~~ [rohitkrai03]
~~frontend/packages/rhoas-plugin/OWNERS~~ [rohitkrai03]
~~frontend/packages/topology/OWNERS~~ [rohitkrai03]

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

openshift-ci bot requested review from jerolimov and vojtechszocs May 26, 2021 14:31

openshift-ci bot added component/sdk Related to console-plugin-sdk component/topology Related to topology labels May 26, 2021

glekner force-pushed the dp-sdk-topology branch from ec9a2b0 to da205e3 Compare May 27, 2021 09:14

glekner force-pushed the dp-sdk-topology branch 2 times, most recently from 9ea76ea to 29a39e2 Compare May 31, 2021 08:59

christianvogt reviewed May 31, 2021

View reviewed changes

glekner force-pushed the dp-sdk-topology branch from 29a39e2 to c141866 Compare June 1, 2021 08:46

openshift-ci bot added component/helm Related to helm-plugin component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/pipelines Related to pipelines-plugin labels Jun 1, 2021

christianvogt reviewed Jun 1, 2021

View reviewed changes

glekner force-pushed the dp-sdk-topology branch from c141866 to 256f5d6 Compare June 2, 2021 12:52

glekner force-pushed the dp-sdk-topology branch from 256f5d6 to fd40642 Compare June 6, 2021 08:28

glekner force-pushed the dp-sdk-topology branch from fd40642 to 7adb32c Compare June 6, 2021 08:34

glekner force-pushed the dp-sdk-topology branch from 7adb32c to 385bc1a Compare June 9, 2021 09:54

openshift-ci bot requested a review from rohitkrai03 June 9, 2021 11:10

rohitkrai03 suggested changes Jun 9, 2021

View reviewed changes

glekner force-pushed the dp-sdk-topology branch 2 times, most recently from 827c8f3 to 70a6fb9 Compare June 11, 2021 21:09

glekner force-pushed the dp-sdk-topology branch from 70a6fb9 to 21860b5 Compare June 14, 2021 13:30

glekner force-pushed the dp-sdk-topology branch 2 times, most recently from 5fafe6b to 6f5f9f9 Compare June 16, 2021 10:12

glekner force-pushed the dp-sdk-topology branch from 6f5f9f9 to deda27c Compare June 16, 2021 11:46

glekner force-pushed the dp-sdk-topology branch from deda27c to aa71d8e Compare June 17, 2021 09:41

glekner force-pushed the dp-sdk-topology branch from aa71d8e to cc9913f Compare June 17, 2021 18:45

christianvogt reviewed Jun 18, 2021

View reviewed changes

add dynamic sdk support for topology extensions

3f19e78

glekner force-pushed the dp-sdk-topology branch from cc9913f to 3f19e78 Compare June 20, 2021 09:48

openshift-ci bot assigned rohitkrai03 Jun 21, 2021

rohitkrai03 approved these changes Jun 21, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2021

openshift-merge-robot merged commit b3606c9 into openshift:master Jun 21, 2021

spadgett added this to the v4.9 milestone Jul 14, 2021

add dynamic sdk support for topology extensions #9055

add dynamic sdk support for topology extensions #9055

Conversation

glekner commented May 26, 2021 • edited Loading

glekner commented May 27, 2021

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

glekner commented Jun 1, 2021

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

christianvogt commented Jun 1, 2021

christianvogt commented Jun 1, 2021

christianvogt commented Jun 2, 2021

glekner commented Jun 6, 2021

glekner commented Jun 6, 2021

glekner commented Jun 7, 2021

rohitkrai03 commented Jun 9, 2021

glekner commented Jun 9, 2021

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

glekner Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glekner commented Jun 13, 2021

glekner commented Jun 14, 2021

glekner commented Jun 16, 2021

glekner commented Jun 16, 2021

glekner commented Jun 16, 2021

glekner commented Jun 17, 2021

glekner commented Jun 17, 2021

glekner commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitkrai03 commented Jun 21, 2021

rohitkrai03 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 21, 2021

glekner commented May 26, 2021 •

edited

Loading

glekner Jun 9, 2021 •

edited

Loading