Use MultiListPage for BaremetalHostsPage #1698

jtomasek · 2019-06-12T11:30:49Z

This allows to use machines and nodes data in hosts page

knowncitizen · 2019-06-12T12:13:21Z

spadgett · 2019-06-12T12:40:36Z

Do you mind transitioning to PF4 tables? See #1465

jtomasek · 2019-06-12T12:43:46Z

Do you mind transitioning to PF4 tables? See #1465

I am happy to do so, but I did not want to make this dependent on #1465 as it does not seem to be ready yet. It also seems to be using temporary @patternfly/react-table version (https://github.com/openshift/console/pull/1465/files#diff-0a6694dd0d6b93a2aa14f32e3d873b4cR67)

Edit: Hmm, I see react-table version has been updated and the PR is merged now. I'll create a new PR which changes the table to PF4.

spadgett · 2019-06-12T12:57:28Z

Edit: Hmm, I see react-table version has been updated and the PR is merged now. I'll create a new PR which changes the table to PF4.

Yeah, it's already merged. The only tables not transitioned are the ones in our static plugins. When we update those, we can remove the legacy list components. Thanks!

jtomasek · 2019-06-13T15:05:56Z

Updated PR with commit which updates the Baremetal Hosts listing to use new PF4 table

This allows to use machines and nodes data in hosts page

spadgett · 2019-06-14T00:50:40Z

frontend/packages/metal3-plugin/src/components/host.tsx

-  </ListHeader>
-);
+const tableColumnClasses = [
+  classNames('pf-m-5-col-on-lg', 'pf-m-8-col-on-md', 'pf-m-12-col-on-sm'),


Sorry if we're asking you to code to a moving target, but we removed these classes. They aren't standard Patternfly classes. See #1707

due to false positives in typescript when using React.FC

spadgett · 2019-06-14T12:49:16Z

frontend/packages/eslint-plugin-console/lib/config/rules/react.js

+
+  // Prevent missing props validation in a React component definition
+  // Off due to false positives in typescript
+  'react/prop-types': 'off',


@christianvogt fyi, looks like this will conflict with #1721

I gave the suggestion. That's fine. Whichever comes first.

spadgett · 2019-06-14T19:08:18Z

frontend/packages/console-shared/src/selectors/machine.ts

+import { getName } from './common';
+
+export const getMachineRole = (machine) =>
+  _.get(machine, ['metadata', 'labels', 'machine.openshift.io/cluster-api-machine-role']);


We already have a helper for this here:

https://github.com/openshift/console/blob/master/frontend/public/components/machine.tsx#L26

It looks like the label is different though. We should check what's correct and only have this helper in one place.

Looking here https://github.com/openshift/machine-api-operator it seems that the label has been changed to machine.openshift.io

Looking here https://github.com/openshift/machine-api-operator it seems that the label has been changed to machine.openshift.io

@jtomasek We need to evaluate whether this was shipped as a bug in 4.1. If it was, we should open a Bugzilla to fix it in 4.1.z.

spadgett · 2019-06-14T19:08:34Z

frontend/packages/console-shared/src/selectors/machine.ts

+
+export const getMachineRole = (machine) =>
+  _.get(machine, ['metadata', 'labels', 'machine.openshift.io/cluster-api-machine-role']);
+export const getMachineNodeName = (machine) => _.get(machine, 'status.nodeRef.name');


Already defined here:

https://github.com/openshift/console/blob/master/frontend/public/components/machine.tsx#L28

I am going to update the components/machine.tsx to use selectors from @console/shared

spadgett · 2019-06-14T19:09:33Z

frontend/packages/console-shared/src/selectors/machine.ts

+  _.get(machine, ['metadata', 'labels', 'machine.openshift.io/cluster-api-machine-role']);
+export const getMachineNodeName = (machine) => _.get(machine, 'status.nodeRef.name');
+
+export const getMachineNode = (machine: K8sResourceKind, nodes: K8sResourceKind[]) =>


Prefer NodeKind

Suggested change

export const getMachineNode = (machine: K8sResourceKind, nodes: K8sResourceKind[]) =>

export const getMachineNode = (machine: K8sResourceKind, nodes: NodeKind[]) =>

spadgett · 2019-06-14T19:09:55Z

frontend/packages/metal3-plugin/src/components/host-role.tsx

+import { K8sResourceKind } from '@console/internal/module/k8s';
+
+type BaremetalHostRoleProps = {
+  machine: K8sResourceKind;


Suggested change

machine: K8sResourceKind;

machine: MachineKind;

spadgett · 2019-06-14T19:11:30Z

frontend/packages/metal3-plugin/src/components/host.tsx

+    title: 'Machine',
+    sortField: 'spec.machineRef.name',
+    transforms: [sortable],
+    props: { className: tableColumnClasses[2] },


We're skipping a column class. Is that intentional?

Yes, that one is going to be used by Host Status. I've updated first column to span its space for the time being.

spadgett · 2019-06-14T19:11:48Z

frontend/packages/metal3-plugin/src/components/host.tsx

+];
+
+type HostsTableRowProps = {
+  obj: K8sResourceKind & { machine: K8sResourceKind; node: K8sResourceKind };


Suggested change

obj: K8sResourceKind & { machine: K8sResourceKind; node: K8sResourceKind };

obj: K8sResourceKind & { machine: MachineKind; node: NodeKind };

spadgett · 2019-06-14T19:13:52Z

/assign

* update machine selector types to use specific kinds * remove selectors definition from machine module * use machine selectors from @console/shared in machine, machine-set and machine-deployment modules

spadgett · 2019-06-17T19:22:38Z

frontend/packages/console-shared/src/selectors/common.ts

@@ -0,0 +1,6 @@
+import * as _ from 'lodash-es';


We should consider a different name for this folder. "selectors" has a specific meaning in k8s (label selectors)

spadgett · 2019-06-17T19:24:53Z

frontend/packages/console-shared/src/selectors/machine.ts

+import { getName } from './common';
+
+export const getMachineRole = (machine) =>
+  _.get(machine, ['metadata', 'labels', 'machine.openshift.io/cluster-api-machine-role']);


Looking here https://github.com/openshift/machine-api-operator it seems that the label has been changed to machine.openshift.io

@jtomasek We need to evaluate whether this was shipped as a bug in 4.1. If it was, we should open a Bugzilla to fix it in 4.1.z.

spadgett

/approve
/lgtm

spadgett · 2019-06-17T19:27:26Z

frontend/packages/metal3-plugin/src/components/host.tsx

+
+  const flatten = (resources) => {
+    // TODO(jtomasek): Remove loaded check once ListPageWrapper_ is updated to call flatten only
+    // when resources are loaded


Are we sure we want to make that change in ListPageWrapper_? I'd think we'd want to show the data as it comes in. Otherwise we'd block rendering the table.

I'll create a separate issue for this and try to elaborate on the issue I am seeing.

openshift-ci-robot · 2019-06-17T19:28:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jtomasek, knowncitizen, spadgett

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~~ [spadgett]

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

openshift-bot · 2019-06-17T19:32:47Z

/retest

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

openshift-bot · 2019-06-17T21:42:33Z

/retest

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

openshift-bot · 2019-06-17T21:54:42Z

/retest

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

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 12, 2019

openshift-ci-robot requested review from knowncitizen and kyoto June 12, 2019 11:31

knowncitizen approved these changes Jun 12, 2019

View reviewed changes

openshift-ci-robot assigned jhadvig Jun 12, 2019

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

Jiri Tomasek added 2 commits June 13, 2019 18:18

Use MultiListPage for BaremetalHostsPage

11da9e4

This allows to use machines and nodes data in hosts page

Add Nodes to Host MultiListPage

657e50c

knowncitizen approved these changes Jun 13, 2019

View reviewed changes

spadgett reviewed Jun 14, 2019

View reviewed changes

Jiri Tomasek added 2 commits June 14, 2019 13:23

Convert Baremetal Hosts list to use PF4 table

91f354e

Disable react/prop-types eslint rule

5017a6f

due to false positives in typescript when using React.FC

knowncitizen approved these changes Jun 14, 2019

View reviewed changes

spadgett requested changes Jun 14, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Jun 14, 2019

Use machine selectors from @console/shared

3c293aa

* update machine selector types to use specific kinds * remove selectors definition from machine module * use machine selectors from @console/shared in machine, machine-set and machine-deployment modules

spadgett reviewed Jun 17, 2019

View reviewed changes

spadgett approved these changes Jun 17, 2019

View reviewed changes

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

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

openshift-merge-robot merged commit 890db4c into openshift:master Jun 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use MultiListPage for BaremetalHostsPage #1698

Use MultiListPage for BaremetalHostsPage #1698

jtomasek commented Jun 12, 2019

knowncitizen commented Jun 12, 2019

spadgett commented Jun 12, 2019

jtomasek commented Jun 12, 2019 •

edited

Loading

spadgett commented Jun 12, 2019

jtomasek commented Jun 13, 2019

spadgett Jun 14, 2019

jtomasek Jun 14, 2019

spadgett Jun 14, 2019

christianvogt Jun 17, 2019

spadgett Jun 14, 2019 •

edited

Loading

jtomasek Jun 17, 2019

spadgett Jun 17, 2019

spadgett Jun 14, 2019

jtomasek Jun 17, 2019

jtomasek Jun 17, 2019

spadgett Jun 14, 2019

jtomasek Jun 17, 2019

spadgett Jun 14, 2019

jtomasek Jun 17, 2019

spadgett Jun 14, 2019

jtomasek Jun 17, 2019

spadgett Jun 14, 2019

jtomasek Jun 17, 2019

spadgett commented Jun 14, 2019

spadgett Jun 17, 2019

spadgett Jun 17, 2019

spadgett left a comment

spadgett Jun 17, 2019

jtomasek Jun 18, 2019

openshift-ci-robot commented Jun 17, 2019

openshift-bot commented Jun 17, 2019

openshift-bot commented Jun 17, 2019

openshift-bot commented Jun 17, 2019

	export const getMachineNode = (machine: K8sResourceKind, nodes: K8sResourceKind[]) =>
	export const getMachineNode = (machine: K8sResourceKind, nodes: NodeKind[]) =>

	obj: K8sResourceKind & { machine: K8sResourceKind; node: K8sResourceKind };
	obj: K8sResourceKind & { machine: MachineKind; node: NodeKind };

Use MultiListPage for BaremetalHostsPage #1698

Use MultiListPage for BaremetalHostsPage #1698

Conversation

jtomasek commented Jun 12, 2019

knowncitizen commented Jun 12, 2019

spadgett commented Jun 12, 2019

jtomasek commented Jun 12, 2019 • edited Loading

spadgett commented Jun 12, 2019

jtomasek commented Jun 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

spadgett Jun 14, 2019 • edited Loading

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

spadgett commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

openshift-ci-robot commented Jun 17, 2019

openshift-bot commented Jun 17, 2019

openshift-bot commented Jun 17, 2019

openshift-bot commented Jun 17, 2019

jtomasek commented Jun 12, 2019 •

edited

Loading

spadgett Jun 14, 2019 •

edited

Loading