Code review comment for lp://staging/~ack/charms/trusty/odl-controller/odl-package-from-repo

Revision history for this message
Alberto Donato (ack) wrote :

> Minor points:
>
> *I think 'install-sources' is better than 'install-origin' and is consistent
> with other charms.
>
> *Using 'configure_sources(True, "install-sources")' is better than just
> 'add_source(install_origin)' so that you can use a list of PPAs or other
> methods for installation.
>
> *Remove 'install-origin' empty default.
>
> *I think the logic of the install hook could be cleaner. It may be easier to
> express as:
>
> if config.get("install-origin"):
> # configure sources
>
> # install dependencies
> apt_install(PACKAGES)
>
> url = config.get("install-url")
> if url:
> # install opendaylight from url
> else:
> # install opendaylight using apt
>
> Then 'install-origin' is more like a apt repo override and 'install-url' a
> package override where both options can be used with or without each other.

Thanks for the review. All points addressed as suggested.

« Back to merge proposal