Create a pool and attach machines to it #985
Conversation
huwshimi
force-pushed
the
create-and-set-pool
branch
from
74bae5d to
95f2ea1
Compare
There was a problem hiding this comment.
Code and QA + 1 from me, but I think the websocket and saga code is a little over my head so I'll let @squidsoup add the approval if he's happy with it.
ui/src/app/base/sagas/actions.js
Outdated
| * @param {Array} machines - A list of machine ids. | ||
| * @returns {Array} The list of action creator functions. | ||
| */ | ||
| export const generateActionCreators = (machines) => |
There was a problem hiding this comment.
Will every one of these chained websocket sagas need its own function generator like this?
There was a problem hiding this comment.
Yeah, something like that. It depends on the action that needs to be performed. The generator is outside. This is only external to the saga to make the saga testable.
| .provide([[call(getNextActions, 99), [actionCreator]]]) | ||
| .call(actionCreator, response.result) | ||
| .put(action) | ||
| .run(); |
There was a problem hiding this comment.
Gah! I forgot how confusing these saga tests are.
huwshimi
force-pushed
the
create-and-set-pool
branch
from
95f2ea1 to
f3c98e8
Compare
There was a problem hiding this comment.
A few small comments for your consideration, but I think overall this seems workable. Thanks for figuring this out @huwshimi. Have you given any thought as to how error handling will work? If our action requires two 'commits' across the websocket, what happens if the first fails? (e.g. if creating a new resource pool fails, before associating machines).
ui/src/app/base/sagas/actions.js
Outdated
| * @param {Array} machines - A list of machine ids. | ||
| * @returns {Array} The list of action creator functions. | ||
| */ | ||
| export const generateActionCreators = (machines) => |
There was a problem hiding this comment.
The name of this function seems a bit odd, generateActionCreators sounds generic, but this function is specific to generating actionCreators for machineActions.setPool.
ui/src/app/base/sagas/websockets.js
Outdated
| * | ||
| * @param {Object} response - A websocket response. | ||
| */ | ||
| export function* handleNextActions({ request_id, result }) { |
There was a problem hiding this comment.
I think typically you would only destructure an object as a function param if you were setting a default value, otherwise it is probably clearer to destructure the object in the function body so the jsdoc matches the function signature.
| @@ -292,7 +340,7 @@ export function* sendMessage(socketClient, action) { | |||
| /** | |||
| * Connect to the WebSocket and watch for message. | |||
| */ | |||
| export function* setupWebSocket() { | |||
| export function* setupWebSocket(messageHandlers = []) { | |||
|
@squidsoup my plan is that the form will receive the error from the redux store for the pool model, I've created an issue for that here: #984. I need to do #881 first so that the form stays open so it can display the errors. |
huwshimi
force-pushed
the
create-and-set-pool
branch
from
3d4d50c to
dd717c2
Compare
Done
Note: Errors and saving state will be handled by #984 and #881.
QA
Fixes
Fixes: #928.