Overall this patch looks really good Sandy. Nice tests, and some really
helpful comments in there (as usual).
Generally speaking, I think a few whitespaces cleanups would make a make this
patch even better (in particular lining up arg-lists). Not a huge deal,
obviously, but worth mentioning.
In terms of design and implementation, I think it looks good. This matches up
with what we whiteboarded (AFAICT), so I'm all for it.
Specifics
=========
> 10 +from nova import log as logging
Imported but not used.
> 35 + for cap in caps:
> 36 + key_values = cap.split('=')
> 37 + zone[key_values[0]] = key_values[1]
Might be slightly easier to read with tuple-unpacking (also, passing 1 to
split in case the value has a '=' in it):
for cap in caps:
key, value = cap.split('=', 1)
zone[key] = value
Overall this patch looks really good Sandy. Nice tests, and some really
helpful comments in there (as usual).
Generally speaking, I think a few whitespaces cleanups would make a make this
patch even better (in particular lining up arg-lists). Not a huge deal,
obviously, but worth mentioning.
In terms of design and implementation, I think it looks good. This matches up
with what we whiteboarded (AFAICT), so I'm all for it.
Specifics
=========
> 10 +from nova import log as logging
Imported but not used.
> 35 + for cap in caps:
> 36 + key_values = cap.split('=')
> 37 + zone[key_values[0]] = key_values[1]
Might be slightly easier to read with tuple-unpacking (also, passing 1 to
split in case the value has a '=' in it):
for cap in caps:
key, value = cap.split('=', 1)
zone[key] = value
> 62 + super(ComputeMa nager, self)._ _init__ (service_ name="compute" ,
> 63 + *args, **kwargs)
Might be better if args aligned:
super( ComputeManager, self)._ _init__ (service_ name="compute" ,
* args, **kwargs) list('zone_ capabilities' , xenserver; kvm', 'os=linux; windows' ],
> 77 +DEFINE_
> 78 + ['hypervisor=
> 79 + 'Key/Multi-value list representng capabilities of this zone')
Same as above:
DEFINE_ list('zone_ capabilities' ,
['hypervisor= xenserver; kvm', 'os=linux; windows' ],
'Key/ Multi-value list representng capabilities of this zone')
> 88 +from nova import log as logging
Might be worth using the LOG pattern here to get more specific logging:
LOG = logging. getLogger( 'nova.manager' )
> 106 + update_ service_ capabilities is called with non-None values.""" name="undefined "):
> 107 + def __init__(self, host=None, db_driver=None, service_
Spacing. Might be better as:
"""
def __init__(self, host=None, db_driver=None, service_ name="undefined "):
> 313 + params= dict(service= service) )
Couple spaces ---> thataway (to line up with open paren +1)
> 209 + LOG.info(_("Created '%(exchange)s' fanout exchange " self.exchange, key=self. routing_ key))
> 210 + "with '%(key)s' routing key"),
> 211 + dict(exchange=
Could use a bit of formatting work:
LOG. info(_( "Created '%(exchange)s' fanout exchange "
"with '%(key)s' routing key"),
dict( exchange= self.exchange, key=self. routing_ key))
Although, this might be personal preference on my part, so, if that change
doesn't look good to you, please feel free to ignore! :-)
> 638 + svc10_a=(99, 99), svc10_b=(99, 99)))
Spacing, a couple of spaces <---- thattaway.