-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Host Checks selection saga #1618
Conversation
daa2674
to
812e5ac
Compare
ef3f035
to
21d9a8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good in general. I just dropped some comments.
I have some doubts how this will evolve though. What are the "follow up" tasks that you mention in the PR description?
PD:
Don't take the next comment as a blocker to merge the PR.
I personally find difficult to review such a dense PR (that includes changes in some many places). I might have overlooked some changes. I think we should drive to have more atomic things. For example, a PR with the redux state changes, that in this case, would already unify the check selection for clusters/hosts first. Instead of duplicating now the code, and refactoring later (or followup work as you describe). The need for refactoring new code looks like a smell to me.
Because now, we have some redux state as checkSelection
which is used only by hosts, but according the name it looks a generic thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @arbulu89 for the feedback!
Getting to the state you're mentioning 😄
For the rest I understand it all, it's another way to get to the same place imho. All in all I am aware of the smells, I just got to make a choice and accept some tradeoffs. I didn't think this PR was exceeding the acceptable size, tho 😅 |
702b415
to
3e5f7ed
Compare
4127488
to
1dbc208
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to merge from my side.
I still have my concerns on how this will evolve and how the different target types checks selection logic will be, but I guess we will see it soon XD
PD: I think we need to have a general consensus on how to write named functions, to know if using global variables inside them is ok or not. I would vote for passing the values as arguments if possible, but I don't even know which is the best practice
@@ -101,6 +101,14 @@ function ChecksSelection({ clusterId, cluster }) { | |||
} | |||
}, [loading]); | |||
|
|||
const saveSelection = () => | |||
dispatch( | |||
checksSelected({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to pass selectedChecks
and clusterId
as the function arguments?
Using global args looks a bit unsafe.
But talking from my ignorance... XD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to have different names tho, otherwise it will clash.
And at the end we would be still passing the same variables, with a different name.
I am open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a suggestion...
Anyway, now that i checked a bit, we should maybe wrap this functions in useCallback
, otherwise the function is redefined in every render. Maybe a cheap operation, but with the useCallback
it is avoided.
Maybe @dottorblaster has stronger opinions on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey man nice job!
As I'm a bit unfamiliar with Sagas, so I only left a comment --> The rest LGTM :D
Well done.
Description
This PR adds a saga to enable saving of host checks selection.
Note that:
checksSelection
slice is intentionally similar to the existingclusterChecksSelection
.As a followup of this work I'd like to converge to just one slice for check selection based on targetType/targetID for both host check selection and cluster check selection
We'll follow up by aligning both host and cluster check selection to proposed design and get rid of some legacy
Successful saving
Failed saving
I didn't want to add the bigger refactoring of checks selection saga/state right now. It could have been too much.
How was this tested?
Automated tests added.