Added insights-plugin #6660

bond95 · 2020-09-17T07:09:45Z

This PR adds new insights-plugin, which is used to show insights information about cluster, such as number of issues that was found and Total Risks of issues and link to cluster's page in OCM.

More info about integration insights into OCP WebConsole could be found here: https://docs.google.com/document/d/1d4KdH3DpUrUTFIrFN6tnItQPFV2NRFYnm_-yMJ6gNN0/edit#

openshift-ci-robot · 2020-09-17T07:10:09Z

Hi @bond95. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

iNecas · 2020-09-23T14:21:09Z

frontend/packages/insights-plugin/src/models.ts

@@ -0,0 +1,13 @@
+import { K8sKind } from '@console/internal/module/k8s';
+
+export const InsightsModel: K8sKind = {


I guess this can be removed now, right?

bond95 · 2020-09-29T07:59:43Z

@spadgett @smarterclayton @vojtechszocs @doruskova Could you, please, also take a look?

doruskova · 2020-09-29T11:53:07Z

@bond95 Can you add a better quality image for the card? Thanks

bond95 · 2020-09-29T14:39:44Z

@doruskova done

doruskova · 2020-09-29T14:42:15Z

@bond95 It looks great, but is possible to align the icons with the title "Total risk"?

spadgett

A few comments, but looks good overall. Nice work. Thanks!

spadgett · 2020-09-30T12:26:02Z

frontend/packages/insights-plugin/package.json

+    "@console/ceph-storage-plugin": "0.0.0-fixed",
+    "@console/container-security": "0.0.0-fixed",
+    "@console/dev-console": "0.0.0-fixed",
+    "@console/internal": "0.0.0-fixed",
+    "@console/knative-plugin": "0.0.0-fixed",
+    "@console/kubevirt-plugin": "0.0.0-fixed",
+    "@console/local-storage-operator-plugin": "0.0.0-fixed",
+    "@console/metal3-plugin": "0.0.0-fixed",
+    "@console/network-attachment-definition-plugin": "0.0.0-fixed",
+    "@console/noobaa-storage-plugin": "0.0.0-fixed",
+    "@console/operator-lifecycle-manager": "0.0.0-fixed",


I assume you don't really depend on all of these? You should only need to specify dependencies specific to your plugin here.

spadgett · 2020-09-30T12:27:04Z

frontend/packages/insights-plugin/OWNERS

+  - bond95
+  - tisnik
+labels:
+  - component/insights


You'll need a PR to the openshift/release repo to add this label. See openshift/release#4526

I created PR for that openshift/release#12398

spadgett · 2020-09-30T12:28:23Z

frontend/packages/insights-plugin/src/components/InsightsPopup/index.tsx

+
+export const InsightsPopup: React.FC<PrometheusHealthPopupProps> = ({ responses, k8sResult }) => {
+  const resource = mapMetrics(responses[0].response);
+  const clusterId = _.get(k8sResult, 'data.spec.clusterID', '');


nit: For new code, we prefer optional chaining. Console naming conventions would make this uppercase ID

Suggested change

const clusterId = _.get(k8sResult, 'data.spec.clusterID', '');

const clusterID = k8sResult?.data?.spec?.clusterID || '';

spadgett · 2020-09-30T12:30:40Z

frontend/packages/insights-plugin/src/components/InsightsPopup/index.tsx

+          <>
+            <div style={{ fontWeight: 'bold' }}>Fixable issues</div>
+            <div>
+              <a href={`https://cloud.redhat.com/openshift/details/${clusterId}`}>


Use the ExternalLink component. Here's an example:

console/frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-community-provider-modal.tsx

Lines 52 to 55 in 24912ac

<ExternalLink

href={RH_OPERATOR_SUPPORT_POLICY_LINK}

text="Learn more about Red Hat’s third party software support policy"

/>

spadgett · 2020-09-30T12:30:54Z

frontend/packages/insights-plugin/src/components/InsightsPopup/index.tsx

+          </>
+        )}
+        {!isThereIssues && (
+          <a href="https://docs.openshift.com/container-platform/latest/support/getting-support.html">


ExternalLink here as well.

spadgett · 2020-09-30T12:32:58Z

frontend/packages/insights-plugin/src/components/InsightsPopup/style.scss

+	.co-insights__no-rules {
+		color: var(--pf-global--Color--200);
+	}
+}


nit: missing newline at the end of the file.

spadgett · 2020-09-30T12:33:38Z

frontend/packages/insights-plugin/src/components/InsightsPopup/style.scss

@@ -0,0 +1,5 @@
+.co-insights__box {
+	.co-insights__no-rules {


We typically avoid nesting of CSS styles and find it's not often necessary with BEM. Is .co-insights__no-rules by itself specific enough here?

rawagner · 2020-09-30T12:43:21Z

frontend/packages/insights-plugin/src/components/InsightsPopup/mappers.ts

+  moderate?: number;
+};
+
+export const mapMetrics = (response) => {


missing types

rawagner · 2020-09-30T12:47:58Z

frontend/packages/insights-plugin/src/plugin.tsx

+          (m) => m.InsightsPopup,
+        ),
+      popupTitle: 'Insights status',
+    },


You need to gate the extension by some model contributed by Insights operator. To do so:

create ModelDefinition extension

{ type: 'ModelDefinition', properties: { models: [YourModel], }, },

Add FeatureFlag extension

const INSIGHTS_FLAG = 'INSIGHTS_FLAG';

{ type: 'FeatureFlag/Model', properties: { model: YourModel, flag: INSIGHTS_FLAG, }, },

gate Dashboard Health with the above flag

Suggested change

},

flags: {

required: [InsightsFlag],

},

},

@bond95 As @rawagner wrote, we should enable the plugin's extensions only when the cluster has the right capability (i.e. Insights operator is installed).

Detecting Insights operator can be done via CRD detection. Use ModelDefinition extension to make Console aware of your CRD, then use FeatureFlag/Model extension to introduce a feature flag reflecting the presence of such CRD.

Every other extension can then use the flags object to declare which feature flags are required and/or disallowed in order for that particular extension to be in use. Typically, you'd want all Insights extensions to be gated by INSIGHTS_FLAG.

@rawagner @vojtechszocs What if we don't have any CRD?

Another option is to implement your own detection logic and use CustomFeatureFlag . Example https://github.com/openshift/console/blob/master/frontend/packages/ceph-storage-plugin/src/plugin.ts#L76

@rawagner We had discussion with @smarterclayton and @spadgett, they suggested not to hide tab even if insights-operator is disabled.

@bond95 Assuming that Insights integration is implemented via cluster-level operator (i.e. not via OLM operator), it makes sense to have Insights plugin extensions active by default 👍

bond95 · 2020-09-30T15:37:36Z

@doruskova I updated screenshot, please, take a look

vojtechszocs · 2020-09-30T16:53:45Z

frontend/packages/console-app/package.json

@@ -12,6 +12,7 @@
    "@console/container-security": "0.0.0-fixed",
    "@console/dev-console": "0.0.0-fixed",
    "@console/internal": "0.0.0-fixed",
+    "@console/insights-plugin": "0.0.0-fixed",


@bond95 @spadgett FYI, this means Insights plugin will be included in all Console builds by default.

vojtechszocs · 2020-09-30T17:02:29Z

frontend/packages/insights-plugin/package.json

+{
+  "name": "@console/insights-plugin",
+  "version": "0.0.0-fixed",
+  "description": "Plugin for Insights operator",


Nit: I'd put a more descriptive message here, like:

Insights - provide cluster health data and integrate with OpenShift Cluster Manager

vojtechszocs · 2020-09-30T17:04:07Z

frontend/packages/insights-plugin/package.json

+  "private": true,
+  "main": "src/index.ts",
+  "scripts": {
+    "test": "yarn --cwd ../.. run test packages/insights-plugin"


Suggested change

"test": "yarn --cwd ../.. run test packages/insights-plugin"

"test": "yarn --cwd ../.. test packages/insights-plugin"

vojtechszocs · 2020-09-30T17:18:19Z

frontend/packages/insights-plugin/src/plugin.tsx

+          (m) => m.InsightsPopup,
+        ),
+      popupTitle: 'Insights status',
+    },


@bond95 As @rawagner wrote, we should enable the plugin's extensions only when the cluster has the right capability (i.e. Insights operator is installed).

Detecting Insights operator can be done via CRD detection. Use ModelDefinition extension to make Console aware of your CRD, then use FeatureFlag/Model extension to introduce a feature flag reflecting the presence of such CRD.

Every other extension can then use the flags object to declare which feature flags are required and/or disallowed in order for that particular extension to be in use. Typically, you'd want all Insights extensions to be gated by INSIGHTS_FLAG.

doruskova · 2020-10-01T11:01:05Z

@bond95 Thank you for the update. I noticed 2 things:

Insights status title looks smaller than the Total risk title. Insights status should use 16px and Total risk 14px (If it's too small > use 16px).
Could you align Insights status, Total risk, and Fixable issues? Currently, the total risk is more indented to the right.

bond95 · 2020-10-05T10:40:29Z

@doruskova I updated screenshot. About headers, Insights header is default one, it size is 14px, so I made Total risk size same as Fixable issues (13px).

doruskova · 2020-10-05T10:59:58Z

@bond95 The alignment looks great now, but you should keep 14px for every title because 13px is not an official variable and it's too small for a user. Then it should be fine :)

vojtechszocs · 2020-10-06T19:05:05Z

frontend/packages/insights-plugin/src/components/InsightsPopup/mappers.ts

+  global_palette_gold_400,
+  global_palette_orange_300,
+  global_palette_red_200,
+} from '@patternfly/react-tokens';


Please disable @typescript-eslint/camelcase rule only for the relevant imports:

import * as _ from 'lodash'; /* eslint-disable @typescript-eslint/camelcase */ import { global_palette_blue_50, global_palette_blue_300, global_palette_gold_400, global_palette_orange_300, global_palette_red_200, } from '@patternfly/react-tokens'; /* eslint-enable @typescript-eslint/camelcase */

vojtechszocs · 2020-10-06T19:13:08Z

frontend/packages/insights-plugin/src/plugin.tsx

+          (m) => m.InsightsPopup,
+        ),
+      popupTitle: 'Insights status',
+    },


@bond95 Assuming that Insights integration is implemented via cluster-level operator (i.e. not via OLM operator), it makes sense to have Insights plugin extensions active by default 👍

vojtechszocs · 2020-10-06T19:14:09Z

frontend/packages/insights-plugin/src/plugin.tsx

+import {
+  Plugin,
+  DashboardsOverviewHealthPrometheusSubsystem,
+  ModelFeatureFlag,


Nit: unnecessary type import, since we don't need to add any feature flags.

Please remove it here and update the ConsumedExtensions type alias.

vojtechszocs · 2020-10-07T13:45:38Z

/lgtm

vojtechszocs · 2020-10-07T13:47:51Z

/lgtm cancel

vojtechszocs · 2020-10-07T13:50:42Z

Please import from @patternfly/react-tokens like

import { global_palette_blue_300 as blueInfoColor } from '@patternfly/react-tokens/dist/js/global_palette_blue_300';

vojtechszocs · 2020-10-07T14:02:49Z

/lgtm

openshift-ci-robot · 2020-10-07T14:03:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bond95, 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~~ [vojtechszocs]

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

spadgett · 2020-10-07T14:57:18Z

/ok-to-test

openshift-bot · 2020-10-07T15:13:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-07T16:44:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-10-07T18:07:13Z

@bond95: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	`f15bcd5`	link	`/test kubevirt-plugin`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-bot · 2020-10-07T19:07:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Added insights-plugin

20e34e7

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/core Related to console core functionality labels Sep 17, 2020

openshift-ci-robot requested review from rhamilto and TheRealJon September 17, 2020 07:10

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels Sep 17, 2020

bond95 force-pushed the added-insights-plugin branch from 5a7d565 to 4ec3d93 Compare September 22, 2020 23:25

bond95 changed the title ~~WIP: Added insights-plugin~~ Added insights-plugin Sep 22, 2020

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 Sep 22, 2020

bond95 force-pushed the added-insights-plugin branch from 4ec3d93 to c8e0826 Compare September 23, 2020 14:01

iNecas reviewed Sep 23, 2020

View reviewed changes

bond95 force-pushed the added-insights-plugin branch from c8e0826 to 2cae023 Compare September 29, 2020 07:57

spadgett reviewed Sep 30, 2020

View reviewed changes

rawagner suggested changes Sep 30, 2020

View reviewed changes

bond95 force-pushed the added-insights-plugin branch from 2cae023 to 761f37c Compare September 30, 2020 15:36

vojtechszocs suggested changes Sep 30, 2020

View reviewed changes

bond95 mentioned this pull request Oct 2, 2020

Added component/insights label openshift/console labels openshift/release#12398

Merged

bond95 force-pushed the added-insights-plugin branch 2 times, most recently from 77fce98 to eff72c0 Compare October 5, 2020 10:37

bond95 force-pushed the added-insights-plugin branch from eff72c0 to a596493 Compare October 5, 2020 14:28

vojtechszocs suggested changes Oct 6, 2020

View reviewed changes

bond95 force-pushed the added-insights-plugin branch 3 times, most recently from 9fb0646 to dbf394b Compare October 7, 2020 13:33

openshift-ci-robot assigned vojtechszocs Oct 7, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 7, 2020

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 7, 2020

bond95 force-pushed the added-insights-plugin branch from dbf394b to 7e2d6df Compare October 7, 2020 13:53

Changed CRD fetching to Prometheus

79a226b

bond95 force-pushed the added-insights-plugin branch from 7e2d6df to 79a226b Compare October 7, 2020 13:56

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

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

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 7, 2020

openshift-merge-robot merged commit b47c876 into openshift:master Oct 7, 2020

spadgett added this to the v4.7 milestone Oct 13, 2020

ahardin-rh mentioned this pull request Oct 21, 2020

Added Insights plug-in release notes openshift/openshift-docs#26626

Merged

quarckster mentioned this pull request Nov 26, 2020

Added tests for Insights popup #7340

Closed

		@@ -0,0 +1,13 @@
		import { K8sKind } from '@console/internal/module/k8s';

		export const InsightsModel: K8sKind = {

	const clusterId = _.get(k8sResult, 'data.spec.clusterID', '');
	const clusterID = k8sResult?.data?.spec?.clusterID \|\| '';

	<ExternalLink
	href={RH_OPERATOR_SUPPORT_POLICY_LINK}
	text="Learn more about Red Hat’s third party software support policy"
	/>

-    },
+   flags: {
+      required: [InsightsFlag],
+    },
+    },

	"test": "yarn --cwd ../.. run test packages/insights-plugin"
	"test": "yarn --cwd ../.. test packages/insights-plugin"

Added insights-plugin #6660

Added insights-plugin #6660

Conversation

bond95 commented Sep 17, 2020 • edited

openshift-ci-robot commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bond95 commented Sep 29, 2020 • edited

doruskova commented Sep 29, 2020

bond95 commented Sep 29, 2020

doruskova commented Sep 29, 2020

spadgett 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

bond95 commented Sep 30, 2020

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

doruskova commented Oct 1, 2020

bond95 commented Oct 5, 2020

doruskova commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs commented Oct 7, 2020

vojtechszocs commented Oct 7, 2020

vojtechszocs commented Oct 7, 2020

vojtechszocs commented Oct 7, 2020

openshift-ci-robot commented Oct 7, 2020

spadgett commented Oct 7, 2020

openshift-bot commented Oct 7, 2020

openshift-bot commented Oct 7, 2020

openshift-ci-robot commented Oct 7, 2020 • edited

openshift-bot commented Oct 7, 2020

bond95 commented Sep 17, 2020 •

edited

bond95 commented Sep 29, 2020 •

edited

openshift-ci-robot commented Oct 7, 2020 •

edited