Add details card #1696

rawagner · 2019-06-12T08:27:45Z

Connect dashboard cards to K8s resources with Firehose.

openshift-ci-robot · 2019-06-12T08:27:59Z

Hi @rawagner. 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.

Details

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.

spadgett · 2019-06-12T12:29:30Z

frontend/public/components/dashboards-page/with-dashboard-resources.tsx

  [RESULTS_TYPE.URL]: ImmutableMap<string, any>;
 };

+export type FirehoseResource = {


We should try to move this to a location where this type can be used in other contexts.

moved to module/k8s/index.ts. Im not sure if there is more appropriate place.

Similar type was used in factory/details so I reused it there too.

Yeah, I'm not sure the best place either. @alecmerdler ?

I would like to use this type in other places. We should eventually change firehose to use TypeScript. @alecmerdler has looked at that in the past.

Put it in utils/index.tsx until we have firehose.tsx.

ok, moved to utils/index

frontend/public/components/dashboard/details-card/detail-item.tsx

frontend/public/components/dashboards-page/overview-dashboard/details-card.tsx

spadgett · 2019-06-12T12:37:21Z

frontend/public/components/dashboards-page/overview-dashboard/details-card.tsx

+  return null;
+};
+
+const getKubernetesVersion = (kubernetesVersionResponse): string =>


We have this logic in the about dialog. We should try to write it once and use it everywhere.

Extracted the logic into separate functions in about dialog and they are now reused in Details Card. Not sure if there is a better place to move them to ?

Extracted the logic into separate functions in about dialog and they are now reused in Details Card. Not sure if there is a better place to move them to ?

I'd suggest cluster-settings.ts

rawagner · 2019-06-14T09:15:39Z

@spadgett please take a look

spadgett · 2019-06-14T17:48:06Z

/assign

frontend/public/components/about-modal.tsx

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

spadgett · 2019-06-14T17:51:47Z

frontend/public/components/dashboards-page/overview-dashboard/details-card.tsx

We should make sure we have a consistent implementation with #1721. I'm in favor of using the API server host as-is for the moment since we don't have access to a real cluster name.

@christianvogt fyi

Im good with using API server as-is. Extracted the getClusterName function to cluster-settings.ts so it can be reused by #1721

frontend/public/components/dashboards-page/overview-dashboard/details-card.tsx

spadgett · 2019-06-14T17:54:46Z

frontend/public/components/dashboards-page/overview-dashboard/details-card.tsx

Suggested change

const infrastructureData = _.get(resources, 'infrastructure.data', {});

const infrastructureData: K8sResourceKind = _.get(resources, 'infrastructure.data', {});

although TypeScript will yell since {} isn't a K8sResourceKind. We should fix that though so we have the type checking.

spadgett · 2019-06-14T17:56:24Z

frontend/public/components/dashboard/details-card/detail-item.tsx

I haven't seen how this looks, but I suspect having a loading icon for every detail item is too much animation.

Again we should consider skeleton while the widget is loading.

Yes, there is going to be quite a few loadings through the Overview Tab. We will be adding skeletons as a follow up.

spadgett · 2019-06-14T17:57:45Z

frontend/public/components/dashboards-page/with-dashboard-resources.tsx

Is there a better type we can use? Is this a FirehoseResource[]?

that is actually what Firehose passes down - i've added better type but Im not sure if it is already created somewhere so I could reuse it.

frontend/public/components/dashboards-page/overview-dashboard/details-card.tsx

spadgett · 2019-06-17T19:12:49Z

frontend/public/components/dashboards-page/overview-dashboard/details-card.tsx

This seems cleaner to me:

Suggested change

const clusterVersion = _.get(resources, 'cv');

const clusterVersion = _.get(resources, 'cv.data') as ClusterVersionKind;

// ...

const openshiftVersion = getOpenShiftVersion(clusterVersion);

Typescript complains Conversion of type 'FirehoseResult' to type 'ClusterVersionKind' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first. . It thinks that cv.data is FirehoseResult but in reality it is K8sResourceKind | K8sResourceKind[] :/ Splitting the expression in a way I do does not result in error. Seems like a bug in TS.

That seems weird. Is this a bug in the lodash typings? How does TypeScript know the type returned by _.get?

I'd say that is an issue of @types/lodash-es but Im not very familiar with it. I tried updating to latest version (4.17.3, console is using 4.17.0) but it didnt help :/

spadgett · 2019-06-17T19:14:06Z

frontend/public/components/utils/index.tsx

We should update details.tsx to use this:

console/frontend/public/components/factory/details.tsx

Line 10 in ec544a3

export type FirehoseResource = {

That is already updated - see https://github.com/openshift/console/pull/1696/files#diff-c09a5bef454710fc5ba83321c2e0fe1fR5

spadgett · 2019-06-17T19:14:50Z

frontend/public/components/utils/index.tsx

We should update overview/index.tsx to use this:

https://github.com/openshift/console/blob/master/frontend/public/components/overview/index.tsx#L1065-L1068

overview/index.tsx is now updated. I've changed FirehoseResult type definition a bit to fit overview/index.tsx better

spadgett

Thanks, please squash

spadgett · 2019-06-18T18:57:45Z

frontend/public/components/dashboard/details-card/detail-item.tsx

How is id used for Tooltip? I don't see where it's referenced. If it's not required, we should leave it out.

It is required https://github.com/react-bootstrap/react-bootstrap/blob/master/src/Tooltip.js#L14

spadgett · 2019-06-18T18:58:45Z

frontend/public/components/dashboard/details-card/detail-item.tsx

Can this be {value} or if necessary <React.Fragment>{value}</React.Fragment> to avoid the unnecessary span?

no :/ using {value} results in error in runtime and <React.Fragment>{value}</React.Fragment> does not work either - no error, but tooltip does not appear when hovering over

That seems like a Patternfly bug :/

spadgett · 2019-06-18T19:04:05Z

frontend/public/components/dashboards-page/overview-dashboard/details-card.tsx

That seems weird. Is this a bug in the lodash typings? How does TypeScript know the type returned by _.get?

spadgett

/lgtm

spadgett · 2019-06-18T19:46:01Z

/approve

spadgett · 2019-06-18T19:46:31Z

/ok-to-test

Connect dashboard cards to K8s resources with Firehose

spadgett · 2019-06-18T19:50:25Z

/lgtm

openshift-ci-robot · 2019-06-18T19:50:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 12, 2019

openshift-ci-robot requested review from TheRealJon and jcaianirh June 12, 2019 08:28

spadgett reviewed Jun 12, 2019

View reviewed changes

spadgett added this to the v4.2 milestone Jun 14, 2019

openshift-ci-robot assigned spadgett Jun 14, 2019

spadgett reviewed Jun 14, 2019

View reviewed changes

spadgett reviewed Jun 17, 2019

View reviewed changes

rawagner force-pushed the dashboard-k8s-simple branch 3 times, most recently from d5f67e8 to 7b38fb2 Compare June 18, 2019 17:35

spadgett reviewed Jun 18, 2019

View reviewed changes

spadgett approved these changes Jun 18, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2019

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 Jun 18, 2019

Add details card.

414e422

Connect dashboard cards to K8s resources with Firehose

rawagner force-pushed the dashboard-k8s-simple branch from 7b38fb2 to 414e422 Compare June 18, 2019 19:48

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2019

openshift-merge-robot merged commit 358505f into openshift:master Jun 19, 2019

	const infrastructureData = _.get(resources, 'infrastructure.data', {});
	const infrastructureData: K8sResourceKind = _.get(resources, 'infrastructure.data', {});

-  const clusterVersion = _.get(resources, 'cv');
+  const clusterVersion = _.get(resources, 'cv.data') as ClusterVersionKind;
+  // ...
+  const openshiftVersion = getOpenShiftVersion(clusterVersion);

Add details card #1696

Add details card #1696

Uh oh!

Conversation

rawagner commented Jun 12, 2019

Uh oh!

openshift-ci-robot commented Jun 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rawagner commented Jun 14, 2019

Uh oh!

spadgett commented Jun 14, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!