Code review comment for lp:~springfield-team/charms/precise/nova-cloud-controller/trunk

Revision history for this message
James Page (james-page) wrote :

OK _ so I'd like to review the entire springfield charm set from an overall implementation perspective as well but with regards this merge proposal specifically:

1) /root/openrc and associated config.yaml changes

I really don't like this; the nova-cloud-controller gets related to keystone, which give it a username and password so providing it with one external seems just plain wrong;

I *think* this is used by the vxlan-gateway charm to upload its image to glance - but I think we need to review this approach to getting images into glance; this may be better addressed using a helper installed by package (maybe in the vxgw package from the springfield PPA) that gets run from outside the charms, rather than trying todo this from within the charms themselves.

2) Scope of impact on nova-cloud-controller

I think that the change we really need here is the configuration files and support for the nv1k plugin, as for NVP/NSX.

3) charmhelpers first

the changes in hooks/charmhelpers/contrib/ need to be proposed against lp:charm-helpers before landing changes into nova-cloud-controller charm.

review: Needs Fixing

« Back to merge proposal