Add operators status to Dashboards #3755

rawagner · 2019-12-11T15:35:09Z

depends on #3754

andybraren

Looking awesome to me. 🎉

I notice that "InstallSucceeded" matches what appears in the Installed Operators List page, but the Details page for a given operator/CSV shows the status as "Succeeded" instead.

I'd probably expect/prefer to see "Succeeded" in this popover, but I'm not sure which one is correct. Is one coming from status.reason and the other from status.phase in the CSV? FYI @itsptk

andybraren · 2019-12-11T19:41:29Z

frontend/packages/console-app/src/components/dashboards-page/OperatorStatus.tsx

+}) => (
+  <>
+    <div className="co-operator-section">
+      Operators extend Kubernetes with additional custom resources to manage applications


Suggested change

Operators extend Kubernetes with additional custom resources to manage applications

Operators extend OpenShift with additional custom resources to manage applications.

Added a period and switched to "OpenShift". We might want to validate the language here with an SME.

itsptk

@dmesser @tlwu2013 I spoke with @alecmerdler and he mentioned we are transforming this CSV status in the Installed Operator details to show as just "Succeeded." Could that be appropriate here as well? This is a roll-up status of all the CSVs' statuses for an operator across namespaces, showing the "worst" status.

rawagner · 2019-12-12T09:05:56Z

Updated operator status to read from status.phase instead of status.reason

itsptk · 2019-12-12T13:54:28Z

Updated operator status to read from status.phase instead of status.reason

I think my inclination would be to continue to gather the operator status from reason and just transform the string in the status card (similar to the installed operator details.) Changing where this status is gathered from is a bigger change and would need more consideration.

rawagner · 2019-12-12T14:08:55Z

Updated operator status to read from status.phase instead of status.reason

I think my inclination would be to continue to gather the operator status from reason and just transform the string in the status card (similar to the installed operator details.) Changing where this status is gathered from is a bigger change and would need more consideration.

CSV details page is using status.phase https://github.com/openshift/console/blob/master/frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx#L714 and CSV list page status.reason. So which one is correct/better ?

andybraren · 2019-12-12T15:16:53Z

CSV details page is using status.phase https://github.com/openshift/console/blob/master/frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx#L714 and CSV list page status.reason. So which one is correct/better ?

@dmesser @tlwu2013?

andybraren · 2019-12-12T19:11:44Z

For the "Degraded" and "Upgrading" sublabels, would we be able to show a count of how many are degraded/upgrading like 2 Degraded or 7 Upgrading?

andybraren · 2019-12-12T23:34:38Z

@rawagner I shared the original design at a UX meeting earlier today and got some great feedback about the all-okay state. I haven't gotten feedback on this design tweak yet, but here it is for reference:

Instead of showing the first 5 alphabetical operators that are all "Succeeded" in each section (which is somewhat arbitrary, distracting, and doesn't confirm that all the other operators are healthy) we'd show a collapsed "All succeeded" (or whatever it ends up being) status with the same "View all" link on the left.

Sorry for the mid-PR change, just trying to keep up with you. 🙂

itsptk · 2019-12-13T19:20:18Z

Spoke with @tlwu2013 , we agreed that this popover should be using status.phase, since thats what the user would see in the the operator details if they clicked the operator name in popover. We didn't want to change the operator list view status at this time, as that is being actively reworked in https://jira.coreos.com/browse/PD-275

vojtechszocs · 2020-01-14T14:42:42Z

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

+
+    /**
+     * Link to all resources page.
+     * If not provided than list page of first resource from resources prop is used.


Suggested change

* If not provided than list page of first resource from resources prop is used.

* If not provided then a list page of first resource from resources prop is used.

vojtechszocs · 2020-01-14T14:43:18Z

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

+    /**
+     * Link to all resources page.
+     * If not provided than list page of first resource from resources prop is used.
+     * */


Suggested change

* */

*/

vojtechszocs · 2020-01-14T14:48:54Z

frontend/packages/console-shared/src/components/dashboard/status-card/OperatorStatusBody.tsx

+  const operatorStatuses = getOperatorsWithStatuses(resources);
+  const sortedOperatorStatuses = getMostImportantStatuses(operatorStatuses)
+    .sort((a, b) => a.operators[0].metadata.name.localeCompare(b.operators[0].metadata.name))
+    .slice(0, 5);


Are we limiting displayed operator statuses to a total of 5 by design?

yes, max 5 is by design

Hm, thinking about this again, maybe we should reconsider and show all degraded/failed operators in the popover rather than just the first alphabetical 5 (which is arbitrary). I think that would be more useful.

Ok, limit is now removed

vojtechszocs · 2020-01-14T14:50:31Z

frontend/packages/console-shared/src/components/dashboard/status-card/OperatorStatusBody.tsx

+      ) : (
+        !operatorsHealthy &&
+        sortedOperatorStatuses.map((operatorStatus) => (
+          <AsyncComponent


vojtechszocs · 2020-01-14T14:54:14Z

frontend/packages/console-shared/src/components/dashboard/status-card/state-utils.ts

+      operators: [],
+    };
+  }
+  const operatorsByStatus: { [key: string]: OperatorStatusWithResources<R> } = operators.reduce<{


I'd prefer the alternative way, which types the accumulator value

const operatorsByStatus = operators.reduce( (acc, o) => { // .. }, {} as { [key: string]: OperatorStatusWithResources<R> }, );

doing

{} as { [key: string]: OperatorStatusWithResources<R> }

throws error

Type assertion on object literals is forbidden, use a type annotation instead.

Is that an ESLint rule? It shouldn't do any harm on empty objects 😃 oh well.

yes, eslint rule eslint(@typescript-eslint/no-object-literal-type-assertion) :)

vojtechszocs · 2020-01-14T14:59:21Z

frontend/packages/console-shared/src/components/dashboard/status-card/states.tsx

+  },
+};
+
+type HealthStateMappingKeys = Exclude<keyof typeof HealthState, 'LOADING'>;


vojtechszocs · 2020-01-14T15:04:27Z

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

+      ],
+      getOperatorsWithStatuses: getClusterServiceVersionsWithStatuses,
+      operatorRowLoader: () =>
+        import(


Let's stay consistent in how we use dynamic imports within this specific module:

loader: async () => (await import('./components/dashboard/csv-status' /* webpackChunkName: "csv-dashboard-status" */)) .default,

vojtechszocs · 2020-01-14T15:06:27Z

frontend/packages/operator-lifecycle-manager/src/status/csv-status.ts

+    status: {
+      installedCSV: getName(csv),
+    },
+  } as any); // 'as any' to supress typescript error caused by lodash;


Can't we use generic type params when calling _.find function? Using as any is discouraged.

vojtechszocs · 2020-01-14T15:10:06Z

frontend/public/components/dashboard/dashboards-page/overview-dashboard/health-item.tsx

+        />
+      );
+    })
+    .reverse();


What's the intention behind reversing the sections array?

we want to have CSV operators before Cluster operators thus we need to reverse the order of extensions (cluster operators are contributed by console app, CSV by operator-lifecycle). We can improve that later on if we will have any more complex cases, like including some extensions before/after others (similar to nav items) but reverse is enough for now

Isn't it better then to explicitly sort the extensions in a way that Cluster operators come before any other ones?

A better solution IMO is to add e.g. priority property to allow sorting extensions in a specific way when they are consumed.

For now, I'm OK with a simple reverse.

vojtechszocs · 2020-01-14T15:18:38Z

frontend/public/components/utils/async.tsx

+export type AsyncComponentProps = {
+  loader: () => Promise<React.ComponentType>;
+  LoadingComponent?: React.ReactNode;
+} & any;


We can improve that later, but I'd encourage not to use type Foo = xxx & any in new code, just like with as any type cast.

vojtechszocs · 2020-01-16T19:16:15Z

/lgtm

openshift-ci-robot · 2020-01-16T19:17:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeff-phillips-18, rawagner, vojtechszocs

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~~ [rawagner,vojtechszocs]

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 requested review from andybraren and benjaminapetersen December 11, 2019 15:35

rawagner force-pushed the operators_status branch 2 times, most recently from 9f20b2f to e90f256 Compare December 11, 2019 16:20

andybraren reviewed Dec 11, 2019

View reviewed changes

itsptk reviewed Dec 11, 2019

View reviewed changes

rawagner force-pushed the operators_status branch 3 times, most recently from f0a4be6 to d924a91 Compare December 12, 2019 09:04

rawagner force-pushed the operators_status branch from d924a91 to c35bfae Compare December 12, 2019 09:09

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2019

rawagner force-pushed the operators_status branch 8 times, most recently from 62fc88d to 910a506 Compare January 13, 2020 09:15

vojtechszocs suggested changes Jan 14, 2020

View reviewed changes

rawagner force-pushed the operators_status branch 2 times, most recently from 8e33ab1 to f8b9b47 Compare January 15, 2020 13:15

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2020

rawagner force-pushed the operators_status branch from f8b9b47 to ad46355 Compare January 15, 2020 13:37

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 15, 2020

rawagner force-pushed the operators_status branch from ad46355 to 754cc33 Compare January 15, 2020 19:40

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2020

jeff-phillips-18 approved these changes Jan 16, 2020

View reviewed changes

Add operators status to Dashboards

e8db4b9

rawagner force-pushed the operators_status branch from 754cc33 to e8db4b9 Compare January 16, 2020 17:23

vojtechszocs approved these changes Jan 16, 2020

View reviewed changes

openshift-ci-robot assigned vojtechszocs Jan 16, 2020

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

openshift-merge-robot merged commit d7ee64a into openshift:master Jan 16, 2020

spadgett added this to the v4.4 milestone Jan 17, 2020

This was referenced Jan 17, 2020

Add Operator updating activity #3575

Closed

Remove cluster operator upgrade activity #3996

Merged

itsptk mentioned this pull request Feb 14, 2020

Bug 1798858: Fix bug where installed operators list status is missin… #4270

Merged

	Operators extend Kubernetes with additional custom resources to manage applications
	Operators extend OpenShift with additional custom resources to manage applications.

	* If not provided than list page of first resource from resources prop is used.
	* If not provided then a list page of first resource from resources prop is used.

Add operators status to Dashboards #3755

Add operators status to Dashboards #3755

Conversation

rawagner commented Dec 11, 2019

andybraren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsptk left a comment

Choose a reason for hiding this comment

rawagner commented Dec 12, 2019 • edited

itsptk commented Dec 12, 2019

rawagner commented Dec 12, 2019

andybraren commented Dec 12, 2019

andybraren commented Dec 12, 2019

andybraren commented Dec 12, 2019

itsptk commented Dec 13, 2019

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

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

vojtechszocs commented Jan 16, 2020

openshift-ci-robot commented Jan 16, 2020

rawagner commented Dec 12, 2019 •

edited