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

Revision history for this message
Andy Doan (doanac) wrote :

> === 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.

> === 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

> === 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?

« Back to merge proposal