-
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
Add Redux Sagas for getting SUSE Manager software updates #2416
Conversation
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.
Good job, Jamie.
It's a good starting point, however I think we need to keep iterating in order to make things click.
There might be edge cases I've overlooked, but let's make it work first, then we polish what needs fine tuning 😉
Hope the comments are somewhat useful. Happy to help.
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.
Cool, good enhancements!
I guess we just need to change software_updates
to upgradable_packages
😝
What I am actually missing is the clicking part on error occurrence, cause I am not sure yet what the backed would return. #2415 (comment)
I am afraid we need to keep this a bit on hold.
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.
No blockers from my side, LGTM
assets/js/state/softwareUpdates.js
Outdated
state.softwareUpdates = Object.entries(state.softwareUpdates).reduce( | ||
(newState, [id, updates]) => | ||
id !== hostId ? { ...newState, [id]: updates } : newState, | ||
{} | ||
); |
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.
state.softwareUpdates = Object.entries(state.softwareUpdates).reduce( | |
(newState, [id, updates]) => | |
id !== hostId ? { ...newState, [id]: updates } : newState, | |
{} | |
); | |
state.softwareUpdates = { | |
...state.softwareUpdates, | |
[hostId]: {}, | |
}; |
What do you think?
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.
Sure, I think it's good. It got me thinking though; what do we think about clearing setting [hostId]: { relevant_patches: [], upgradable_packages: [] }
as opposed to [hostId]: {}
?
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.
On second thought, it might be bad as the selector would report relevant_patches
/upgradable_packages == 0
instead of undefined
. But as @nelsonkopliku mentioned, error handling should handle this before it even hits the selector 🤔
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.
No strong opinion, let's ship whatever you prefer 👍
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 like having a semantic difference between undefined
vs 0
. Sure, the error handling should handle this but it still gives a more semantically correct representation of the state of the system.
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 guess we can give this a go.
Error handling should properly feed the state with errors caught from 404s and 422s.
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
Description
Add Redux Sagas to allow retrieval of software updates for a given host.
This PR also includes Selectors to derive the number of relevant patches and upgradable packages from the state.
How was this tested?
Added unit tests.