Add users list to vm details page #5698

yaacov · 2020-06-09T12:28:13Z

Add users list to vm details

Design:
http://openshift.github.io/openshift-origin-design/designs/virtualization/4.x/expose-guest-data/expose%20guest%20data.html#details-tab

Screenshot:
two users is logged in

guest agent is not installed

no logged in users

vm is not running

load error:

yaacov · 2020-06-09T12:28:35Z

@pcbailey please reveiw

yaacov · 2020-06-09T13:36:22Z

@suomiy @pcbailey @glekner @irosenzw please review

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

pcbailey · 2020-06-09T13:30:34Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+      props: { className: tableColumnClasses[1] },
+    },
+    {
+      title: 'Log in time',


Use "Login time" instead of "Log in time". There's a slight difference between the two. "Log in" is an instruction to someone to log in to the system, with "log" being used as a verb. A "login" is a noun and refers to either the credentials used to log in to a system or a specific instance where a user has logged in.

hmm.. thanks !
did look a little off to me didn't know why :-)

pcbailey · 2020-06-09T13:32:52Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+
+const UsersTableRow = ({ obj, index, key, style }) => {
+  return (
+    <TableRow id={obj?.metadata.uid} index={index} trKey={key} style={style}>


What isobj? I assume it's user information. I would recommend using a more descriptive name unless there's a reason to leave it generic. Could we use selectors to access the properties of obj?

nice +1, fixed

pcbailey · 2020-06-09T13:33:31Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+        {obj?.metadata?.domain}
+      </TableData>
+      <TableData className={tableColumnClasses[2]}>
+        <Timestamp timestamp={new Date(obj?.metadata?.loginTime).toUTCString()} />


This is much cleaner than the date formatting code you were working with before. =)

pcbailey · 2020-06-09T13:35:13Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+      return (
+        <StatusItem
+          Icon={BlueInfoCircleIcon}
+          message="This VM does not have guest agent installed. Some metrics and management features will not be available."


We should probably add this message text as a constant in one of the strings files so we can use it wherever it's needed.

pcbailey · 2020-06-09T13:37:02Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+    const url = `apis/subresources.kubevirt.io/v1alpha3/namespaces/${namespace}/virtualmachineinstances/${name}/guestosinfo`;
+
+    const usersResult = urlResults.getIn([url, 'data']);
+    const usersResultError = urlResults.getIn([url, 'loadError']);
+
+    const data =
+      usersResult &&
+      usersResult?.userList &&
+      usersResult?.userList.map((u) => ({
+        metadata: {
+          name: u?.userName,
+          domain: u?.domain,
+          loginTime: u?.loginTime && u.loginTime * 1000,
+        },
+      }));
+
+    React.useEffect(() => {
+      watchURL(url);
+      return () => {
+        stopWatchURL(url);
+      };
+    }, [watchURL, stopWatchURL, url]);


I assume we're going to put this code in a centralized location? I can add it to my existing PR if you want, and maybe create a wrapper class to make it easier to work with. What do you think?

@yaacov @pcbailey
Are you sure we should be using withDashboardResources HOC for list which is not inside the dashboard?

We are going to reuse this url call in multiple places?

Having a hook for this would be nice. It would be also nicer if we could remove the HOC altogether and have all the logic in the hook.

@suomiy @yaacov We don't plan on using the dashboard code in the final implementation. We're going to write our own and will likely favor using a hook over a HOC. We're just using the dashboard code as a starting point.

atiratree · 2020-06-09T13:41:33Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+
+    const name = getName(vmi);
+    const namespace = getNamespace(vmi);
+    const url = `apis/subresources.kubevirt.io/v1alpha3/namespaces/${namespace}/virtualmachineinstances/${name}/guestosinfo`;


can we make a function out of this?

atiratree · 2020-06-09T13:51:42Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+    const url = `apis/subresources.kubevirt.io/v1alpha3/namespaces/${namespace}/virtualmachineinstances/${name}/guestosinfo`;
+
+    const usersResult = urlResults.getIn([url, 'data']);
+    const usersResultError = urlResults.getIn([url, 'loadError']);
+
+    const data =
+      usersResult &&
+      usersResult?.userList &&
+      usersResult?.userList.map((u) => ({
+        metadata: {
+          name: u?.userName,
+          domain: u?.domain,
+          loginTime: u?.loginTime && u.loginTime * 1000,
+        },
+      }));
+
+    React.useEffect(() => {
+      watchURL(url);
+      return () => {
+        stopWatchURL(url);
+      };
+    }, [watchURL, stopWatchURL, url]);


@yaacov @pcbailey
Are you sure we should be using withDashboardResources HOC for list which is not inside the dashboard?

We are going to reuse this url call in multiple places?

Having a hook for this would be nice. It would be also nicer if we could remove the HOC altogether and have all the logic in the hook.

atiratree · 2020-06-09T13:54:48Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+      usersResult &&
+      usersResult?.userList &&
+      usersResult?.userList.map((u) => ({
+        metadata: {


I know we do this for name sorting, but still it is a bit weird to wrap everything in metadata. I wonder if we can do something about it.

ahhh, forgot to do the sorting ... addning sort ( I knew the metadata has a reason :-) )

added the missing sort

atiratree · 2020-06-09T13:56:55Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+
+const tableColumnClasses = [
+  classNames('col-lg-3', 'col-md-3', 'col-sm-4', 'col-xs-6'),
+  classNames('col-lg-3', 'col-md-3', 'col-sm-4', 'hidden-xs'),


What is the domain? Is it a good decision to hide it for smaller devices?

Shouldn't we switch the priority and show the Login time instead of Elapsed login time on smaller devices? Especially, because there is timezone in Login time

What is the domain?

domain is the ip address user logged in from, if we need to choose logged in time and domain, I'll choose logged in time

Shouldn't we switch the priority and show the Login time instead of Elapsed login time on smaller devices? Especially, because there is timezone in Login time

make sense to me too, switching

p.s.
@matthewcarleton in the designs do we have a way to specify how things look on small screens ?

domain is the ip address user logged in from, if we need to choose logged in time and domain, I'll choose logged in time

I see, I vote for having user | domain | login time visible every time

ok :-) fixing

What is the domain?

domain is the ip address user logged in from, if we need to choose logged in time and domain, I'll choose logged in time

Shouldn't we switch the priority and show the Login time instead of Elapsed login time on smaller devices? Especially, because there is timezone in Login time

make sense to me too, switching

p.s.
@matthewcarleton in the designs do we have a way to specify how things look on small screens ?

If we are using a PF4 component it should be good for mobile right? Is there anything unique about this that doesn't just use a default PF table?

If we are using a PF4 component it should be good for mobile right? Is there anything unique about this that doesn't just use a default PF table?

@matthewcarleton
yes, the question here is witch columns should magically disappear on small displays.
The discussion above is: should we show 3 or only 2 columns on extra small display.

for example we decided here to show 3 columns ( removed the elapsed time ):

sorry I didn't get this before it merged but we should be using a modifier class to get the responsive state.
If you use pf-m-grid-md you won't need to remove that column for smaller screens b/c it will stack.

yaacov · 2020-06-09T16:03:05Z

@pcbailey @suomiy I addressed all the issues except the replacement for the withDashboardResources can you re-review ?

matthewcarleton · 2020-06-09T17:46:44Z

Add users list to vm details

Design:
http://openshift.github.io/openshift-origin-design/designs/virtualization/4.x/expose-guest-data/expose%20guest%20data.html#details-tab

Screenshot:
two users is logged in

What's the reason for the Login time being blue?

guest agent is not installed

This text could say
This virtual machine does not have the guest agent installed, therefor, user data can not be reported.

no logged in users

The text for "No Logged in Users" should probably look the same as the empty state for the services empty state above (centered, and darker)

vm is not running

Same here for that empty state

yaacov · 2020-06-09T17:55:16Z

What's the reason for the Login time being blue?

Patternfly table makes the sorted column blue, see:
https://www.patternfly.org/v4/documentation/react/components/table#sortable

yaacov · 2020-06-09T17:55:29Z

/test e2e-gcp-console

yaacov · 2020-06-09T18:11:18Z

The text for "No Logged in Users" should probably look the same as the empty state for the services empty state above (centered, and darker)

fixed :-)

yaacov · 2020-06-09T20:39:10Z

@suomiy @pcbailey removed the 'withDashboardResources' 🎉 , please re-review

rawagner · 2020-06-09T20:43:11Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

@@ -0,0 +1,152 @@
+import * as React from 'react';
+import { connect } from 'react-redux';
+import { watchURL, stopWatchURL } from '@console/internal/actions/dashboards';


dont use dashboards stuff outside of dashboards. It would be good to create a new hook for polling some url. You can take a look at https://github.com/openshift/console/blob/master/frontend/public/components/graphs/prometheus-poll-hook.ts - create something generic and reuse it also in prometheus-poll-hook

Thanks ! using pool hook now

rawagner · 2020-06-09T21:26:52Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+import { VMIKind } from '../../types';
+
+const guestAgentURL = (name: string, namespace: string) =>
+  `apis/subresources.kubevirt.io/v1alpha3/namespaces/${namespace}/virtualmachineinstances/${name}/guestosinfo`;


There's already logic for creating subresources url such as https://github.com/openshift/console/blob/master/frontend/packages/kubevirt-plugin/src/selectors/vmi/combined.ts#L60

you should try to reuse that

I think it's fixed now :-)

rawagner · 2020-06-09T21:28:19Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-users.tsx

+    }));
+
+  // eslint-disable-next-line react-hooks/rules-of-hooks
+  React.useEffect(() => {


this is a coditional usage of hook. It needs to be moved on top before all if/returns

yaacov · 2020-06-10T03:56:55Z

/test e2e-gcp-console

rawagner · 2020-06-10T07:01:34Z

frontend/public/components/graphs/prometheus-poll-hook.ts

 const DEFAULT_SAMPLES = 60;
 const DEFAULT_TIMESPAN = 60 * 60 * 1000; // 1 hour

 export const usePrometheusPoll = ({
-  delay = DEFAULT_DELAY,
+  delay = URL_POLL_DEFAULT_DELAY,


no need to define the default value as url-poll hook does that.

Suggested change

delay = URL_POLL_DEFAULT_DELAY,

delay,

rawagner · 2020-06-10T07:29:03Z

frontend/public/components/utils/use-url-hook.ts

+
+export const URL_POLL_DEFAULT_DELAY = 15000; // 15 seconds
+
+export const useURLPoll = (


we could improve the types with some generics. Like defining the return value to not be any

Suggested change

export const useURLPoll = (

export const useURLPoll = <R>(

url: string,

delay = URL_POLL_DEFAULT_DELAY,

...dependencies: any[]

): URLPoll<R> => {

rawagner · 2020-06-10T07:29:21Z

frontend/public/components/utils/use-url-hook.ts

+  return { response, error, loading };
+};
+
+export type URLPoll = {


Suggested change

export type URLPoll = {

export type URLPoll<R> = [ R, any, boolean ];

rawagner · 2020-06-10T07:29:47Z

frontend/public/components/utils/use-url-hook.ts

+
+  usePoll(tick, delay, ...dependencies);
+
+  return { response, error, loading };


I'd prefer to return an array to align with other hooks

Suggested change

return { response, error, loading };

return [ response, error, loading ];

rawagner · 2020-06-10T07:30:16Z

frontend/public/components/utils/use-url-hook.ts

@@ -0,0 +1,46 @@
+/* eslint-disable react-hooks/exhaustive-deps */
+import { useCallback, useState } from 'react';
+import { usePoll, useSafeFetch } from '.';


please dont import from index file, it causes circular deps

rawagner · 2020-06-10T07:31:00Z

frontend/public/components/utils/index.tsx

@@ -59,3 +59,4 @@ export { default } from './operator-backed-owner-references';
 export * from './field-level-help';
 export * from './details-item';
 export * from './types';
+export * from './use-url-hook';


lets not add this hook to index file. We want to remove all indexes as they cause issues due to circular deps

rawagner · 2020-06-10T07:31:36Z

frontend/public/components/graphs/prometheus-poll-hook.ts

-  }, [url]);
-
-  usePoll(tick, delay, endTime, query, timespan);
+  const { response, error, loading } = useURLPoll(url, delay, endTime, query, timespan);


if you use array format and generics for useURLPoll use can do

Suggested change

const { response, error, loading } = useURLPoll(url, delay, endTime, query, timespan);

return useURLPoll<PrometheusResponse>(url, delay, endTime, query, timespan);

rawagner · 2020-06-10T07:33:02Z

frontend/public/components/utils/use-url-hook.ts

@@ -0,0 +1,46 @@
+/* eslint-disable react-hooks/exhaustive-deps */


to align with other hooks we should call this file url-poll-hook

rawagner · 2020-06-10T07:33:30Z

frontend/public/components/utils/use-url-hook.ts

+  ...dependencies: any[]
+): URLPoll => {
+  const [error, setError] = useState();
+  const [response, setResponse] = useState();


Suggested change

const [response, setResponse] = useState();

const [response, setResponse] = useState<R>();

rawagner · 2020-06-10T09:02:13Z

/lgtm

openshift-ci-robot · 2020-06-10T09:02:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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 the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2020

openshift-ci-robot requested review from rawagner and vojtechszocs June 9, 2020 12:28

openshift-ci-robot added component/kubevirt Related to kubevirt-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 9, 2020

yaacov force-pushed the add-user-list branch 2 times, most recently from 1f07213 to d625935 Compare June 9, 2020 12:58

yaacov changed the title ~~[WIP] Add users list to vm details page~~ Add users list to vm details page Jun 9, 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 Jun 9, 2020

yaacov force-pushed the add-user-list branch from d625935 to f6ae03e Compare June 9, 2020 13:24

pcbailey reviewed Jun 9, 2020

View reviewed changes

atiratree reviewed Jun 9, 2020

View reviewed changes

yaacov force-pushed the add-user-list branch 4 times, most recently from f23851a to 69bd9d1 Compare June 9, 2020 14:35

yaacov force-pushed the add-user-list branch from f465427 to c67dc41 Compare June 9, 2020 20:40

rawagner suggested changes Jun 9, 2020

View reviewed changes

yaacov force-pushed the add-user-list branch from c67dc41 to 398d726 Compare June 10, 2020 06:38

openshift-ci-robot added the component/core Related to console core functionality label Jun 10, 2020

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2020

rawagner suggested changes Jun 10, 2020

View reviewed changes

add users list to vm details page

315c8c4

yaacov force-pushed the add-user-list branch from 6eb8a43 to 315c8c4 Compare June 10, 2020 08:41

openshift-ci-robot assigned rawagner Jun 10, 2020

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

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

openshift-merge-robot merged commit 2aae8b3 into openshift:master Jun 10, 2020

spadgett added this to the v4.6 milestone Jun 15, 2020


		export const URL_POLL_DEFAULT_DELAY = 15000; // 15 seconds

		export const useURLPoll = (

	export type URLPoll = {
	export type URLPoll<R> = [ R, any, boolean ];


		usePoll(tick, delay, ...dependencies);

		return { response, error, loading };

	return { response, error, loading };
	return [ response, error, loading ];

	const { response, error, loading } = useURLPoll(url, delay, endTime, query, timespan);
	return useURLPoll<PrometheusResponse>(url, delay, endTime, query, timespan);

		@@ -0,0 +1,46 @@
		/* eslint-disable react-hooks/exhaustive-deps */

	const [response, setResponse] = useState();
	const [response, setResponse] = useState<R>();

Add users list to vm details page #5698

Add users list to vm details page #5698

Conversation

yaacov commented Jun 9, 2020 • edited

yaacov commented Jun 9, 2020

yaacov commented Jun 9, 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

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

yaacov Jun 9, 2020 • edited

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

yaacov commented Jun 9, 2020

matthewcarleton commented Jun 9, 2020

yaacov commented Jun 9, 2020

yaacov commented Jun 9, 2020

yaacov commented Jun 9, 2020

yaacov commented Jun 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawagner Jun 10, 2020 • edited

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

rawagner Jun 10, 2020 • edited

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

rawagner commented Jun 10, 2020

openshift-ci-robot commented Jun 10, 2020

yaacov commented Jun 9, 2020 •

edited

yaacov Jun 9, 2020 •

edited

rawagner Jun 10, 2020 •

edited

rawagner Jun 10, 2020 •

edited