Code review comment for lp:~tr3buchet/nova/multi_nic

Revision history for this message
Trey Morris (tr3buchet) wrote :

Brian:

docstrings should be good now.

--

| I see what you mean. We also have the methods 'instance_get_fixed_addresses' which is another option.

That option is fine, but it's coming from the other direction. I propose punting on this for now. As there are multiple functions which may need this pluralizing (some not related to this merge), let's fix them all at once after this is merged. I'm fine filing either a blueprint or a bug for this. Thoughts?

--

| It may just be my preference, but I like to use exception names to communicate the actual error, while the message can have more of a description and any extra information (through keyword arguments). I also like the inheritance hierarchy because you can try/except a more basic 'VirtualInterfaceError' and catch any of its more specific subclasses. Again, it may just be my personal preference. Maybe somebody else can chime in here.

Screw it.. it's like 2 lines. DONE!

--

| 2129: Could you make NoFixedIpsDefinedForHost inherit from NoFixedIpsDefined? If there are any other exceptions like this that you added I would appreciate it if you would define the correct inheritance. It can be really handy.

| It also seems that NoFloatingIpsDefined, NoFloatingIpsDefinedForHost, and NoFloatingIpsDefinedForInstance aren't used anymore. Should we delete those?

done.

--

next up are tushar's changes. I'm considering a small revamp to the network_info fields.

« Back to merge proposal