Code review comment for lp:~rackspace-titan/nova/osapi-serialization

Revision history for this message
Brian Waldon (bcwaldon) wrote :

 > - I like the convenience functions for creating Resources for each entry in
> the mapper. But a factory is usually an object, not a function. Maybe the
> controller module functions should be called 'get_resource()' or
> 'create_resource()' ?

Good point. I renamed resource_factory to create_resource.

> - I see that the approach we take with extensions leans on the serialization
> components you are changing. However, I think it might be better if we don't
> reuse Resources for extensions. Rather, we might want to reuse the serializer
> and deserializer directly. Does this sound feasible?
>
> - It seems like an unnecessary duplication to pass in both the request object
> and the deserialized body of that request to the controller. Mostly it seems
> like controllers use the request object to get access to the nova.context
> object. Perhaps we could extract context from the request and just pass in the
> context and the body? If that is the case, we could also rename the body
> variable to "request" which would be more intuitive to me. This change might
> cause problems with the extensions, which seem to need access to the full
> request--which is partly why I propose that we pull Resource out of the
> extensions module.

I am definitely for all this, but I feel it is out of scope. After addressing how much will have to be refactored to accomplish the goal here, I would love to do this as a separate branch. Thoughts?

> - I think you have some conflicts to resolve from the request extensions
> branch that just merged into trunk.

Took care of this.

I also removed some obsolete code in a few places.

« Back to merge proposal