Introduce Metal3 plugin for openshift console #1539

jtomasek · 2019-05-07T11:27:34Z

This is the initial introduction of the metal3 plugin. Currently or testing purposes only.

~~Depends on #1586~~

TODO:

~~remove web-ui-components dependency~~
split into multiple PRs
add OWNERS file

jtomasek · 2019-05-07T11:34:28Z

frontend/public/kinds.ts

-    ? state.k8s.getIn(['RESOURCES', 'models']).get(plural)
-    : _.find(k8sModels, model => model.plural === plural);
+    ? state.k8s.getIn(['RESOURCES', 'models', plural])
+    : state.k8s.getIn(['RESOURCES', 'models']).find(model => model.plural === plural);


This allows the models to be defined in the plugin rather than in common k8sModels module

I think this will break some other cases. For instance, if there are multiple deployments in different API versions, we might get the wrong one. We need to make sure we get the one defined in the static model.

I agree that Console should keep referring to existing model definitions (public/models/index.ts) while also allowing plugins to contribute new model definitions.

I think the best way to solve this is via auto-generated @console/plugin-models module which exports additional model definitions for Console application to work with.

@jelkosz ^^ please create "Allow plugins to contribute new model definitions" task & assign it to me.

@vojtechszocs ok, done

I think the best way to solve this is via auto-generated @console/plugin-models module which exports additional model definitions for Console application to work with.

I've re-thinked this and will avoid generating another virtual module in favor of collecting models from extension objects.

#1586 should fix this.

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

spadgett · 2019-05-07T12:40:24Z

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

+        />
+      </div>
+      <div className={statusColumnClasses}>
+        <WithResources resourceMap={machineName ? hostResourceMap : {}}>


We should not be opening WebSockets for each individual table cell

I remember addressing this kind of issue in our Console fork, @rawagner can you please share the details?

We dealt with this on multiple places, i.e. to show VM status which depends on multiple referenced objects. Optimized by requesting larger dataset via lower count of WebSockets and filtering on UI.

spadgett · 2019-05-07T12:41:50Z

frontend/packages/console-shared/src/components/utils/with-resources.tsx

+};
+
+/*
+ * Firehose helper


Can you explain what this does and why it's needed?

This seems to be a KubeVirt utility, ping @rawagner @suomiy

https://github.com/kubevirt/web-ui/blob/master/frontend/public/kubevirt/components/utils/withResources.jsx

Yes, it is KubeVirt utility which simplifies work with Firehose and adds some features.
For example:

Firehose wont render children if model does not exist in k8s. WithResources will - both cases has their place

Resources accepts resourceMap which is object instend of array as in Firehose which brings us a few enhancements:

you need to define key (which is used as firehose resource prop - just makes it easier if you have more queries for the same model, object forces you to use unique keys)

you can define if resource from resourceMap is required - if yes we wait with child rendering until the resource is loaded (or we can show Loading)

there is also way to define your own error handling

you no longer need to check firehose's loaded prop - when injected resource is undefined it means it is still loading

vojtechszocs · 2019-05-07T15:26:49Z

@spadgett @christianvogt @jelkosz

remove web-ui-components dependency

To clarify, the kubevirt-web-ui-components library provides React components for KubeVirt, Metal3 and Storage related extensions (despite its name, it's not KubeVirt-only). This library was created to reduce the amount of changes in kubevirt/web-ui fork of Console, while also allowing us to test & UXD-review such components in isolation.

We're currently in a "transition" period, where new Console plugins are created while the kubevirt/web-ui fork is still maintained. For now, I'd suggest to continue using kubevirt-web-ui-components as an external dependency in both fork and Console (due to plugins). Once the fork is discontinued, we can move it into the Console monorepo, alongside plugins that depend on it.

add OWNERS file

This is very important, thanks for putting it on the list.

vojtechszocs · 2019-05-07T16:04:51Z

frontend/packages/console-app/package.json

@@ -7,6 +7,7 @@
  "dependencies": {
    "@console/internal": "0.0.0-fixed",
    "@console/plugin-sdk": "0.0.0-fixed",
+    "@console/metal3": "0.0.0-fixed",


Committing this change to git means that Metal 3 plugin will be included in development and production builds by default.

Until the Metal 3 plugin leaves the experimental phase, I'd suggest to treat this as a local change and not commit to git.

vojtechszocs · 2019-05-07T16:15:35Z

frontend/packages/console-metal3/package.json

@@ -0,0 +1,13 @@
+{
+  "name": "@console/metal3",


I'd suggest to follow these conventions:

package path

should include the plugin suffix to denote the purpose of the package

should not include the console prefix (console-xxx is reserved for core Console packages)

proposed value: packages/metal3-plugin

package name

@console scope is required due to monorepo and tooling

proposed value: @console/metal3-plugin

Later on, Metal3 team could add & own other packages as necessary, e.g. packages/metal3-components etc.

vojtechszocs · 2019-05-07T16:22:43Z

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

+  getResource,
+  getSimpleHostStatus,
+  getUid
+} from 'kubevirt-web-ui-components';


We need to add this dependency into the appropriate package.json file.

For now, all core Console dependencies are declared in the monorepo root package. These dependencies reflect the requirements of application code within the public directory, to be moved into the @console/app package in future.

As for kubevirt-web-ui-components, this is a plugin-specific dependency so please add it to Metal3 plugin's package.json file.

vojtechszocs · 2019-05-07T16:32:34Z

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

+        />
+      </div>
+      <div className={statusColumnClasses}>
+        <WithResources resourceMap={machineName ? hostResourceMap : {}}>


I remember addressing this kind of issue in our Console fork, @rawagner can you please share the details?

vojtechszocs · 2019-05-07T16:36:16Z

frontend/packages/console-metal3/src/components/machine-cell.jsx

+import { Icon } from 'patternfly-react';
+import {
+  canHostAddMachine,
+  DASHES,


Nit: isn't there some constant that we can import from @console/internal instead?

It seems that simple '-' string is used frequently in the codebase.

vojtechszocs · 2019-05-07T16:38:25Z

frontend/packages/console-metal3/src/models.ts

+  namespaced: true,
+  kind: 'BareMetalHost',
+  id: 'baremetalhost',
+  crd: true


Nit: missing trailing comma, you can run yarn lint in the monorepo root to ensure there are no linting issues.

vojtechszocs · 2019-05-07T16:41:07Z

frontend/packages/console-metal3/src/plugin.ts

+  {
+    type: 'NavItem/ResourceNS',
+    properties: {
+      section: 'Compute',


This currently yields a TypeScript error since ExtensionProperties.NavItem.section is hard-coded as:

'Home' | 'Workloads'

I'm working on a PR to fix this.

In the meantime, please update the packages/console-plugin-sdk/src/typings/nav.ts module to include the sections you need.

cc @jelkosz ^^ is related to the "Improve nav item extension capabilities" task.

#1584 should fix this.

vojtechszocs · 2019-05-07T16:47:57Z

frontend/packages/console-metal3/src/plugin.ts

+      model: BaremetalHostModel,
+      loader: () =>
+        import(
+          './components/host' /* webpackChunkName: "baremetalhost" */


The webpackChunkName value should reflect the name of your plugin for better traceability.

I'd suggest to start with a single split point, i.e. use webpackChunkName: "metal3" for all async module requests.

We typically use different chunk names for different pages for better code splitting. Then the JS is only loaded when you go to that page.

OK, sounds reasonable 😃

Looking at public/components/resource-pages.ts the convention seems to be to put both resource list and detail pages under the same split point, e.g. pod for both PodsPage and PodsDetailsPage (group by resource).

I've used metal3-baremetalhost

vojtechszocs · 2019-05-07T16:53:08Z

frontend/packages/console-shared/src/components/utils/with-resources.tsx

@@ -0,0 +1,167 @@
+import * as React from 'react';


There doesn't seem to be a corresponding code removal in context of the public directory, where does this code originate from?

@rawagner Is this code used in kubevirt/web-ui fork?

Right, the code is coming from kubevirt/web-ui, I had to add it as hosts listing depends on it. I am happy to split it into a separate PR and update this one to depend on it if needed.

vojtechszocs · 2019-05-07T17:06:27Z

FYI, the kubevirt-web-ui-components dependency also implies adding @patternfly/react-charts.

spadgett · 2019-05-07T17:08:14Z

FYI, the kubevirt-web-ui-components dependency also implies adding @patternfly/react-charts.

OK, we're doing that anyway in #1448

vojtechszocs · 2019-05-07T17:23:01Z

FYI, the kubevirt-web-ui-components dependency also implies adding @patternfly/react-charts.

OK, we're doing that anyway in #1448

Thanks for the update.

#1448 adds "@patternfly/react-charts": "^3.4.2", however the latest kubevirt-web-ui-components release has a peer dependency on "@patternfly/react-charts": "2.x" which should be bumped.

@rawagner @mareklibra ^^ we should fix outdated peer dependencies in kubevirt-web-ui-components.

spadgett · 2019-05-08T17:03:29Z

#1448 adds "@patternfly/react-charts": "^3.4.2", however the latest kubevirt-web-ui-components release has a peer dependency on "@patternfly/react-charts": "2.x" which should be bumped.

@rawagner @mareklibra ^^ we should fix outdated peer dependencies in kubevirt-web-ui-components.

FYI, #1448 merged and now has "@patternfly/react-charts": "^3.4.5"

vojtechszocs · 2019-05-17T20:30:30Z

@jtomasek Please rebase on top of #1586 (includes commits from #1528 and #1584).

vojtechszocs · 2019-05-24T18:41:58Z

@jtomasek FYI, if you need to add YAML templates for your models, check out #1613.

jtomasek · 2019-05-28T08:36:33Z

Rebased on top of #1586
Simplified to avoid using WithResources which will be added in a subsequent changes
Added OWNERS file

mareklibra

lgtm

I am curious to see how often we will be getting merge conflicts in yarn.lock in practise.

mareklibra · 2019-05-30T07:54:34Z

frontend/packages/metal3-plugin/src/plugin.ts

+    type: 'FeatureFlag/Model',
+    properties: {
+      model: BaremetalHostModel,
+      flag: 'METAL3',


nitpick: as this is repeatedly used key, we can introduce a constant for it

vojtechszocs · 2019-05-31T14:56:32Z

I am curious to see how often we will be getting merge conflicts in yarn.lock in practise.

There's one yarn.lock per the whole Console frontend monorepo, so we'll have to deal with those merge conflicts. Instead of manually sync'ing upstream yarn.lock vs. your local yarn.lock, you can rebase and execute dependency-mgmt Yarn commands on top of the latest upstream yarn.lock.

vojtechszocs

Looks good, just a couple of comments.

Please make sure to run yarn lint to catch any stylish issues.

vojtechszocs · 2019-05-31T14:59:29Z

frontend/packages/metal3-plugin/OWNERS

+approvers:
+  - jtomasek
+  - honza
+  - flofuchs


Nit: sources should have one trailing newline by default.

Done

Looks like it's still missing the final newline

vojtechszocs · 2019-05-31T15:06:16Z

frontend/packages/metal3-plugin/src/plugin.ts

+  {
+    type: 'ModelDefinition',
+    properties: {
+      models: [BaremetalHostModel],


You can also do

-import { BaremetalHostModel } from './models'; +import * as models from './models';

-models: [BaremetalHostModel], +models: _.values(models),

like in #1592 to avoid manually importing individual models (if you expect to have lots of them).

I don't expect there to be many more models, so I listed it explicitly here

OK, I'm fine with that.

vojtechszocs · 2019-05-31T15:09:16Z

frontend/packages/metal3-plugin/src/plugin.ts

+        name: 'Bare Metal Hosts',
+        resource: 'baremetalhosts',
+        required: METAL3_FLAG
+      },


If you want to put your nav link after an existing one, you can use mergeAfter.

spadgett · 2019-05-31T17:28:27Z

/go/src/github.com/openshift/console/frontend/packages/metal3-plugin/src/plugin.ts
  19:29  error  Missing semicolon       semi
  32:24  error  Missing trailing comma  comma-dangle
  42:30  error  Missing trailing comma  comma-dangle

✖ 3 problems (3 errors, 0 warnings)
  3 errors and 0 warnings potentially fixable with the `--fix` option.

spadgett · 2019-05-31T17:30:20Z

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

+  // getHostMachineName,
+  getHostBmcAddress,
+  getName,
+  getNamespace,


Same comment as in #1592, these aren't kubevirt specific. They should be added somewhere that can be used generally across console.

I am happy to move this to console/shared in a subsequent PR and gradually reduce the dependency on web-ui-components. To unblock work depending on this PR. Would that be sufficient?

We plan to move kubevirt-web-ui-components to console/shared or similar packages within one of the last steps of merging web-ui. Until this happens, we would need to maintain these utility functions (or similar code) on two places - kubevirt-web-ui is still in play until openshift-console meets full feature parity.

See my response in #1592 for more details.

I am happy to move this to console/shared in a subsequent PR and gradually reduce the dependency on web-ui-components.

The idea was that @console/shared contains code used by Console itself as well as other packages. The typical contribution workflow here would be moving code from public or @console/app to @console/shared. Package owners should be inherited from monorepo root OWNERS.

For code not needed by Console itself, we could add a dedicated package, e.g. @console/rhhi-shared (or similar). Common kubevirt/web-ui or kubevirt-web-ui-components code (KubeVirt, Metal3, Storage) could be moved there. Package owners should be declared explicitly, including people who work on said projects.

We plan to move kubevirt-web-ui-components to console/shared or similar packages within one of the last steps of merging web-ui.

I'd create a dedicated package for that, e.g. @console/rhhi-shared suggested above.

(kubevirt-web-ui-components is not KubeVirt only, it also covers Metal3 and Storage.)

I agree that for getting rid of kubevirt/web-ui-components dependency creating of a separate console/rhhi-shared package makes sense as it would contribute to speeding up the process.

On the other hand in ideal world it seems to me that package specific code can live in the package directly (e.g. metal3 specific components) and common ones should align to general use in console and therefore end up in console/shared. But I agree that we're not there yet and it will take time to get there, so the solution you propose is perfectly reasonable.

Regarding Sam's specific comment, these selectors can be used across the whole console codebase so I think it is quite reasonable to put it directly to console/shared.

package specific code can live in the package directly (e.g. metal3 specific components) and common ones should align to general use in console and therefore end up in console/shared

Yes, that's an ideal solution - console-shared changes should pass core team reviews, so any new code we pull from kubevirt/web-ui fork or kubevirt-web-ui-components library would be revisited and aligned with Console core.

We should aim for the best possible solution, so +1 on just going straight through console-shared, bit by bit.

spadgett · 2019-05-31T17:31:11Z

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

+const addressColumnClasses = 'col-lg-2 visible-lg';
+
+const HostHeader: React.FC<React.ComponentProps<typeof ColHead>> = props => (
+  <ListHeader>


Heads up that we're in the process of updating all tables to PF4 (#1465)

@priley86 FYI

spadgett · 2019-05-31T17:32:59Z

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

+  return (
+    <ResourceRow obj={host}>
+      <div className={nameColumnClasses}>
+        <ResourceLink kind={BaremetalHostModel.kind} name={name} namespace={namespace} title={uid} />


I'd remove title={uid}

spadgett · 2019-05-31T17:35:55Z

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

+        <ResourceLink kind={BaremetalHostModel.kind} name={name} namespace={namespace} title={uid} />
+      </div>
+      <div className={statusColumnClasses}>
+        {/* <WithResources resourceMap={machineName ? hostResourceMap : {}}>


I'd remove the empty table columns and add them back when we're ready. We try to keep master as close to production quality as we can.

spadgett · 2019-05-31T17:37:02Z

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

+//   },
+// ];
+
+/* eslint-disable no-unused-vars, no-undef */


You should be able to remove this comment.

spadgett · 2019-05-31T17:37:09Z

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

+// ];
+
+/* eslint-disable no-unused-vars, no-undef */
+export type BaremetalHostsPageProps = {


Unnecessary export

spadgett · 2019-05-31T17:38:49Z

frontend/packages/metal3-plugin/src/components/machine-cell.tsx

+        title={machineName}
+      />
+    );
+  } else if (canHostAddMachine(host)) {


Unnecessary else since you return inside the if

spadgett · 2019-05-31T17:42:17Z

frontend/packages/metal3-plugin/src/components/machine-cell.tsx

+    const ref = referenceForModel(MachineModel);
+    const href = `/k8s/ns/${namespace || 'default'}/${ref}/new`;
+    return (
+      <Link to={href}>


Use RequireCreatePermission or another RBAC check.

spadgett · 2019-06-03T17:33:16Z

/retest

spadgett · 2019-06-03T17:33:42Z

frontend/packages/metal3-plugin/OWNERS

+  - jtomasek
+  - honza
+  - flofuchs
+


nit: extra newline

ugh, @vojtechszocs suggested adding it in the last comment :) #1539 (comment)

spadgett · 2019-06-03T17:36:05Z

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

+// const statusColumnClasses = 'col-lg-2 col-md-4 hidden-sm hidden-xs';
+const machineColumnClasses = 'col-lg-3 visible-lg';
+// const roleColumnClasses = 'col-lg-2 visible-lg';
+const addressColumnClasses = 'col-lg-2 visible-lg';


I know this temporary, but columns no longer add up to 12. You might add a column like created to fill things out until we are able to add the others.

I've added common column class for the 3 current columns and will adjust it back once remaining columns are added.

spadgett · 2019-06-03T17:37:58Z

frontend/packages/metal3-plugin/src/selectors/index.ts

+export const isHostOnline = host => _.get(host, 'spec.online', false);
+export const getHostNics = host => _.get(host, 'status.hardware.nics', []);
+export const getHostStorage = host => _.get(host, 'status.hardware.storage', []);
+export const getHostCpus = host => _.get(host, 'status.hardware.cpus', []);


Suggested change

export const getHostCpus = host => _.get(host, 'status.hardware.cpus', []);

export const getHostCPUs = host => _.get(host, 'status.hardware.cpus', []);

spadgett · 2019-06-03T17:38:17Z

frontend/packages/metal3-plugin/src/selectors/index.ts

+export const getHostMachineName = host => _.get(host, 'spec.machineRef.name');
+export const getHostBmcAddress = host => _.get(host, 'spec.bmc.address');
+export const isHostOnline = host => _.get(host, 'spec.online', false);
+export const getHostNics = host => _.get(host, 'status.hardware.nics', []);


Suggested change

export const getHostNics = host => _.get(host, 'status.hardware.nics', []);

export const getHostNICs = host => _.get(host, 'status.hardware.nics', []);

spadgett · 2019-06-03T17:38:25Z

frontend/packages/metal3-plugin/src/selectors/index.ts

+export const getOperationalStatus = host => _.get(host, 'status.operationalStatus');
+export const getProvisioningState = host => _.get(host, 'status.provisioning.state');
+export const getHostMachineName = host => _.get(host, 'spec.machineRef.name');
+export const getHostBmcAddress = host => _.get(host, 'spec.bmc.address');


Suggested change

export const getHostBmcAddress = host => _.get(host, 'spec.bmc.address');

export const getHostBMCAddress = host => _.get(host, 'spec.bmc.address');

spadgett · 2019-06-03T17:38:35Z

frontend/packages/metal3-plugin/src/selectors/index.ts

+export const getHostNics = host => _.get(host, 'status.hardware.nics', []);
+export const getHostStorage = host => _.get(host, 'status.hardware.storage', []);
+export const getHostCpus = host => _.get(host, 'status.hardware.cpus', []);
+export const getHostRam = host => _.get(host, 'status.hardware.ramGiB');


Suggested change

export const getHostRam = host => _.get(host, 'status.hardware.ramGiB');

export const getHostRAM = host => _.get(host, 'status.hardware.ramGiB');

jtomasek · 2019-06-04T06:45:57Z

/retest

jtomasek · 2019-06-04T08:11:43Z

frontend/packages/console-app/package.json

@@ -7,7 +7,8 @@
  "dependencies": {
    "@console/internal": "0.0.0-fixed",
    "@console/plugin-sdk": "0.0.0-fixed",
-    "@console/shared": "0.0.0-fixed"
+    "@console/shared": "0.0.0-fixed",
+    "@console/metal3-plugin": "0.0.0-fixed"


This needs to be removed

jtomasek · 2019-06-04T10:20:21Z

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

+  </ListHeader>
+);
+
+const HostRow = ({ obj: host }: React.ComponentProps<typeof ResourceRow>) => {


I had to switch from using React.FC as it did not satisfy react/prop-types lint rule.
(getting error 'obj' is missing in props validation react/prop-types)

vojtechszocs

This PR and #1592 both add kubevirt-web-ui-components dependency (hopefully with the same version resolution) so there will likely be conflicts in yarn.lock changes.

Jirka, please see my inline comments.

vojtechszocs · 2019-06-04T15:42:36Z

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

+  // getHostMachineName,
+  getHostBmcAddress,
+  getName,
+  getNamespace,


I am happy to move this to console/shared in a subsequent PR and gradually reduce the dependency on web-ui-components.

The idea was that @console/shared contains code used by Console itself as well as other packages. The typical contribution workflow here would be moving code from public or @console/app to @console/shared. Package owners should be inherited from monorepo root OWNERS.

For code not needed by Console itself, we could add a dedicated package, e.g. @console/rhhi-shared (or similar). Common kubevirt/web-ui or kubevirt-web-ui-components code (KubeVirt, Metal3, Storage) could be moved there. Package owners should be declared explicitly, including people who work on said projects.

We plan to move kubevirt-web-ui-components to console/shared or similar packages within one of the last steps of merging web-ui.

I'd create a dedicated package for that, e.g. @console/rhhi-shared suggested above.

(kubevirt-web-ui-components is not KubeVirt only, it also covers Metal3 and Storage.)

vojtechszocs · 2019-06-04T15:45:13Z

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

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


See my comment on KubeVirt models - shouldn't we use crd: true here?

@spadgett In kubevirt/web-ui fork, any additional models are declared without the crd flag, which is a mistake IMO.

I agree, although when I add the crd flag, I am getting kindObj: no model for kind BareMetalHost in the console and the ListPage won't show up. It seems like there is a mismatch in keys of the resources in redux store and how crd resources are retrieved.

Fixed the problem and added crd: true flag to the model.

It seems like there is a mismatch in keys of the resources in redux store and how crd resources are retrieved.

There's currently a duality in how Console references models. Non-CRD models are typically referenced only by their kind, CRD models are referenced via referenceForModel function that takes their apiGroup, apiVersion and kind into account.

Getting kindObj: no model for kind BareMetalHost could mean you aren't referencing your CRD model via referenceForModel function.

vojtechszocs · 2019-06-04T15:48:10Z

frontend/packages/metal3-plugin/src/plugin.ts

+  {
+    type: 'ModelDefinition',
+    properties: {
+      models: [BaremetalHostModel],


OK, I'm fine with that.

vojtechszocs · 2019-06-04T21:22:09Z

@jtomasek #1668 changes resource page extension types, please adjust accordingly:

ResourcePage/List => Page/Resource/List
ResourcePage/Detail => Page/Resource/Details

jtomasek · 2019-06-05T20:53:01Z

@jtomasek #1668 changes resource page extension types, please adjust accordingly:

ResourcePage/List => Page/Resource/List

ResourcePage/Detail => Page/Resource/Details

Done

jtomasek · 2019-06-06T09:16:24Z

/retest

spadgett · 2019-06-06T13:40:44Z

frontend/packages/metal3-plugin/src/components/machine-cell.tsx

+    const ref = referenceForModel(MachineModel);
+    const href = `/k8s/ns/${namespace || 'default'}/${ref}/new`;
+    return (
+      <RequireCreatePermission model={MachineModel} namespace={namespace}>


This doesn't handle the all namespaces case correctly

spadgett · 2019-06-06T13:44:51Z

frontend/yarn.lock

@@ -8021,6 +8085,21 @@ kind-of@^6.0.0, kind-of@^6.0.2:
  version "6.0.2"
  resolved "https://registry.yarnpkg.com/kind-of/-/kind-of-6.0.2.tgz#01146b36a6218e64e58f3a8d66de5d7fc6f6d051"

+kubevirt-web-ui-components@^0.1.34-2:


I'm concerned with all the additional dependencies this is bringing in. It looks like it includes some alternative libraries to things we've already included in console. Is this only being used for the getName / getNamespace / getUid helpers?

I realize you use it in the kubevirt fork for other things.

As an alternative, we could move the code we need from kubevirt-web-ui-components to console-shared (or create another, more focused shared package) and align it with current Console dependencies, code (like Firehose vs. WithResources) etc.

Is this only being used for the getName / getNamespace / getUid helpers?

+ a couple of things in machine-cell.tsx

I can quite easily remove this dependency for metal3 plugin in general as it does not have large amount of parts implemented in web-ui-components. For kubevirt plugin It will be more difficult.

With recent change I've removed the kubevirt/web-ui-components dependency and added required selectors to @console/shared package.

vojtechszocs · 2019-06-06T14:22:07Z

frontend/packages/metal3-plugin/package.json

+  "dependencies": {
+    "@console/plugin-sdk": "0.0.0-fixed",
+    "@console/shared": "0.0.0-fixed",
+    "kubevirt-web-ui-components": "^0.1.34-2"


Note: you can evaluate the impact of adding kubevirt-web-ui-components on webpack-generated JS dist code by running yarn analyze which runs the webpack-bundle-analyzer.

vojtechszocs · 2019-06-06T14:31:57Z

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

+  // getHostMachineName,
+  getHostBmcAddress,
+  getName,
+  getNamespace,


package specific code can live in the package directly (e.g. metal3 specific components) and common ones should align to general use in console and therefore end up in console/shared

Yes, that's an ideal solution - console-shared changes should pass core team reviews, so any new code we pull from kubevirt/web-ui fork or kubevirt-web-ui-components library would be revisited and aligned with Console core.

We should aim for the best possible solution, so +1 on just going straight through console-shared, bit by bit.

Including basic Baremetal hosts listing page

jtomasek · 2019-06-06T17:15:24Z

Removed kubevirt/web-ui-components dependency.

spadgett · 2019-06-06T18:24:12Z

/retest

spadgett

/lgtm
/approve

openshift-ci-robot · 2019-06-06T19:21:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jtomasek, 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-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 7, 2019

openshift-ci-robot requested review from alecmerdler and jeff-phillips-18 May 7, 2019 11:28

jtomasek commented May 7, 2019

View reviewed changes

spadgett requested changes May 7, 2019

View reviewed changes

vojtechszocs suggested changes May 7, 2019

View reviewed changes

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 13, 2019

spadgett changed the base branch from master-next to master May 13, 2019 22:45

spadgett added this to the v4.2 milestone May 27, 2019

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 28, 2019

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

jtomasek changed the title ~~[WIP] Introduce Metal3 plugin for openshift console~~ Introduce Metal3 plugin for openshift console May 28, 2019

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 May 28, 2019

jtomasek mentioned this pull request May 28, 2019

Support models declared by plugins #1586

Merged

mareklibra reviewed May 30, 2019

View reviewed changes

jtomasek mentioned this pull request May 30, 2019

Add WithResources to shared components #1636

Closed

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 May 31, 2019

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 31, 2019

vojtechszocs suggested changes May 31, 2019

View reviewed changes

spadgett reviewed May 31, 2019

View reviewed changes

spadgett reviewed Jun 3, 2019

View reviewed changes

jtomasek commented Jun 4, 2019

View reviewed changes

vojtechszocs suggested changes Jun 4, 2019

View reviewed changes

spadgett reviewed Jun 6, 2019

View reviewed changes

vojtechszocs reviewed Jun 6, 2019

View reviewed changes

Add initial metal3 plugin

e1d1ef7

Including basic Baremetal hosts listing page

spadgett approved these changes Jun 6, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Jun 6, 2019

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

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

openshift-merge-robot merged commit 30c16b7 into openshift:master Jun 6, 2019

jtomasek mentioned this pull request Jun 13, 2019

Add Status component to @console/shared #1697

Merged

2 tasks

	export const getHostCpus = host => _.get(host, 'status.hardware.cpus', []);
	export const getHostCPUs = host => _.get(host, 'status.hardware.cpus', []);

	export const getHostNics = host => _.get(host, 'status.hardware.nics', []);
	export const getHostNICs = host => _.get(host, 'status.hardware.nics', []);

	export const getHostBmcAddress = host => _.get(host, 'spec.bmc.address');
	export const getHostBMCAddress = host => _.get(host, 'spec.bmc.address');

	export const getHostRam = host => _.get(host, 'status.hardware.ramGiB');
	export const getHostRAM = host => _.get(host, 'status.hardware.ramGiB');

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

		export const BaremetalHostModel: K8sKind = {

Introduce Metal3 plugin for openshift console #1539

Introduce Metal3 plugin for openshift console #1539

Conversation

jtomasek commented May 7, 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

vojtechszocs May 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett May 7, 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

vojtechszocs commented May 7, 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

vojtechszocs commented May 7, 2019

spadgett commented May 7, 2019

vojtechszocs commented May 7, 2019

spadgett commented May 8, 2019

vojtechszocs commented May 17, 2019

vojtechszocs commented May 24, 2019

jtomasek commented May 28, 2019

mareklibra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs commented May 31, 2019

vojtechszocs 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

spadgett commented May 31, 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

jtomasek commented May 7, 2019 •

edited

Loading

vojtechszocs May 13, 2019 •

edited

Loading

spadgett May 7, 2019 •

edited

Loading