Code review comment for ~tamilmani1989/cloud-init:azure_networking

Revision history for this message
Chad Smith (chad.smith) wrote :

Updated my comment and patch suggestions, please feel free to disagree where you think points are irrelevant or wrong

  Here are a couple thoughts on your branch to aid in testability of the branch features:

The complete patch suggestion is here http://paste.ubuntu.com/p/pDYvtFQN8g/

I've itemized the general thoughts here for your review to see what you think
  1. generally let's limit what lives in a try/except block to code that we expect will actually generate exceptions. I moved general increment logic outside of the struct.unpack calls and moved the try/except socket.error checks to around the read_netlink_socket as I didn't think socket.errors would be raised by anything else in wait_for_media_disconnect_connect

  2. I suggest breaking up NetworkEvents class as it seems just a collection of functions more than a class with state. When I see one-line class methods that don't actually depend on any instance attributes, it's a hint that it might be an empty level of indirection we just don't need (like select_netlink_socket, read_socket and close_socket methods).

 3. renam3 networkevents.py -> netlink.py as a module to make it a bit more specific
 4 personal preference is to use namedtuples instead of classes to host simple structured attributes like your rta_attr definition
 5. I'd like to see simple unit tests on each function per the simple example I added in cloudinit/sources/helpers/tests/test_netlink.py

review: Needs Information

« Back to merge proposal