Kubevirt VmDetails #1682

mareklibra · 2019-06-07T13:08:30Z

Introduces Details Page for Virtual Machines.

Depends on:

openshift-ci-robot · 2019-06-07T13:08:44Z

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

mareklibra · 2019-06-07T13:13:46Z

frontend/packages/_plugin-style.scss

@@ -0,0 +1,4 @@
+/* Styles of all active plugins */


Simple, but working.
See additional comments for more details.

Can I ask for comments here, please? @spadgett , @vojtechszocs

This can be removed in favor of importing style from plugin.ts.

@mareklibra Sorry for responding late.

Plugin-specific styles can be loaded as a side effect of loading the plugin's entry module, e.g. in kubevirt-plugin/src/plugin.ts:

import './style.scss';

I see you've already updated the PR to do that, so 👍 on that.

@mareklibra Please note, once #1708 is merged and you enable the kubevirt-plugin & run tests (yarn test), we'll need to ensure Jest properly mocks scss files. Right now, the relevant module mapper config is:

"\\.(css|less)$": "<rootDir>/__mocks__/styleMock.js"

So we'll have to add scss there ^^.

So we'll have to add scss there ^^.

This is addressed by #1721 (commit).

mareklibra · 2019-06-07T13:19:24Z

To achieve full encapsulation of plugins, styles should be extendable as well.
We can handle that on plugin's package.json level with modification to webpack.
@vojtechszocs , wdyt?

If such an extension point is considered as waste of effort, we can go with the simple solution proposed here:

if a plugin needs custom styling, there will be single entry src/style.scss in its root
frontend/packages/_plugin-style.scss is manually maintained to register all styling of active plugins

So to (de)activate a plugin, one-line modification of frontend/packages/_plugin-style.scss and console-app/package.json is required

mareklibra · 2019-06-10T09:00:33Z

@spadgett , can I ask you for review here, please?

atiratree · 2019-06-11T10:24:42Z

frontend/packages/kubevirt-plugin/src/components/vm-details.tsx

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


Can we move this to ./vm/ folder? - to distinguish between vm and vm template files in the future

👍 public/components/* seem to use the plural for directories, so I'd suggest using components/vms and components/vm-templates here for better organization.

atiratree · 2019-06-11T10:25:35Z

frontend/packages/kubevirt-plugin/src/components/constants.js

@@ -0,0 +1 @@
+export const DASHES = '---';


this should probably go to console-shared

We use a single dash, -, throughout console. Is there a reason it's --- here?

IIRC, this was requested during UX review some time ago.
But I agree, it makes sense to unify and potentially change later everywhere.

One more change: the DASHES are imported from kubevirt-web-ui-components now, so triple dash --- is still used per former UXD review. This way it will be consistent across Virtual Machines pages and can be unified with OpenShift when moving kubevirt-web-ui-components under console. Or if it is a blocker, I can fix there and fire new release. @spadgett , are you ok with that, please?

Sorry, I disagree. We should change it everywhere in console or not at all. It sounds like we reviewed one page and didn't consider any of the existing pages that already have a dash. IMO, the right thing to do is fix kubevrit-web-ui-components so that it's consistent with existing console pages.

We're adding kubevirt as a plugin is so that it feels like an integrated part of console. This might seem like a little thing, but it undercuts that goal. The kubevirt pages should look and behave like other console pages.

For the record, I prefer a single dash -. But I'm most concerned about keeping consistency.

Can't we simply use the shared package here?

// packages/console-shared/src/constants/index.ts export const DASH = '-';

atiratree · 2019-06-11T10:31:46Z

frontend/packages/kubevirt-plugin/src/components/vm-details.tsx

+  const { name, namespace } = vm.metadata;
+
+  const resources = [
+    getResource(VirtualMachineInstanceModel, { name, namespace, isList: false }),


we could probably move this method here rather sooner than later, because it is used in many places.

note: it builds requested resource object with reasonable defaults

And it appeared that it can be even farther enhanced. As discussed before, it will be moved along the rest of kubevirt-web-ui-components.

atiratree · 2019-06-11T10:42:43Z

frontend/packages/kubevirt-plugin/src/utils/resources.ts

@@ -0,0 +1,4 @@
+import * as _ from 'lodash-es';
+
+export const getLoadedResource = (props, kind) =>


This kind of functionality should be probably included in the firehose.
Many components do not care about the loaded prop. They just need the data or undefined/null if the data is not available yet.

note: previously implemented by WithResources.

Components can choose to handle !loaded the same as no data, but that should be up to the component. It's a worse API not to have an explicit loaded prop. In most cases, you should handle it as well as handle errors.

I am not suggesting to make any regression to the API. It is enough Just to enhance it, possibly by adding some flags to the resource (or the whole firehose).

Many components need just the raw data and this would make it easier for them to have less logic or a need to specify getLoadedResource for each resource (like this PR).

In most cases, components should explicitly handle loaded. Changing the API to make it easy to ignore seems like a bad change.

I also think getLoadedResource is a not a useful helper because it assumes the property of the data. The prop is not always the kind. It would be better to remove it from the PR.

Recently I try to avoid WithResources in favour of just Firehose. There will be probably a small change to Firehose needed when migrating later features, but it's not a recent concern.

The purpose of this getLoadedResource function is to provide simple mapping to meet contract with existing component. So far, similar logic was handled via WithResources which took care of error handling and potentially passed empty data to the component or tentatively rendered a loader component.

I can improve components in kubevirt-web-ui-components to deal with the Firehose state machine explicitly. But it's extra complexity as I can't break the "safe-path" for potentially releasing web-ui even in 4.2 timeframe.

I will try, but definitely I'll definitely need to create new release of web-ui-components for that.

When looking into the code once again, I think it's better to enhance Firehose (see #1711) and we can move checks to children components consistently with the rest of console. Please have a look at recent changes in this PR.

spadgett

@mareklibra Can you add a screenshot?

spadgett · 2019-06-11T12:52:39Z

frontend/packages/kubevirt-plugin/src/components/constants.js

@@ -0,0 +1 @@
+export const DASHES = '---';


We use a single dash, -, throughout console. Is there a reason it's --- here?

spadgett · 2019-06-13T12:49:48Z

This page does not look or behave like other console pages. This goes against the goal of making kubevirt feel like an integrated, first-class part of console.

It doesn't use the standard ResourceSummary component on every other detail page
It uses more columns than every other details page
Empty values should be a single - with text-muted style
The Edit buttons should be a menu item in an actions dropdown

What does the floating (?) icon do?

lizsurette · 2019-06-13T14:46:44Z

This page does not look or behave like other console pages. This goes against the goal of making kubevirt feel like an integrated, first-class part of console.

@mareklibra Here is our updated design based on feedback from @beanh66 and the OpenShift design team...

You can track the larger design PR if needed, too.

FYI @matthewcarleton

spadgett · 2019-06-13T15:01:02Z

Thanks @lizsurette. Looks good!

Note that ResourceSummary forces a certain layout of the first few fields for consistency across pages. So the order of the items in the left column might change slightly. (Namespace is always directly under name for instance.)

lizsurette · 2019-06-13T15:15:05Z

Note that ResourceSummary forces a certain layout of the first few fields for consistency across pages. So the order of the items in the left column might change slightly. (Namespace is always directly under name for instance.)

That makes sense to me @spadgett. FYI @matthewcarleton

matthewcarleton · 2019-06-13T18:41:34Z

I've suggested on the updated design PR that we include designs around the inline edit rather than just omitting the bulk for clarity. @spadgett @lizsurette

mareklibra · 2019-06-14T06:46:15Z

@lizsurette , @spadgett , thanks for the review.

Recent design conforms web-ui. Original plan was to move our fork to upstream as it is with just minor necessary modifications. Improvements were supposed to be done in follow-ups to not delay the effort more then necessary while targeting full feature parity for 4.2.

I agree that the pages should look and behave consistently and especially changes on upstream side should be reflected here.
I will adapt this PR accordingly. Anyway, I suggest to implement new features (like the charts) still in follow-ups.

@jelkosz, FYI., this will take a little bit more time then originally expected.

jelkosz · 2019-06-14T06:54:36Z

I will adapt this PR accordingly. Anyway, I suggest to implement new features (like the charts) still in follow-ups.

this makes sense to me! Redesign to look like the rest of the pages but without adding new features is a good approach.

FYI., this will take a little bit more time then originally expected.

:(

vojtechszocs · 2019-06-14T18:30:36Z

To achieve full encapsulation of plugins, styles should be extendable as well.
We can handle that on plugin's package.json level with modification to webpack.
@vojtechszocs , wdyt?

If such an extension point is considered as waste of effort, we can go with the simple solution proposed here:

if a plugin needs custom styling, there will be single entry src/style.scss in its root

frontend/packages/_plugin-style.scss is manually maintained to register all styling of active plugins

So to (de)activate a plugin, one-line modification of frontend/packages/_plugin-style.scss and console-app/package.json is required

Again, sorry for responding late. Based on discussion with @christianvogt I think the best approach at the moment is to import the necessary stylesheets within the plugin's entry module.

Enabling a plugin shouldn't involve more steps beyond updating console-app's dependencies.

Ideally, plugin entry modules should be side-effect-free and only export the list of extensions, because they are loaded early and therefore impact the initial chunk size. We can revisit this in future and have a better separation.

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

frontend/packages/kubevirt-plugin/src/components/vm-details.tsx

+  ];
+
+  const breadcrumbsFor = (obj) =>
+    breadcrumbsForOwnerRefs(obj).concat({


note: this is changing in #1725

Rebased on top of that and updated accordingly

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

frontend/packages/kubevirt-plugin/src/types.ts

+import { K8sResourceKind, ObjectMetadata } from '@console/internal/module/k8s';
+
+// https://kubevirt.io/api-reference/master/definitions.html#_v1_virtualmachineinstancespec
+export type VmiSpec = {


Per console naming conventions, acronyms should be caps like

Suggested change

export type VmiSpec = {

export type VMISpec = {

Here and below

done, changed on multiple places

spadgett · 2019-06-14T19:58:22Z

frontend/packages/kubevirt-plugin/src/components/vm-details.tsx

+      kind={PodModel.kind}
+      name={getName(pod)}
+      namespace={getNamespace(pod)}
+      uid={pod.metadata.uid}


I don't see a uid prop used by ResourceLink?

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

frontend/packages/kubevirt-plugin/src/components/vm-details.tsx

+  const { vm } = props;
+
+  const namespaceResourceLink = () => (
+    <ResourceLink kind={NamespaceModel.kind} name={getNamespace(vm)} title={getNamespace(vm)} />


title shouldn't be necessary (unless it's a hover to show the full value when truncated)

spadgett · 2019-06-14T20:00:36Z

frontend/packages/kubevirt-plugin/src/components/vm-details.tsx

+    DASHES
+  );
+
+const VmDetailsConnected = (props: VmDetailsProps) => {


Suggested change

const VmDetailsConnected = (props: VmDetailsProps) => {

const VMDetailsConnected: React.FC<VMDetailsProps> = (props) => {

I'm confused: this is VMDetailsConnected, but I don't see a call to connect(). We should give this a different name if it's not referring to redux connect

spadgett · 2019-06-14T20:01:03Z

frontend/packages/kubevirt-plugin/src/components/vm-details.tsx

+  );
+
+const VmDetailsConnected = (props: VmDetailsProps) => {
+  const { vm } = props;


Consider

Suggested change

const { vm } = props;

const { vm, ...rest } = props;

and using {...rest} below

spadgett · 2019-06-14T20:05:14Z

frontend/packages/kubevirt-plugin/src/components/vm-details.tsx

+    isList: false,
+    props: 'vmi',
+  });
+  vmiRes.ignoreIfMissing = true;


optional

Add this to the declaration. No reason this needs to be on a separate line.

spadgett · 2019-06-14T20:06:55Z

frontend/public/components/utils/firehose.jsx

+    let stuff = k8s.get(reduxID);
+    if (stuff) {
+      stuff = stuff.toJS();
+      stuff.ignoreIfMissing = props.ignoreIfMissing;


This should be removed since #1711 merged

resolved by rebase

jtomasek · 2019-06-19T07:42:01Z

/ok-to-test

atiratree · 2019-06-19T09:59:16Z

Redesigned to fit into openshift design. All review comments have been addressed + any other potential comments can be addressed in followups.

Merging in order to unblock other PRs.

atiratree · 2019-06-19T09:59:26Z

/lgtm

openshift-ci-robot · 2019-06-19T09:59:34Z

@suomiy: changing LGTM is restricted to assignees, and only openshift/console repo collaborators may be assigned issues.

In response to this:

/lgtm

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.

jtomasek · 2019-06-19T10:06:21Z

/assign

jtomasek · 2019-06-19T10:09:08Z

/lgtm

jelkosz · 2019-06-19T10:12:38Z

/assign

openshift-ci-robot · 2019-06-19T10:12:40Z

@jelkosz: GitHub didn't allow me to assign the following users: jelkosz.

Note that only openshift members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign

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.

mareklibra · 2019-06-19T12:18:50Z

Last force-push was done by mistake while rebasing other PRs stacked on top of this. Should be ok now. I'm sorry for confusion.

spadgett

/approve
/lgtm

openshift-ci-robot · 2019-06-20T01:46:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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-20T03:15:39Z

/retest

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 7, 2019

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 7, 2019

openshift-ci-robot requested review from dtaylor113 and TheRealJon June 7, 2019 13:08

mareklibra commented Jun 7, 2019

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 10, 2019

mareklibra changed the title ~~WIP: Kubevirt.vm details~~ Kubevirt VmDetails Jun 10, 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 Jun 10, 2019

mareklibra mentioned this pull request Jun 10, 2019

kubevirt: Add Status column to VmList #1689

Merged

7 tasks

atiratree reviewed Jun 11, 2019

View reviewed changes

spadgett reviewed Jun 11, 2019

View reviewed changes

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

spadgett requested changes Jun 14, 2019

View reviewed changes

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

atiratree mentioned this pull request Jun 17, 2019

VirtualMachine details: add Disks tab #1731

Merged

2 tasks

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 19, 2019

yaacov mentioned this pull request Jun 19, 2019

[WIP]Kubevirt VmTemplateDetails #1754

Closed

1 task

openshift-ci-robot assigned jtomasek Jun 19, 2019

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

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2019

mareklibra added 3 commits June 19, 2019 14:14

Introduce VirtualMachine Details page

baa98ae

VmDetail: redesign to use ResourceSummary

b5a1f6b

Refactor: split VMDetails to separate file

aadc02d

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

mareklibra mentioned this pull request Jun 19, 2019

Kubevirt: port VM list to PF4 Table #1758

Merged

2 tasks

spadgett approved these changes Jun 20, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Jun 20, 2019

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

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

openshift-merge-robot merged commit fdf6b17 into openshift:master Jun 20, 2019

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

		export const getLoadedResource = (props, kind) =>

	const VmDetailsConnected = (props: VmDetailsProps) => {
	const VMDetailsConnected: React.FC<VMDetailsProps> = (props) => {

Kubevirt VmDetails #1682

Kubevirt VmDetails #1682

Conversation

mareklibra commented Jun 7, 2019 • edited Loading

openshift-ci-robot commented Jun 7, 2019

mareklibra Jun 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

vojtechszocs Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

mareklibra commented Jun 7, 2019

mareklibra commented Jun 10, 2019

Choose a reason for hiding this comment

vojtechszocs 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

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

spadgett commented Jun 13, 2019

lizsurette commented Jun 13, 2019

spadgett commented Jun 13, 2019

lizsurette commented Jun 13, 2019

matthewcarleton commented Jun 13, 2019

mareklibra commented Jun 14, 2019

jelkosz commented Jun 14, 2019

vojtechszocs commented Jun 14, 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

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

jtomasek commented Jun 19, 2019

atiratree commented Jun 19, 2019

atiratree commented Jun 19, 2019

openshift-ci-robot commented Jun 19, 2019

jtomasek commented Jun 19, 2019

jtomasek commented Jun 19, 2019

jelkosz commented Jun 19, 2019

openshift-ci-robot commented Jun 19, 2019

mareklibra commented Jun 19, 2019

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 20, 2019

openshift-bot commented Jun 20, 2019

mareklibra commented Jun 7, 2019 •

edited

Loading

mareklibra Jun 7, 2019 •

edited

Loading

vojtechszocs Jun 14, 2019 •

edited

Loading

vojtechszocs Jun 14, 2019 •

edited

Loading

spadgett Jun 14, 2019 •

edited

Loading