feat(xo-web/host): warning when host and XOA's time differ too much #4173

Rajaa-BARHTAOUI · 2019-04-30T14:16:33Z

fixes #4113

Screenshots

Check list

Check items when done or if not relevant

PR reference the relevant issue (e.g. Fixes #007)
if UI changes, a screenshot has been added to the PR
CHANGELOG.unreleased.md:
- enhancement/bug fix entry added
- list of packages to release updated (${name} v${new version})
documentation updated
I have tested added/updated features (and impacted code)

Process

create a PR as soon as possible
mark it as WiP: (Work in Progress) if not ready to be merged
when you want a review, add a reviewer
if necessary, update your PR, and re- add a reviewer

badrAZ · 2019-05-13T12:00:22Z

packages/xo-web/src/xo-app/home/host-item.js

@@ -128,6 +129,12 @@ export default class HostItem extends Component {
                  </Tooltip>
                )}
                &nbsp;
+                {!assertConsistentHostServerTime(host) && (


The promise is not yet resolved.

badrAZ · 2019-05-13T14:51:50Z

packages/xo-web/src/xo-app/host/index.js

@@ -254,6 +259,12 @@ export default class Host extends Component {
                  </Link>
                </Tooltip>
              )}
+              &nbsp;
+              {!assertConsistentHostServerTime(host) && (


The promise is not yet resolved.

badrAZ

Please fix the PR title by adding (feat/fix(...)).

badrAZ · 2019-05-15T12:48:33Z

packages/xo-server/src/api/host.js

@@ -215,6 +215,20 @@ emergencyShutdownHost.resolve = {

 // -------------------------------------------------------------------

+export async function assertConsistentHostServerTime({ host }) {
+  await this.getXapi(host)._assertConsistentHostServerTime(host._xapiRef)


Simply return the _assertConsistentHostServerTime promise because we don't need to change it's returned value.

badrAZ · 2019-05-15T13:25:08Z

packages/xo-web/src/common/xo/index.js

@@ -777,6 +777,12 @@ export const emergencyShutdownHosts = hosts => {
  }).then(() => map(hosts, host => emergencyShutdownHost(host)), noop)
 }

+export const assertConsistentHostServerTime = host =>
+  _call('host.assertConsistentHostServerTime', { host: resolveId(host) }).then(


It's a public function which can be used on other cases. Simply return the promise to not change the behavior.

badrAZ · 2019-05-15T13:27:42Z

packages/xo-web/src/xo-app/home/host-item.js

@@ -52,6 +53,12 @@ export default class HostItem extends Component {
    return host && host.power_state === 'Running'
  }

+  componentDidMount() {
+    assertConsistentHostServerTime(this.props.item).then(res => {
+      this.setState({ assertConsistentHostServerTime: res })


Better create a new var which its initial value is false.

badrAZ

Please take care of the comment

badrAZ · 2019-05-16T11:33:43Z

packages/xo-server/src/api/host.js

@@ -215,6 +215,20 @@ emergencyShutdownHost.resolve = {

 // -------------------------------------------------------------------

+export async function assertConsistentHostServerTime({ host }) {


The async is not needed.

badrAZ · 2019-05-16T11:36:07Z

packages/xo-server/src/api/host.js

@@ -215,6 +215,20 @@ emergencyShutdownHost.resolve = {

 // -------------------------------------------------------------------

+export async function assertConsistentHostServerTime({ host }) {
+  return this.getXapi(host)._assertConsistentHostServerTime(host._xapiRef)


_assertConsistentHostServerTime is a private method which cannot be call out side its class. Please make it public.

I don't see why I change it to public method? it reserved for host.

A private method (_methodName) should not be called by the instance.

badrAZ · 2019-05-16T11:38:18Z

packages/xo-web/src/common/intl/messages.js

@@ -922,6 +922,10 @@ const messages = {
  changelogAuthor: 'Author',
  changelogDate: 'Date',
  changelogDescription: 'Description',
+  // ----- Host home -----
+  assertConsistentHostServerTimeTooltip:
+    'host server time and XOA date are not consistent with each other',


To be consistent, host server time and XOA date -> time.

packages/xo-web/src/xo-app/home/host-item.js

badrAZ · 2019-05-16T14:28:18Z

packages/xo-web/src/xo-app/home/host-item.js

@@ -47,11 +48,21 @@ import styles from './index.css'
  ),
 }))
 export default class HostItem extends Component {
+  state = {
+    assertConsistentHostServerTime: false,


isHostServerTimeConsistent

badrAZ · 2019-05-17T07:18:33Z

packages/xo-web/src/xo-app/home/host-item.js

  get _isRunning() {
    const host = this.props.item
    return host && host.power_state === 'Running'
  }

+  componentDidMount() {
+    assertConsistentHostServerTime(this.props.item).catch(() => {


What about creating a component using reaclette to avoid code duplication ?

badrAZ · 2019-05-17T11:07:55Z

packages/xo-web/src/common/utils.js

+
+// ===================================================================
+
+export const AssertConsistentHostServerTime = decorate([


Let's move it to its own module to avoid have a big file with a lot of functions.

badrAZ · 2019-05-17T11:09:41Z

packages/xo-web/src/common/utils.js

+        <Icon color='text-danger' icon='alarm' />
+      </Tooltip>
+    ),
+])


Please define propTypes.

badrAZ · 2019-05-17T11:10:16Z

packages/xo-web/src/common/utils.js

+    },
+  }),
+  injectState,
+  ({ state: { isHostServerTimeConsistent } }) =>


{ state: { isHostServerTimeConsistent = true } }

badrAZ · 2019-05-17T11:11:02Z

packages/xo-web/src/xo-app/home/host-item.js

  connectStore,
  formatSizeShort,
  hasLicenseRestrictions,
  osFamily,
 } from 'utils'
+import { Col, Row } from 'grid'


Not related change.

I made some changes that require Sort.

It's not sorted.

Inline imports before multiple lines imports.

badrAZ · 2019-05-17T11:11:08Z

packages/xo-web/src/xo-app/home/host-item.js

 import {
  createDoesHostNeedRestart,
  createGetObject,
  createGetObjectsOfType,
  createSelector,
 } from 'selectors'
+import { Text } from 'editable'


Not related change.

I made some changes that require Sort.

badrAZ · 2019-05-17T11:14:13Z

packages/xo-web/src/xo-app/home/host-item.js

                      <Icon icon='alarm' />
                    </Link>
                  </Tooltip>
                )}
                &nbsp;
+                <AssertConsistentHostServerTime host={id} />


Simply host.id to avoid unrelated changes.

I do it to not access to id many times.

It not cost a lot of performance and it produce a lot of unrelated change. Better use host.id to avoid it.

badrAZ · 2019-05-17T11:14:28Z

packages/xo-web/src/xo-app/home/host-item.js

                      <Icon icon='alarm' />
                    </Link>
                  </Tooltip>
                )}
                &nbsp;
+                <AssertConsistentHostServerTime host={id} />


host -> hostId.

pdonias · 2019-05-21T13:44:47Z

packages/xo-server/src/api/host.js

@@ -215,6 +215,20 @@ emergencyShutdownHost.resolve = {

 // -------------------------------------------------------------------

+export function assertConsistentHostServerTime({ host }) {


Should this be a checkConsistentHostServerTime?

Ping @julien-f

For me assert (check to a lesser extent) means that it will throws if the condition is not fulfilled.

IMHO, if it returns a boolean, it should be called isConsistentHostServerTime.

Ok so my (unclear) question should have been: should this return a boolean (and be named accordingly)?

Yes, for an API, I agree it makes more sense.

pdonias · 2019-05-21T14:05:18Z

packages/xo-web/src/common/intl/messages.js

@@ -887,6 +889,10 @@ const messages = {
  changelogAuthor: 'Author',
  changelogDate: 'Date',
  changelogDescription: 'Description',
+  // ----- Host home -----
+  assertConsistentHostServerTimeTooltip:
+    'Host server time and XOA time are not consistent with each other',


"Host time"?

pdonias · 2019-05-21T14:07:04Z

packages/xo-web/src/xo-app/home/host-item.js

+      power_state: powerState,
+      tags,
+      version,
+    } = host


Avoid adding too many unrelated changes.

pdonias · 2019-05-21T14:11:13Z

packages/xo-web/src/common/intl/messages.js

@@ -887,6 +889,10 @@ const messages = {
  changelogAuthor: 'Author',
  changelogDate: 'Date',
  changelogDescription: 'Description',
+  // ----- Host home -----
+  assertConsistentHostServerTimeTooltip:


This is not an assert.

pdonias · 2019-05-21T14:12:13Z

packages/xo-web/src/common/assert-consistent-host-server-time.js

+  }),
+  injectState,
+  ({ state: { isHostServerTimeConsistent = true } }) =>
+    isHostServerTimeConsistent ? null : (


IMO, it's a bit weird to say that something is "consistent" by itself.

pdonias · 2019-05-24T09:55:29Z

packages/xo-server/src/api/host.js

@@ -215,6 +215,21 @@ emergencyShutdownHost.resolve = {

 // -------------------------------------------------------------------

+export async function isConsistentHostServerTime({ host }) {
+  await this.getXapi(host).assertConsistentHostServerTime(host._xapiRef)


We don't want something that rejects as a normal behaviour. You'd need something like:

try { await this.getXapi(host).assertConsistentHostServerTime(host._xapiRef) return true } catch () { return false }

But then it would also take any other error as a "no", or maybe create a whole new helper method.

pdonias · 2019-05-24T13:38:20Z

packages/xo-web/src/common/assert-consistent-host-time.js

+  provideState({
+    computed: {
+      isHostTimeConsistentToXoaTime: (_, { hostId }) =>
+        isHostTimeConsistentToXoaTime(hostId).then(() => true, () => false),


It should already resolve with a boolean.

pdonias · 2019-05-24T13:40:14Z

packages/xo-web/src/common/assert-consistent-host-time.js

+import { injectState, provideState } from 'reaclette'
+import { isHostTimeConsistentToXoaTime } from 'xo'
+
+const AssertConsistentHostTime = decorate([


Assert is a verb. This is a component and it doesn't assert anything.

pdonias · 2019-06-17T12:06:34Z

packages/xo-server/src/api/host.js

@@ -211,6 +211,25 @@ emergencyShutdownHost.resolve = {

 // -------------------------------------------------------------------

+export async function isConsistentHostServerTime({ host }) {


isHostServerTimeConsistent

pdonias · 2019-06-17T14:40:22Z

packages/xo-web/src/common/consistent-host-time-warning.js

+import { injectState, provideState } from 'reaclette'
+import { isHostTimeConsistentToXoaTime } from 'xo'
+
+const ConsistentHostTimeWarning = decorate([


So it would be Inconsistent... if it's a warning.

pdonias · 2019-06-17T14:40:44Z

packages/xo-web/src/common/consistent-host-time-warning.js

+  }),
+  injectState,
+  ({ state: { isHostTimeConsistentToXoaTime = true } }) =>
+    isHostTimeConsistentToXoaTime ? null : (


pdonias · 2019-06-17T14:42:49Z

packages/xo-web/src/common/intl/messages.js

@@ -890,6 +890,10 @@ const messages = {
  changelogAuthor: 'Author',
  changelogDate: 'Date',
  changelogDescription: 'Description',
+  // ----- Host home -----


Not necessarily apparently.

fixes vatesfr#4113

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch from 1efdd6c to ba3356d Compare May 13, 2019 10:55

Rajaa-BARHTAOUI requested a review from badrAZ May 13, 2019 11:10

badrAZ reviewed May 13, 2019

View reviewed changes

badrAZ suggested changes May 13, 2019

View reviewed changes

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch from 91ac451 to 6e71e3f Compare May 14, 2019 10:06

Rajaa-BARHTAOUI requested a review from badrAZ May 14, 2019 10:08

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch from 6e71e3f to 4d61825 Compare May 15, 2019 08:57

badrAZ suggested changes May 15, 2019

View reviewed changes

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch from 4d61825 to ca59421 Compare May 16, 2019 10:13

Rajaa-BARHTAOUI requested a review from badrAZ May 16, 2019 10:36

badrAZ suggested changes May 16, 2019

View reviewed changes

Rajaa-BARHTAOUI changed the title ~~Warning when host's time differs too much from XOA's time~~ feat(xo-web/host): display warning when host's time differs too much from XOA's time May 16, 2019

badrAZ reviewed May 16, 2019

View reviewed changes

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch 3 times, most recently from f03f7ee to 9df523e Compare May 16, 2019 15:26

Rajaa-BARHTAOUI requested a review from badrAZ May 16, 2019 15:40

badrAZ reviewed May 17, 2019

View reviewed changes

Rajaa-BARHTAOUI requested a review from badrAZ May 17, 2019 10:21

badrAZ suggested changes May 17, 2019

View reviewed changes

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch 2 times, most recently from 2900ab9 to bf1f55b Compare May 20, 2019 13:58

Rajaa-BARHTAOUI requested a review from badrAZ May 20, 2019 14:20

badrAZ approved these changes May 21, 2019

View reviewed changes

badrAZ requested a review from pdonias May 21, 2019 10:36

pdonias requested changes May 21, 2019

View reviewed changes

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch 2 times, most recently from a531f4f to cb6d68f Compare May 22, 2019 14:01

Rajaa-BARHTAOUI requested a review from pdonias May 22, 2019 14:04

pdonias requested changes May 24, 2019

View reviewed changes

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch from cb6d68f to 2283d4b Compare June 14, 2019 08:40

Rajaa-BARHTAOUI requested a review from pdonias June 14, 2019 10:08

pdonias requested changes Jun 17, 2019

View reviewed changes

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch from 2283d4b to 088abfe Compare June 19, 2019 08:57

Rajaa-BARHTAOUI added 4 commits June 19, 2019 11:01

feat(xo-web): warning when host's time differs too much from XOA's time

6031ebd

fixes vatesfr#4113

fixes

a201559

more fixes

d0ab1be

fixes

dfd301b

Rajaa-BARHTAOUI force-pushed the rajaa-warning-time-xoa branch from 088abfe to dfd301b Compare June 19, 2019 09:02

minor fixes

fa152ea

Rajaa-BARHTAOUI requested a review from pdonias June 19, 2019 09:56

pdonias approved these changes Jun 21, 2019

View reviewed changes

Merge branch 'master' into rajaa-warning-time-xoa

8177e6e

pdonias changed the title ~~feat(xo-web/host): display warning when host's time differs too much from XOA's time~~ feat(xo-web/host): warning when host and XOA's time differ too much Jun 21, 2019

pdonias merged commit 8782151 into vatesfr:master Jun 21, 2019

		@@ -215,6 +215,20 @@ emergencyShutdownHost.resolve = {

		// -------------------------------------------------------------------

		export async function assertConsistentHostServerTime({ host }) {


		// ===================================================================

		export const AssertConsistentHostServerTime = decorate([

		@@ -215,6 +215,20 @@ emergencyShutdownHost.resolve = {

		// -------------------------------------------------------------------

		export function assertConsistentHostServerTime({ host }) {

		@@ -211,6 +211,25 @@ emergencyShutdownHost.resolve = {

		// -------------------------------------------------------------------

		export async function isConsistentHostServerTime({ host }) {

feat(xo-web/host): warning when host and XOA's time differ too much #4173

feat(xo-web/host): warning when host and XOA's time differ too much #4173

Conversation

Rajaa-BARHTAOUI commented Apr 30, 2019 • edited Loading

Screenshots

Check list

Process

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badrAZ 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

badrAZ 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

badrAZ May 16, 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

Rajaa-BARHTAOUI May 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rajaa-BARHTAOUI commented Apr 30, 2019 •

edited

Loading

badrAZ May 16, 2019 •

edited

Loading

Rajaa-BARHTAOUI May 20, 2019 •

edited

Loading