Bug 1921762: Fix knative serving and eventing breadcrumbs #7885

debsmita1 · 2021-01-20T14:14:23Z

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

Solution description:

using the hook useTabbedTableBreadcrumbsFor to fix the breadcrumb path
fixed the issue with the Triggers list showing an error when subscriber.ref is empty
using extension (created in PR add plugin extension for details page breadcrumbs debsmita1/console#6) for Event Sources, Channels & Brokers

GIF & Screenshot:

when no subscriber is associated with Trigger

Browser conformance:

Chrome
Firefox
Safari
Edge

debsmita1 · 2021-01-20T14:19:21Z

/cc @invincibleJai
/cc @sahil143
/assign @christianvogt

sahil143

@debsmita1 This implementation will only work for the static model. In case of dynamic models eventsources/channels this will fail. I created plugin extension to handle the breadcrumbs for detail page which covers the dynamic event sources as well. I raised a PR against your PR debsmita1#6.

Plugin Extension:

namespace ExtensionProperties {
  export interface DetailPageBreadCrumbs {
    /**
     * array of models(kindObj) against which bread crumb is formed
     */
    getModels: () => K8sKind[] | K8sKind;
    /**
     * returns breadcrumb for the given kindref
     */
    useBreadcrumbs: (kind: string, urlMatch: match<any>) => any;
  }
}

export interface DetailPageBreadCrumbs
  extends Extension<ExtensionProperties.DetailPageBreadCrumbs> {
  type: 'DetailPageBreadCrumbs';
}

export const isDetailPageBreadCrumbs = (e: Extension): e is DetailPageBreadCrumbs => {
  return e.type === 'DetailPageBreadCrumbs';
};

cc @christianvogt @invincibleJai Please take a look.

jerolimov · 2021-01-22T10:56:44Z

/retest

invincibleJai · 2021-01-25T17:23:33Z

/retest

invincibleJai · 2021-01-25T17:34:53Z

I verified the PR , it works as expected however Ci jobs are failing. tried retest

invincibleJai · 2021-01-25T17:39:23Z

frontend/packages/knative-plugin/src/hooks/useEventSourceDetailPageBreadcrumbs.ts

+import { K8sKind } from '@console/internal/module/k8s';
+import { useActivePerspective } from '@console/shared/src/hooks/useActivePerspective';
+import { serverlessTab } from '../utils/serverless-tab-utils';
+import { useTabbedTableBreadcrumbsFor } from '@console/shared';


nit: sort imports

invincibleJai · 2021-01-25T17:43:28Z

frontend/public/components/factory/details.tsx

@@ -49,53 +81,70 @@ export const DetailsPage = withFallback<DetailsPageProps>(({ pages = [], ...prop
        })),
    [resourcePageExtensions, props],
  );
-
+  const kindObj = props.kindObj ? props.kindObj : modelFor(props.kind);


can we have

Suggested change

const kindObj = props.kindObj ? props.kindObj : modelFor(props.kind);

const kindObj = props.kindObj ?? modelFor(props.kind);

debsmita1 · 2021-01-27T09:47:39Z

/kind bug

invincibleJai · 2021-01-27T15:37:58Z

/lgtm

Thanks , verified works as expected!!

openshift-ci-robot · 2021-01-28T15:39:17Z

@debsmita1: This pull request references Bugzilla bug 1921762, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

karthikjeeyar · 2021-01-29T09:59:09Z

Works as expected.
/lgtm

karthikjeeyar · 2021-01-29T09:59:27Z

/retest

spadgett · 2021-01-29T17:14:29Z

/override ci/prow/ceph-storage-plugin

The ceph-storage-plugin job should only have run on PRs with changes to frontend/packages/ceph-storage-plugin. It failed due to an unrelated CI config change. Overriding status on this PR.

openshift-ci-robot · 2021-01-29T17:18:50Z

@spadgett: Overrode contexts on behalf of spadgett: ci/prow/ceph-storage-plugin

In response to this:

/override ci/prow/ceph-storage-plugin

The ceph-storage-plugin job should only have run on PRs with changes to frontend/packages/ceph-storage-plugin. It failed due to an unrelated CI config change. Overriding status on this PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rohitkrai03 · 2021-02-01T16:01:46Z

frontend/public/components/factory/details.tsx

+  const kindObj = props.kindObj ?? modelFor(props.kind);
+  const resolvedBreadcrumbExtension = useBreadCrumbsForDetailPage(kindObj);
+  const onBreadcrumbsResolved = React.useCallback((breadcrumbs) => {
+    setPluginBreadcrumbs(breadcrumbs ? () => breadcrumbs : undefined);


Why are you setting pluginBreadcrumbs with () => breadcrumbs when you're sending it to breadcrumbs prop of PageHeading? The type of breadcrumbs prop is { name: string; path: string }[];

rohitkrai03 · 2021-02-01T16:06:12Z

frontend/packages/knative-plugin/src/hooks/index.ts

-export * from './useEventSourceStatus';
+import { getExecutableCodeRef } from '@console/dynamic-plugin-sdk/src/coderefs/coderef-utils';
+
+export const eventSourceBreadcrumbsProvider = getExecutableCodeRef(() =>


i don't think it's a good idea to replace index.ts with coderef hook exports here. This can be confusing as general imports would expect to work from /hooks directly which will not happen. Also this leads to changing older imports from the hooks folder which is not right. Better to create a new file or something that has all coderefs like these and then import from that.

moved these to src/providers

rohitkrai03 · 2021-02-01T16:07:31Z

frontend/packages/knative-plugin/src/catalog/useEventSourceProvider.ts

@@ -5,7 +5,7 @@ import { CatalogItem, CatalogExtensionHook } from '@console/plugin-sdk';
 import { K8sKind, referenceForModel } from '@console/internal/module/k8s';
 import { getEventSourceIcon } from '../utils/get-knative-icon';
 import { getEventSourceCatalogProviderData } from './event-source-data';
-import { useEventSourceModelsWithAccess } from '../hooks';
+import { useEventSourceModelsWithAccess } from '../hooks/useEventSourceModelsWithAccess';


Do not change older imports. See my comment above.

rohitkrai03 · 2021-02-01T16:07:41Z

frontend/packages/knative-plugin/src/components/add/EventSourcePage.tsx

@@ -11,7 +11,7 @@ import { QUERY_PROPERTIES } from '@console/dev-console/src/const';
 import ConnectedEventSource from './EventSource';
 import EventSourceAlert from './EventSourceAlert';
 import { CamelKameletBindingModel } from '../../models';
-import { useEventSourceStatus } from '../../hooks';
+import { useEventSourceStatus } from '../../hooks/useEventSourceStatus';


Same as above.

rohitkrai03 · 2021-02-01T16:09:14Z

frontend/packages/knative-plugin/src/components/pub-sub/details/SubscriptionDetailsPage.tsx

  const pages = [navFactory.details(SubscriptionDetails), navFactory.editYaml()];
  const commonActions = Kebab.factory.common.map((action) => action);
  const menuActionsCreator = [
    ...Kebab.getExtensionsActionsForKind(EventingSubscriptionModel),
    ...commonActions,
  ];
+  const breadcrumbsFor = useTabbedTableBreadcrumbsFor(


Suggested change

const breadcrumbsFor = useTabbedTableBreadcrumbsFor(

const breadcrumbs = useTabbedTableBreadcrumbsFor(

rohitkrai03 · 2021-02-01T16:09:28Z

frontend/packages/knative-plugin/src/components/pub-sub/details/SubscriptionDetailsPage.tsx

+  return (
+    <DetailsPage
+      {...props}
+      breadcrumbsFor={() => breadcrumbsFor}


Suggested change

breadcrumbsFor={() => breadcrumbsFor}

breadcrumbsFor={() => breadcrumbs}

rohitkrai03 · 2021-02-01T16:09:56Z

frontend/packages/knative-plugin/src/components/pub-sub/details/TriggerDetailsPage.tsx


 const TriggerDetailsPage: React.FC<React.ComponentProps<typeof DetailsPage>> = (props) => {
+  const { kindObj, match } = props;
+  const breadcrumbsFor = useTabbedTableBreadcrumbsFor(


Same as above. Similar suggestion for all such usages.

sahil143

@debsmita1 everything looks good. just one nit.
verified locally as well. works as expected.

sahil143 · 2021-02-02T05:49:01Z

frontend/public/components/factory/details.tsx

+          breadcrumbsFor={
+            props.breadcrumbsFor
+              ? props.breadcrumbsFor
+              : !pluginBreadcrumbs
+              ? breadcrumbsForDetailsPage(props.kindObj, props.match)
+              : undefined
+          }


nit:

Suggested change

breadcrumbsFor={

props.breadcrumbsFor

? props.breadcrumbsFor

: !pluginBreadcrumbs

? breadcrumbsForDetailsPage(props.kindObj, props.match)

: undefined

}

breadcrumbsFor={

props.breadcrumbsFor

?? (!pluginBreadcrumbs

? breadcrumbsForDetailsPage(props.kindObj, props.match)

: undefined)

}

sahil143

/lgtm

rohitkrai03

/approve

openshift-ci-robot · 2021-02-02T11:12:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: debsmita1, invincibleJai, karthikjeeyar, rohitkrai03, 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~~ [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-robot · 2021-02-02T11:56:58Z

@debsmita1: All pull requests linked via external trackers have merged:

openshift/console#7885

Bugzilla bug 1921762 has been moved to the MODIFIED state.

In response to this:

Bug 1921762: Fix knative serving and eventing breadcrumbs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the component/knative Related to knative-plugin label Jan 20, 2021

openshift-ci-robot requested review from rottencandy and vikram-raj January 20, 2021 14:14

openshift-ci-robot assigned christianvogt Jan 20, 2021

openshift-ci-robot requested review from invincibleJai and sahil143 January 20, 2021 14:19

sahil143 reviewed Jan 20, 2021

View reviewed changes

openshift-ci-robot added component/core Related to console core functionality component/sdk Related to console-plugin-sdk labels Jan 21, 2021

debsmita1 changed the title ~~fix knative serving and eventing breadcrumbs~~ WIP: fix knative serving and eventing breadcrumbs Jan 22, 2021

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2021

debsmita1 force-pushed the knative-breadcrumbs branch from ce42865 to 65f7e74 Compare January 22, 2021 17:37

openshift-ci-robot added the component/shared Related to console-shared label Jan 22, 2021

debsmita1 force-pushed the knative-breadcrumbs branch from 65f7e74 to 86e7d3c Compare January 22, 2021 17:51

debsmita1 changed the title ~~WIP: fix knative serving and eventing breadcrumbs~~ Fix knative serving and eventing breadcrumbs Jan 22, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2021

debsmita1 requested a review from sahil143 January 22, 2021 17:54

debsmita1 force-pushed the knative-breadcrumbs branch from 86e7d3c to 17bccd9 Compare January 25, 2021 13:59

invincibleJai reviewed Jan 25, 2021

View reviewed changes

debsmita1 force-pushed the knative-breadcrumbs branch from 17bccd9 to 2d38e6d Compare January 27, 2021 09:41

debsmita1 requested a review from invincibleJai January 27, 2021 09:42

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 27, 2021

openshift-ci-robot assigned invincibleJai Jan 27, 2021

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

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Jan 28, 2021

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 28, 2021

openshift-ci-robot assigned karthikjeeyar Jan 29, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2021

rohitkrai03 suggested changes Feb 1, 2021

View reviewed changes

debsmita1 force-pushed the knative-breadcrumbs branch from b3dfcba to 26fc08f Compare February 1, 2021 17:44

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2021

debsmita1 requested a review from rohitkrai03 February 1, 2021 17:57

sahil143 reviewed Feb 2, 2021

View reviewed changes

debsmita1 and others added 3 commits February 2, 2021 13:03

fix knative serving and eventing breadcrumbs

70ccf37

update breadcrumbs with plugin extension

c26653c

update breadcrumbs for channel details page

fc3b512

debsmita1 force-pushed the knative-breadcrumbs branch from 26fc08f to fc3b512 Compare February 2, 2021 10:03

debsmita1 requested a review from sahil143 February 2, 2021 10:03

sahil143 approved these changes Feb 2, 2021

View reviewed changes

openshift-ci-robot assigned sahil143 Feb 2, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2021

rohitkrai03 approved these changes Feb 2, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2021

openshift-merge-robot merged commit 8cc6f80 into openshift:master Feb 2, 2021

spadgett added this to the v4.7 milestone Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1921762: Fix knative serving and eventing breadcrumbs #7885

Bug 1921762: Fix knative serving and eventing breadcrumbs #7885

debsmita1 commented Jan 20, 2021 •

edited

debsmita1 commented Jan 20, 2021

sahil143 left a comment

jerolimov commented Jan 22, 2021

invincibleJai commented Jan 25, 2021

invincibleJai commented Jan 25, 2021 •

edited

invincibleJai Jan 25, 2021

invincibleJai Jan 25, 2021

debsmita1 commented Jan 27, 2021

invincibleJai commented Jan 27, 2021

openshift-ci-robot commented Jan 28, 2021

karthikjeeyar commented Jan 29, 2021

karthikjeeyar commented Jan 29, 2021

spadgett commented Jan 29, 2021

openshift-ci-robot commented Jan 29, 2021

rohitkrai03 Feb 1, 2021

debsmita1 Feb 1, 2021

rohitkrai03 Feb 1, 2021

debsmita1 Feb 1, 2021

rohitkrai03 Feb 1, 2021

rohitkrai03 Feb 1, 2021

rohitkrai03 Feb 1, 2021

rohitkrai03 Feb 1, 2021

rohitkrai03 Feb 1, 2021

sahil143 left a comment

sahil143 Feb 2, 2021

sahil143 left a comment

rohitkrai03 left a comment

openshift-ci-robot commented Feb 2, 2021

openshift-ci-robot commented Feb 2, 2021

	const kindObj = props.kindObj ? props.kindObj : modelFor(props.kind);
	const kindObj = props.kindObj ?? modelFor(props.kind);

	const breadcrumbsFor = useTabbedTableBreadcrumbsFor(
	const breadcrumbs = useTabbedTableBreadcrumbsFor(

	breadcrumbsFor={() => breadcrumbsFor}
	breadcrumbsFor={() => breadcrumbs}

Bug 1921762: Fix knative serving and eventing breadcrumbs #7885

Bug 1921762: Fix knative serving and eventing breadcrumbs #7885

Conversation

debsmita1 commented Jan 20, 2021 • edited

debsmita1 commented Jan 20, 2021

sahil143 left a comment

Choose a reason for hiding this comment

jerolimov commented Jan 22, 2021

invincibleJai commented Jan 25, 2021

invincibleJai commented Jan 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debsmita1 commented Jan 27, 2021

invincibleJai commented Jan 27, 2021

openshift-ci-robot commented Jan 28, 2021

karthikjeeyar commented Jan 29, 2021

karthikjeeyar commented Jan 29, 2021

spadgett commented Jan 29, 2021

openshift-ci-robot commented Jan 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil143 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil143 left a comment

Choose a reason for hiding this comment

rohitkrai03 left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 2, 2021

openshift-ci-robot commented Feb 2, 2021

debsmita1 commented Jan 20, 2021 •

edited

invincibleJai commented Jan 25, 2021 •

edited