Code review comment for lp:~ursinha/uci-engine/private-lp-mthood

Thanks for reviewing, Andy.

> > === modified file 'branch-source-
> builder/cupstream2distro/launchpadmanager.py'
>
> > +sys.path.append(os.path.join(os.path.dirname(__file__), '../ci-utils'))
>
> I don't think we need sys.path.append stuff in our code anymore.

This branch is on top of mthood one, that is a tad far from lp:uci-engine/trunk. Are these things you mention available since when? I think this should be changed in case we're merging this with trunk, otherwise I'm not sure it's going to work? (same apply for other comments regarding things we have now and didn't have when this was created).

>
> > === modified file 'branch-source-builder/cupstream2distro/packagemanager.py'
>
> > - cmd = ["dput", ppa,
> > + cmd = ["dput", "-u", "-c", "/tmp/dput.cf", ppa,
> > "{}_{}_source.changes".format(source, version_for_source_file)]
>
> I'm being a little over-protective, but as per: "/tmp/dput.cf" - Seems
> like we should either make a constant for that, or (and I don't know the
> code structure/flow, have pass this value down to the function via
> upload_package.py

I'll leave this for Francis to comment, he implemented that part and knows the constraints better than I do.

>
> > === modified file 'branch-source-builder/cupstream2distro/settings.py'
>
> > +PRIVATE_LAUNCHPAD = _cfg.get('private_launchpad', None)
>
> This is beyond the scope of this MP, but a tip: We now have a
> unit_config module: ci_utils.unit_config that was written to handle this
> stuff for all our services.
>
> > === modified file 'branch-source-builder/watch_ppa.py'
>
> def get_launchpad_log(url):
> > '''Retrieves the build log from launchpad (thanks to celso).'''
> > if not url:
> > return None
> > - api_url = url.replace('/launchpad.net/','/api.launchpad.net/devel/')
> > - lp = launchpadmanager.get_launchpad()
> > + api_url = launchpad.get_lp_api_url(api_version="devel")
> > + lp = launchpad.lp_login()
> > return lp._browser.get(api_url)
>
> looks like the "url" parameter to this function is now ignored. Should
> we remove it and refactor the caller of it?
>
> > === added file 'create_private_lp_creds.py'
>
> > +sys.path.insert(0, os.path.join(os.path.dirname(__file__), './ci-utils'))
>
> again - don't think you need to alter sys.path anymore.
>
> > +web_root ="https://mthoodapi.lacinonac.com/"
>
> I don't understand the details well enough, but should this value come
> from our unit_config rather than being hard-coded? Actually it looks
> like deploy.py might be setting this in the unit_config?

This script is only a helper to create the credentials to the private lp instance, I think in trunk the original create_lp_creds.py doesn't even exist anymore. I agree this could be done using values on unit_config, but as this is something that should go away soon I didn't bother much.