Make frontend listen for events when instances' presence changes #1779
Conversation
jamie-suse
added
enhancement
| @@ -79,6 +81,19 @@ function* applicationInstanceHealthChanged({ payload }) { | |||
| yield put(updateApplicationInstanceHealth(payload)); | |||
| } | |||
|
|
|||
| export function* applicationInstanceAbsentAtChanged({ payload }) { | |||
| yield put(updateApplicationInstanceAbsentAt(payload)); | |||
There was a problem hiding this comment.
This may be a bit technical, but payload will include the field sid, that we don't strictly need to pass to updateApplicationInstanceAbsentAt(). Thoughts on this?
There was a problem hiding this comment.
It doesn't really matter, it is fine this way
| @@ -111,6 +113,19 @@ function* databaseInstanceSystemReplicationChanged({ payload }) { | |||
| yield put(updateSAPSystemDatabaseInstanceSystemReplication(payload)); | |||
| } | |||
|
|
|||
| export function* databaseInstanceAbsentAtChanged({ payload }) { | |||
| yield put(updateDatabaseInstanceAbsentAt(payload)); | |||
There was a problem hiding this comment.
| const { sid, absent_at } = payload; | ||
| yield put( | ||
| notify({ | ||
| text: `The application instance ${sid} is now ${ |
There was a problem hiding this comment.
Thoughts on this message?
assets/js/state/sagas/databases.js
Outdated
| const { sid, absent_at } = payload; | ||
| yield put( | ||
| notify({ | ||
| text: `The database instance ${sid} is now ${ |
There was a problem hiding this comment.
There was a problem hiding this comment.
I would put:
The database instance ${instance_number} from ${sid} is absent
and
The database instance ${instance_number} from ${sid} is present again
assets/js/state/sagas/databases.js
Outdated
| const { sid, absent_at } = payload; | ||
| yield put( | ||
| notify({ | ||
| text: `The database instance ${sid} is now ${ |
There was a problem hiding this comment.
I would put:
The database instance ${instance_number} from ${sid} is absent
and
The database instance ${instance_number} from ${sid} is present again
assets/js/state/databases.js
Outdated
| state.databaseInstances = updateInstance( | ||
| state.databaseInstances, | ||
| instance, | ||
| { absent_at } |
There was a problem hiding this comment.
I would put {absent_at: instance.absent_at} to have the same style like the other reducer functions
| @@ -41,6 +43,53 @@ describe('SAP Systems sagas', () => { | |||
| console.error.mockRestore(); | |||
| }); | |||
|
|
|||
| it('should update the absent_at field when the database instance is marked absent', async () => { | |||
There was a problem hiding this comment.
Why this now in the top of the file?
I would put it at the end as it is at the end in the prod code as well
| it('should update the absent_at field when the database instance is marked absent', async () => { | ||
| const { sap_system_id, instance_number, host_id, sid } = | ||
| databaseInstanceFactory.build(); | ||
| const absent_at = Date.now(); |
There was a problem hiding this comment.
I bit strange this absent_at not going in the factory itself, isn't it?
| @@ -79,6 +81,19 @@ function* applicationInstanceHealthChanged({ payload }) { | |||
| yield put(updateApplicationInstanceHealth(payload)); | |||
| } | |||
|
|
|||
| export function* applicationInstanceAbsentAtChanged({ payload }) { | |||
| yield put(updateApplicationInstanceAbsentAt(payload)); | |||
There was a problem hiding this comment.
It doesn't really matter, it is fine this way
There was a problem hiding this comment.
Thank you @CDimonaco
Green light.
Just added some few tiny cosmetic comments, but you don't need a new review in any case
assets/js/state/sapSystems.js
Outdated
| @@ -149,6 +149,15 @@ export const sapSystemsListSlice = createSlice({ | |||
| } | |||
| ); | |||
| }, | |||
| updateApplicationInstanceAbsentAt: (state, { payload: instance }) => { | |||
| const { absent_at } = instance; | |||
There was a problem hiding this comment.
Could we have this like the database reducer?
| setApplicationInstanceDeregistering, | ||
| unsetApplicationInstanceDeregistering, | ||
| setDatabaseInstanceDeregisteringToSAPSystem, | ||
| unsetDatabaseInstanceDeregisteringToSAPSystem, | ||
| updateDatabaseInstanceAbsentToSAPSystem, |
There was a problem hiding this comment.
I would put this together with the first one, in line 247, as they are related
assets/js/state/sapSystems.js
Outdated
| @@ -183,6 +192,15 @@ export const sapSystemsListSlice = createSlice({ | |||
| { deregistering: false } | |||
| ); | |||
| }, | |||
| updateDatabaseInstanceAbsentToSAPSystem: (state, { payload: instance }) => { | |||
There was a problem hiding this comment.
What about putting this right after the other? as they are related and to avoid scrolling
CDimonaco
force-pushed
the
listen-marked-absent
branch
from
b5c68ec to
c9c0d8a
Compare
CDimonaco
force-pushed
the
listen-marked-absent
branch
from
c9c0d8a to
e47b0ef
Compare
Description
This PR adds the ability for the frontend to listen for
application_instance_absent_at_changed&database_instance_absent_at_changedwebsocket events when instances' presence changes.With this change, the frontend's state will update when an instance becomes absent or present.
How was this tested?
Added tests in Redux reducers and Sagas to test this behaviour.