Code review comment for lp:~cbehrens/nova/lp844160-build-works-with-zones

> This looks really good, love the refactoring and cleanups. Making a quick
> first-pass with some notes; will plan on a digging in for a more thorough
> review tomorrow.

Great, thanks!

>
>
> > 2160 + # TODO(comstud): I would love to be able to return the full
> > 2161 + # instance information here, but unfortunately passing things
> > 2162 + # like 'datetime' back through rabbit don't work due to
> having
> > 2163 + # to json encode/decode.
>
> Not really necessary for this patch, but for future work, we can set a
> default handler so that the `json` module will encode datetimes in
> iso8601 format[1].
>
> [1]http://stackoverflow.com/questions/455580/json-datetime-between-python-and-
> javascript

Great...good to know. I hadn't bothered looking for a solution yet... really there's all sorts of cases where we should be doing this and it's a discussion blamar proposed for the summit.

>
> > 2294 + zone, _x, host = availability_zone.partition(':')
>
> Could use split here and avoid the throwaway variable:
>
> zone, host = availability_zone.split(':')
>
> > 859 + expl = _("Personality file limit exceeded")
> > 860 + raise
> exc.HTTPRequestEntityTooLarge(explanation=error.message,
> > 861 + headers={'Retry-
> After': 0})
>
> `expl` is defined but never used. This patch just moved these lines around,
> highlighting the issue. If s/error.message/expl/ is the solution, we could
> just make the change in this patch, but if we need to consult the original
> author we could make a separate bug for it.

Yeah, the other one above was just moving lines around also. I'll take a look at these, though!

[...]
> > 2167 + if local is True:
>
> Per PEP8, preferred is:
>
> if local:

I'll fix that. I think that's a habit from doing "if blah is None" to distinguish keyword arguments with a None default from an empty list or dict being passed.. if you know what I mean.

> > 2388 + instances =
> instances.append(self.encode_instance(instance))
>
> Looks like this should be:
>
> instances.append(self.encode_instance(instance))

Shoot, yes. I copy-pasted that broken line around a number of times and thought I went back and fixed all instances of it. Good catch.