Code review comment for lp:~ed-leafe/nova/powerstate

Revision history for this message
Ed Leafe (ed-leafe) wrote :

> > + """Starts the host. NOTE: Currently not feasible, since the host
>
> Per Hacking, this should probably be NOTE(dabo):

That was left over when copied from another part of the code. I just removed the NOTE:, since it really isn't needed.

> > + except TypeError as e:
>
> Two spaces between 'TypeError' and 'as'. (Surprised Pep8 didn't catch this)

You and me both. Fixed.

> > 124 + def host_power_action(self, context, instance_id=None, host=None,
> > 125 + action=None):
>
> Should this take an instance_id parameter?

I think that this was the result of copying the signatures of similar methods. I've removed the instance_id references from both manager and api.

> > 32 + return []
>
> Out of curiosity, should this raise a 4XX (maybe a 401 error) to let the user
> know that they are doing something wrong if they are trying to access an
> admin_only api call?

This isn't a runtime thing; it only comes into play when the extensions are loaded. It is analogous to the code for the non-extension admin API methods: if the flag is False, they simply aren't loaded. At runtime, if the user tries to call them, they get a 404, since they simply aren't defined.

« Back to merge proposal