Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Managed by section in resource tab for operator backed services #3943

Conversation

nemesis09
Copy link
Contributor

This PR-

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 14, 2020
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin component/shared Related to console-shared labels Jan 14, 2020
@nemesis09 nemesis09 force-pushed the identify-operator-backed-sidebar branch from 10e93dc to 96c1761 Compare January 14, 2020 16:41
@andrewballantyne
Copy link
Contributor

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 15, 2020
@nemesis09 nemesis09 changed the title [WIP] add Managed by section in resource tab for operator backed services add Managed by section in resource tab for operator backed services Jan 16, 2020
@openshift-ci-robot 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 16, 2020
@nemesis09
Copy link
Contributor Author

putting on hold as UXD is yet to be confirmed.
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2020
@nemesis09
Copy link
Contributor Author

removing hold as we are going with the presently available UXD option for the time being.
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2020
@nemesis09
Copy link
Contributor Author

/retest

@@ -45,6 +46,18 @@ const getSidebarResources = ({ obj, ksroutes, revisions, configurations }: Overv
};
const OverviewDetailsKnativeResourcesTab: React.FC<OverviewDetailsResourcesTabProps> = ({
item,
}) => <div className="overview__sidebar-pane-body"> {getSidebarResources(item)} </div>;
}) => (
<div className="overview__sidebar-pane-body">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the same code repeated twice. Make this a pure component and share it in both places.

@@ -34,6 +40,14 @@ export const OverviewDetailsResourcesTab: React.SFC<OverviewDetailsResourcesTabP
const pluginComponents = getPluginTabSectionResource(item);
return (
<div className="overview__sidebar-pane-body">
{item.isOperatorBackedService && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use shared component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL

<span className="sidebar__section-owner-operator-heading">
Managed by operator:
<span className="sidebar__section-owner-reference-operator">
<OwnerReferences resource={item.obj} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like owner references can be an array. Does this mean that every owner ref should be listed or only the one that is backed by an operator?
Reusing this component is probably ok for now but we should ok investigate.

Copy link
Contributor Author

@nemesis09 nemesis09 Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case of multiple owners, we could have a check for the operator backed owner.
I'm not sure if there can be multiple operator backed owners.

@nemesis09 nemesis09 force-pushed the identify-operator-backed-sidebar branch 3 times, most recently from eb0dc03 to 8e0b72a Compare January 21, 2020 01:59
import { OverviewItem } from '@console/shared';
import { OwnerReferences } from './owner-references';

export const OperatorBackedOwnerReferences: React.FC<OperatorBackedOwnerReferencesProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer single default exports for component files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the component to export default OperatorBackedOwnerReferences.
PTAL

@jeff-phillips-18
Copy link
Member

@openshift/team-devconsole-ux

@nemesis09 nemesis09 force-pushed the identify-operator-backed-sidebar branch 4 times, most recently from addc645 to 5b92772 Compare January 22, 2020 14:01
@nemesis09
Copy link
Contributor Author

/retest

@nemesis09 nemesis09 force-pushed the identify-operator-backed-sidebar branch from 5b92772 to 22e6cba Compare January 22, 2020 15:52
@nemesis09
Copy link
Contributor Author

/test images
/test e2e-gcp-console

Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2020
@christianvogt
Copy link
Contributor

The data in topology and list view differs.
I suppose we could fix the list view as a followup bug.

Topology:
image

List view:
image

@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2020
@nemesis09 nemesis09 force-pushed the identify-operator-backed-sidebar branch from 22e6cba to 5586c2b Compare January 24, 2020 03:17
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020
@karthikjeeyar
Copy link
Contributor

/test images

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020
@nemesis09 nemesis09 force-pushed the identify-operator-backed-sidebar branch from 5586c2b to fa97c2b Compare January 24, 2020 05:51
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020
@karthikjeeyar
Copy link
Contributor

Topology · OKD (11)

Knative Services are not considered as operator backed (sidepanel does not have managed by: knative serving operator), we need to show the knative opertaor Icon. we can have a followup bug for this and displaying operator icon.

cc: @christianvogt @nemesis09

@karthikjeeyar
Copy link
Contributor

Verified locally
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, karthikjeeyar, nemesis09, serenamarie125

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:

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

@nemesis09
Copy link
Contributor Author

Topology · OKD (11)

Knative Services are not considered as operator backed (sidepanel does not have managed by: knative serving operator), we need to show the knative opertaor Icon. we can have a followup bug for this and displaying operator icon.

cc: @christianvogt @nemesis09

That would require the isOperatorBackedService flag to be set to true in topology. owner reference for knative operator is also absent. these changes can be made as a part of the bug.

@openshift-merge-robot openshift-merge-robot merged commit a7a1b57 into openshift:master Jan 24, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin component/shared Related to console-shared kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants