-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Slim down Node handler interface #25450
Conversation
475cba0
to
440cb74
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.
LGTM on hubble side. I've been wanting to fix this for a while myself, especially since I've seen a lot more methods added to it recently! Thanks a ton.
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.
Thank you for taking care of this! 🙏 🚀
/test |
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.
Nice cleanup! 🧹
440cb74
to
28b58b6
Compare
Rebased as this picked up a conflict with upstream. |
/test |
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.
nice work! ➕
Review commit by commit, though changes shouldn't be too large
I was looking into
NodeHandler
for a related PR, and noticed that one has to stub out quite a few methods to implement the interface, even if the code is just interested in getting notifed onNode{Add,Update,Delete}
. That seemed suboptimal, so this series splits theNodeHandler
interface in three more focussed interfaces. Purely cosmetic change, no (intentional) functional changes.A next step might be to look into breaking up the
linuxNodeHandler
, but that's stuff for another PR.cc @gandro