Feature/dashboard #211

ewoutp · 2018-07-02T08:12:07Z

This PR lays the groundwork for a dashboard for all operators in kube-arangodb.

This PR implements some of the functionality of the ArangoDeployment operator dashboard.
More functionality will be added in later PRs.
Other operators (replication & storage) will also be handled in other PRs.

pluma · 2018-07-02T16:28:35Z

dashboard/src/App.js

+  }
+
+  render() {
+    setTimeout(this.reloadOperators.bind(this), 5000);


This should happen in componentDidMount. Having side-effects in a render function is generally a bad idea.

Also I think you want to use setInterval as setTimeout is for one-off delayed code.

Also keep in mind setTimeout/setInterval will still keep firing if the component is unmounted. So you need to clean those up in componentWillUnmount.

pluma · 2018-07-02T16:30:26Z

dashboard/src/App.js

+    }
+    return (
+      <Container>
+        {children}


This could be inlined as {this.state.operators.deployment ? <DeploymentOperator/> : <NoOperator/>}.

pluma · 2018-07-02T16:31:50Z

dashboard/src/App.js

+          <Message>
+            <Message.Header>Kube-ArangoDB</Message.Header>
+            <p>
+              Running in Pod <b>{this.state.operators.pod}</b> in namespace <b>{this.state.operators.namespace}</b>.


These will be undefined until the request in componentDidMount resolves. React will render those variables as empty strings in this case but that'll look a bit ugly.

pluma · 2018-07-02T16:42:26Z

dashboard/src/App.js

+          </Message>     
+        </Segment>
+      </Container>
+    );


TBH, I'd probably extract most of the render function into a separate component that's entirely presentational, e.g.

const OperatorsView = ({deployment, pod, namespace}) => ( <Container> {deployment ? <DeploymentOperator /> : <NoOperator />} <Segment basic> <Message> <Message.Header>Kube-ArangoDB</Message.Header> <p>Running in Pod <b>{pod}</b> in namespace <b>{namespace}</b>.</p> </Message> </Segment> </Container> )); class App extends Component { state = {} componentDidMount () { this.reloadOperators(); this._interval = setInterval(this.reloadOperators, 5000); } componentWillUnmount () { clearInterval(this._interval); } reloadOperators = async () => { const operators = await apiGet('...'); this.setState({ operators }); } render () { if (this.state.operators) { return <OperatorsView {...this.state.operators} />; } return <div>Loading...</div>; }

Keep in mind this still doesn't account for timeouts or errors in the async stuff.

FYI the {...this.state.operators} part is just me being lazy because I'm coding in the browser. I'd unpack the props explicitly and pass them individually. The reloadOperators = async () => {...} stuff binds the function to the instance so you don't need to bind it explicitly when creating the timeout/interval. This is generally a good practice when dealing with things like event handlers but not entirely necessary here (just easier to avoid mistakes by doing it here too).

pluma · 2018-07-02T16:43:53Z

dashboard/src/api/api.js

@@ -0,0 +1,8 @@
+// apiGet performs a GET request on the API with given local URL.
+// The result is decoded from JSON and returned.
+export async function apiGet(localURL: string): Promise<Any> {


Here you're using TypeScript syntax but the files are JS files.

If you're going to use Create React App you probably want to use it with the TypeScript react-scripts:
https://github.com/Microsoft/TypeScript-React-Starter#create-our-new-project

BTW if you do use this, make sure to simplify the extends list in the tslint.json it generates:

"extends": ["tslint-config-prettier"],

The defaults are overly pedantic and tend to get in the way.

pluma · 2018-07-02T16:45:23Z

dashboard/src/deployment/DeploymentList.js

+  }
+
+  render() {
+    setTimeout(this.reloadDeployments.bind(this), 5000);


Again see above about side-effects.

pluma · 2018-07-02T16:48:10Z

dashboard/src/deployment/DeploymentList.js

+      return (<div>No deployments</div>);
+    }
+    return (
+      <Table striped celled>


Again, I'd like to keep the presentational part in a presentational component to make it easier to swap out later.

pluma · 2018-07-02T16:49:27Z

dashboard/src/deployment/DeploymentRow.js

+    return (
+      <Table.Row>
+        <Table.Cell><Icon name="bell" color="red"/></Table.Cell>
+        <Table.Cell>{this.props.info.name}</Table.Cell>


No reason to use a class based component unless you have some kind of state. Also I'd unpack the info properties into props directly.

pluma · 2018-07-03T13:37:27Z

dashboard/src/deployment/DeploymentList.js

+  </Table.Header>
+);
+
+const RowView = ({info}) => (


I'd pass in the item's properties individually to make the props more explicit here.

pluma · 2018-07-03T13:38:47Z

dashboard/src/deployment/DeploymentList.js

+    <HeaderView/>
+    <Table.Body>
+      {
+        (items) ? items.map((item) => <RowView key={item.name} info={item}/>) : <p>No items</p>


i.e. here I'd do <RowView key={item.name} name={item.name} mode={item.mode} readyCount={item.ready_pod_count} totalCount={item.pod_count} /> so it's clearer what this takes.

pluma · 2018-07-03T13:40:03Z

dashboard/src/util/Loading.js

+import { Dimmer, Loader, Segment } from 'semantic-ui-react';
+
+class Loading extends Component {
+  render() {


Again, if this doesn't need state or anything special, just use a function component here. Also helps increasing the visibility of what props it takes.

pluma · 2018-07-05T16:03:51Z

dashboard/src/App.js

-    </Segment>
-  </Container>
+  <div>
+    {deployment ? <DeploymentOperator pod-info={<PodInfoView pod={pod} namespace={namespace}/>}/> : <NoOperator />}


BTW React components normally use camelCase for prop names instead of kebab-case as HTML does.

pluma · 2018-07-05T16:04:29Z

dashboard/src/deployment/DeploymentDetails.js

+import Loading from '../util/Loading.js';
+//import CommandInstruction from '../util/CommandInstruction.js';
+
+const MemberGroupsView = ({member_groups}) => (


Here too camelCase would be more idiomatic.

pluma · 2018-07-05T16:05:22Z

dashboard/src/deployment/DeploymentDetails.js

+  <Segment>
+    <Header>{group}</Header>
+    <List divided>
+      {members.map((item) => <MemberView memberInfo={item} active={item.id === activeMemberID} onClick={onClick}/>)}


May want to add key={item.id} to help React diffing renders when the members prop changes.

pluma · 2018-07-05T16:07:37Z

dashboard/src/deployment/DeploymentDetails.js

+  state = {};
+
+  onClick = (id) => { 
+    this.setState({activeMemberID:(this.state.activeMemberID === id) ? null : id}); 


When using this.state to set state, you should use the functional form of setState:

this.setState(({activeMemberID}) => ({ activeMemberID: activeMemberID === id ? null : id }));

Using this.state to derive new state is error-prone as setState is potentially async.

BTW, I'd call the method "handleClick" so you pass onClick={this.handleClick} but I honestly have no idea where that convention comes from.

pluma · 2018-07-05T16:10:31Z

dashboard/src/deployment/DeploymentList.js

      }
    </Table.Body>
  </Table>
 );

 const EmptyView = () => (<div>No deployments</div>);

+function createDeleteCommand(name, namespace) {
+  return `kubectl delete ArangoDeployment -n ${namespace} ${name}`;


If you're not using this in multiple places I'd just inline it.

pluma · 2018-07-05T16:12:35Z

dashboard/src/util/CommandInstruction.js

+import {CopyToClipboard} from 'react-copy-to-clipboard';
+
+class CommandInstruction extends Component {
+  state = {};


In this case I'd initialise state = {open: false} explicitly because having it undefined doesn't really add any meaning distinct from false.

pluma · 2018-07-05T16:13:54Z

dashboard/src/util/Loading.js

        </Dimmer>
-        <div style={{"min-height":"3em"}}/>
+        <div style={{minHeight:"3em"}}/>


For consistency you might want to use styled for this one. Remember emotion also provides a css template handler which simply generates a classname if you don't want to keep track of another component just for the CSS class.

pluma · 2018-07-09T09:50:43Z

dashboard/src/App.js

+import Loading from './util/Loading.js';
+import api from './api/api.js';
+import { Container, Segment, Message } from 'semantic-ui-react';
+import './App.css';


If you're no longer using these styles here, I'd recommend removing this import.

they are (already) being removed in the next PR

pluma · 2018-07-09T09:53:18Z

dashboard/src/NoOperator.js

@@ -0,0 +1,24 @@
+import React, { Component } from 'react';
+import logo from './logo.svg';
+import './App.css';


You may want to replace the external stylesheet with a few styled components to get rid of the CSS file. Right now you have three ways to define styling (semantic-ui, plain CSS and emotion) which seems excessive.

plain css will be gone

pluma · 2018-07-09T09:54:44Z

dashboard/src/api/api.js

+
+var api = new Api();
+
+export default api;


Any reason you're using a "singleton class" instead of simply an object? You can use this in plain object methods too.

pluma · 2018-07-09T09:56:11Z

dashboard/src/auth/Auth.js

+import LogoutContext from './LogoutContext.js';
+import { getSessionItem, setSessionItem } from "../util/Storage.js";
+
+const tokenSessionKey = "auth-token";


Traditionally this would be called TOKEN_SESSION_KEY but that's not a hard rule.

pluma · 2018-07-09T10:00:03Z

dashboard/src/auth/Auth.js

+        authenticated: true,
+        showLoading: false,
+        token: api.token
+      });


If I understand this right, you're setting the global api instance's token property to the one from state, then doing a call and discarding the result, then setting the state's token to that of the api.

Does the token change during the request? Does the request have side-effects? I'm guessing you only use the request to determine whether you're authenticated and authorized.

I'm not really happy with the Auth component and the LogoutContext but I can't think of an easy way to improve on this either, so lgtm 🤷‍♂️

pluma · 2018-07-09T10:03:31Z

dashboard/src/api/api.js

+        if (this.token) {
+            headers['Authorization'] = `bearer ${this.token}`; 
+        }
+        const result = await fetch(localURL, {headers});


Just an FYI because fetch can be a bit too low-level sometimes and have cross-browser surprises: https://yarnpkg.com/en/package/axios

pluma · 2018-07-09T10:05:54Z

dashboard/src/auth/Login.js

+    <Form onSubmit={doLogin}>
+      <Form.Field>
+        <label>Name</label>
+        <input focus="true" value={username} onChange={(e) => usernameChanged(e.target.value)}/>


Conventionally this would be called "onUsernameChange" I think. This also helps indicate it's an event handler rather than e.g. a flag.

pluma · 2018-07-09T10:07:36Z

dashboard/src/deployment/DeploymentDetails.js

+const MemberGroupsView = ({memberGroups, namespace}) => (
+  <div>
+    {memberGroups.map((item) => <MemberList 
+      key={`server-group-${item.group}`}


Any particular reason you're prefixing the keys? They're local to their parent so this doesn't seem to do much.

pluma · 2018-07-09T10:11:40Z

dashboard/src/deployment/DeploymentList.js

+
+class DeploymentList extends Component {
+  state = {
+    items: undefined,


If you need an explicit "this is a thing but it's not set yet" value, null is generally preferable to undefined as it's difficult to distinguish values that exist but are undefined form values that are undefined because they don't exist.

pluma · 2018-07-09T10:18:12Z

dashboard/src/util/Loading.js

+        </Dimmer>
+        <div style={{minHeight:"3em"}}/>
+      </Segment>
+      );


BTW the indenting is a bit messy. Have you heard of our lord and saviour, Prettier? https://prettier.io/

pluma · 2018-07-09T10:19:42Z

None of my comments are blocking. if @neunhoef approves

ewoutp added 2 commits June 29, 2018 15:51

Adding dashboard

da09f17

Merged in master

58e607c

ghost assigned ewoutp Jul 2, 2018

ghost added the 2 - Working label Jul 2, 2018

ewoutp added 5 commits July 2, 2018 12:40

Creating daskboard web-app

001e921

Serve dashboard directly from embedded asset

da6933c

Improving getters

9146c1a

Dashboard UI

06fadff

Updated assets

aa8f92b

pluma reviewed Jul 2, 2018

View reviewed changes

Processed review comments

3b99dae

pluma reviewed Jul 3, 2018

View reviewed changes

ewoutp added 5 commits July 5, 2018 08:30

Minor fixes

791d677

Pass in individual values

7c18cce

View only

7c7849a

Extensing deployment list

81b9802

Adding deployment details view

a65aa3e

pluma reviewed Jul 5, 2018

View reviewed changes

ewoutp added 4 commits July 6, 2018 08:14

Building dashboard in docker

37b5496

Enhancing member view

0018652

init open

5023124

Added authentication

408de54

ewoutp added 4 commits July 6, 2018 14:22

Alpine & status fixes

7de8bc8

Member status

f2274a5

Improved backend polling behavior

06bda8c

Updated assets

bf29d7a

ewoutp added Team Clifton 4 - Review and removed 2 - Working labels Jul 6, 2018

ewoutp requested a review from neunhoef July 6, 2018 15:05

pluma reviewed Jul 9, 2018

View reviewed changes

ewoutp added 2 commits July 9, 2018 13:15

Review comments

de5cd8f

Simplified singleton

c38bb71

ewoutp merged commit 279c736 into master Jul 9, 2018

ghost removed the 4 - Review label Jul 9, 2018

ewoutp deleted the feature/dashboard branch July 9, 2018 12:10

Feature/dashboard #211

Feature/dashboard #211

Conversation

ewoutp commented Jul 2, 2018 • 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

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

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

pluma commented Jul 9, 2018

ewoutp commented Jul 2, 2018 •

edited

Loading