-
Notifications
You must be signed in to change notification settings - Fork 7
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
Liu 186 - Register DIM with zeroconf with MM #156
Conversation
Adds REST APIs for MM to add/remove DIMs & Nodes from DIMs Adds zeroconf discovery of DIMs Adds nm_dim_assigner - assigns NMs to DIMs as they come on/off-line Adds associated tests asserting this functionality works when zeroconf is enable, and does not work when disabled.
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.
Sorry for the delay on reviewing this, I finally got the change to give it a close look.
Overall the code looks good to me, nice to see tests going alongside the new code (and coverage going up 😄). Most changes are minor things, so hopefully it won't be too much of a burden to act on them.
Co-authored-by: rtobar <rtobar@icrar.org>
Co-authored-by: rtobar <rtobar@icrar.org>
Co-authored-by: rtobar <rtobar@icrar.org>
Adjusts two tests which now check for empty lists, rather than None accordingly.
Thank you for the feedback/suggestions by the way, it's very appreciated :D |
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.
Just a final small detail and I'm happy to get this merged.
I found wait_until
and friends a bit confusing and verbose, but (if you agree) I'll compress that a bit afterwards to not block this PR any longer.
Changes look good to me, let's ignore the failure for the time being, I've addressed it in 05076f9 |
Limitations: