add support for Operator Backed SBR through topology inContext #7084

sahil143 · 2020-11-03T15:41:59Z

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

Analysis / Root cause: SBR for Operator Backed through incontext was missing

Solution Description: added support for Operator Backed SBR through topology inContext

Screen shots / Gifs for design review:

Unit test coverage report:

Test setup:

Browser conformance:

Chrome
Firefox
Safari
Edge

jeff-phillips-18

At some point the options for resource menu should come from the plugin as well rather than hardcoding the check and adding the operator backed option.

jeff-phillips-18 · 2020-11-04T12:06:24Z

frontend/packages/dev-console/src/plugin.tsx

+      type: CONNECTOR_INCONTEXT_ACTIONS.serviceBinding,
+      callback: doContextualBinding,
+    },
+  },


I'd rather this be in operatorsTopologyPlugin to keep it with other service binding extension support (eventually moved out of dev-console and into another mono repo).
Should this be dependent on the ALLOW_SERVICE_BINDING flag?

This is just a callback. the actual action is dispatched from the context menu through URL params. Not sure if we should add the flag here.

I agree with what Jeff said here.
This action shouldn't be available if there's no service binding installed.
Also move to the operatorsTopologyPlugin which is where service binding support is currently.

jeff-phillips-18 · 2020-11-04T12:16:19Z

frontend/public/components/catalog/catalog-page.tsx

+  withQueryParams<CatalogListPageProps>(
+    class CatalogListPage extends React.Component<CatalogListPageProps, CatalogListPageState> {
+      constructor(props: CatalogListPageProps) {
+        super(props);


Would be better just to convert this to a Functional Component.

Catalog page is already getting a big refactor in other epic. So didn't wanted to make any big changes here.

christianvogt · 2020-11-05T20:10:31Z

frontend/packages/console-shared/src/hooks/useQueryParams.tsx

+export const withQueryParams = <Props extends {} = {}>(
+  Component: React.ComponentType<Props>,
+): React.FC<Props> => (props: Props) => {
+  const queryParams = useQueryParams();
+  return <Component {...props} queryParams={queryParams} />;
+};


Suggested change

export const withQueryParams = <Props extends {} = {}>(

Component: React.ComponentType<Props>,

): React.FC<Props> => (props: Props) => {

const queryParams = useQueryParams();

return <Component {...props} queryParams={queryParams} />;

};

export type WithQueryParamsProps = {

queryParams: URLSearchParams;

};

export const withQueryParams = <P extends WithQueryParamsProps>(

Component: React.ComponentType<P>,

): React.FC<Omit<P, keyof WithQueryParamsProps>> => (props) => {

const queryParams = useQueryParams();

return <Component {...(props as P)} queryParams={queryParams} />;

};

christianvogt · 2020-11-05T20:11:38Z

frontend/packages/console-plugin-sdk/src/typings/dev-catalog.ts

@@ -4,7 +4,7 @@ import { Extension } from './base';
 namespace ExtensionProperties {
  export interface DevCatalogModel {
    model: K8sKind;
-    normalize: (data: K8sResourceKind[]) => K8sResourceKind[];
+    normalize: (data: K8sResourceKind[], params?: URLSearchParams) => K8sResourceKind[];


I think we're better off just forcing the param onto the query string for them when we create the link.

christianvogt · 2020-11-05T20:34:05Z

frontend/packages/dev-console/src/const.ts

 export enum CONNECTOR_INCONTEXT_ACTIONS {
  /** connects to action for resources */
  connectsTo = 'connectsTo',
+  /** connector action for service binding */
+  serviceBinding = 'serviceBinding',


Better to have these as separate const strings and not an enum as the values aren't being used as an enum with a finite set of types that are allowed.
Furthermore, we will separate service binding out of dev console.

christianvogt · 2020-11-05T20:35:30Z

frontend/packages/dev-console/src/plugin.tsx

+      type: CONNECTOR_INCONTEXT_ACTIONS.serviceBinding,
+      callback: doContextualBinding,
+    },
+  },


I agree with what Jeff said here.
This action shouldn't be available if there's no service binding installed.
Also move to the operatorsTopologyPlugin which is where service binding support is currently.

christianvogt · 2020-11-05T21:29:52Z

frontend/packages/dev-console/src/utils/useAddToProjectAccess.ts

+  const serviceBindingEnabled = useSelector((state: RootState) =>
+    state.FLAGS.get(ALLOW_SERVICE_BINDING),
+  );


I'm surprised we don't have a useFeatureFlag hook. We should create one inside features.ts

christianvogt · 2020-11-05T21:33:34Z

frontend/packages/dev-console/src/utils/useAddToProjectAccess.ts

+  const serviceBindingEnabled = useSelector((state: RootState) =>
+    state.FLAGS.get(ALLOW_SERVICE_BINDING),
+  );
+
  React.useEffect(() => {


This actually shouldn't be an effect. There's no reason for this hook to use useState.
createResourceAccess can easily be derived inside the hook function and at most we can replace useEffect with useMemo which returns createResourceAccess instead of setting state.

christianvogt · 2020-11-05T21:44:43Z

frontend/packages/operator-lifecycle-manager/src/plugin.tsx

-  {
-    type: 'Page/Route',
-    properties: {
-      exact: true,
-      path: `/k8s/ns/:ns/${models.ClusterServiceVersionModel.plural}/:appName/:plural/~new`,
-      loader: async () =>
-        (
-          await import(
-            './components/operand/create-operand' /* webpackChunkName: "create-operand" */
-          )
-        ).CreateOperandPage,
-    },
-  },


I don't think you can remove this route as it seems there are other references in the code.

This is a duplicate route.

Ah i see where the other is now :)

christianvogt · 2020-11-09T21:55:16Z

frontend/packages/operator-lifecycle-manager/src/components/operand/DEPRECATED_operand-form.tsx

+        .then((res) => {
+          postFormCallback(res);
+          return history.push(next);
+        })


Any reason to not wait for the form actions to complete first?

Suggested change

.then((res) => {

postFormCallback(res);

return history.push(next);

})

.then((res) => postFormCallback(res))

.then(() => history.push(next))

christianvogt · 2020-11-09T21:56:17Z

frontend/packages/operator-lifecycle-manager/src/components/operand/operand-form.tsx

+      .then((res) => {
+        postFormCallback(res);
+        return next && history.push(next);
+      })


Suggested change

.then((res) => {

postFormCallback(res);

return next && history.push(next);

})

.then((res) => postFormCallback(res))

.then(() => next && history.push(next))

christianvogt · 2020-11-11T14:23:29Z

/approve

remove comment address review comments address review comments address review comments address review comments support service binding connector for yaml view fix unit tests

invincibleJai · 2020-11-12T15:30:25Z

Thanks @sahil143 , verified the changes works as expected

invincibleJai · 2020-11-12T15:30:33Z

/lgtm

openshift-ci-robot · 2020-11-12T15:30:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, sahil143

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/olm Related to OLM labels Nov 3, 2020

openshift-ci-robot requested review from jeff-phillips-18 and rottencandy November 3, 2020 15:42

openshift-ci-robot added component/sdk Related to console-plugin-sdk component/shared Related to console-shared labels Nov 3, 2020

sahil143 force-pushed the odc-5051 branch 2 times, most recently from 7787d84 to fcf18f9 Compare November 4, 2020 10:19

jeff-phillips-18 reviewed Nov 4, 2020

View reviewed changes

christianvogt reviewed Nov 5, 2020

View reviewed changes

sahil143 force-pushed the odc-5051 branch from fcf18f9 to 69bcaee Compare November 9, 2020 10:35

christianvogt reviewed Nov 9, 2020

View reviewed changes

sahil143 force-pushed the odc-5051 branch 2 times, most recently from 00a733d to 4b83e32 Compare November 11, 2020 10:39

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

add support for Operator Backed SBR through topology inContext

d9449db

remove comment address review comments address review comments address review comments address review comments support service binding connector for yaml view fix unit tests

sahil143 force-pushed the odc-5051 branch from 4b83e32 to d9449db Compare November 11, 2020 14:49

openshift-ci-robot assigned invincibleJai Nov 12, 2020

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

openshift-merge-robot merged commit c6855c1 into openshift:master Nov 12, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for Operator Backed SBR through topology inContext #7084

add support for Operator Backed SBR through topology inContext #7084

sahil143 commented Nov 3, 2020 •

edited

jeff-phillips-18 left a comment

jeff-phillips-18 Nov 4, 2020

sahil143 Nov 5, 2020

christianvogt Nov 5, 2020

jeff-phillips-18 Nov 4, 2020

sahil143 Nov 5, 2020

christianvogt Nov 5, 2020

christianvogt Nov 5, 2020

christianvogt Nov 5, 2020

christianvogt Nov 5, 2020

christianvogt Nov 5, 2020

christianvogt Nov 5, 2020

christianvogt Nov 5, 2020

sahil143 Nov 6, 2020

christianvogt Nov 9, 2020

christianvogt Nov 9, 2020

christianvogt Nov 9, 2020

christianvogt commented Nov 11, 2020

invincibleJai commented Nov 12, 2020

invincibleJai commented Nov 12, 2020

openshift-ci-robot commented Nov 12, 2020

add support for Operator Backed SBR through topology inContext #7084

add support for Operator Backed SBR through topology inContext #7084

Conversation

sahil143 commented Nov 3, 2020 • edited

jeff-phillips-18 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Nov 11, 2020

invincibleJai commented Nov 12, 2020

invincibleJai commented Nov 12, 2020

openshift-ci-robot commented Nov 12, 2020

sahil143 commented Nov 3, 2020 •

edited