Code review comment for lp:~fginther/ubuntu-ci-services-itself/bsb-dput-3

This is a follow up to bsb-dput-2. I screwed up the branch and had to resubmit the whole thing.

> I'm skipping the cupstream2distro code since its mostly copy/paste.
>
> = run-worker:
>
> 2050 + if amqp_utils.progress_completed(trigger):
> 2051 + log.error('Unable to notify progress-trigger completion of action')
>
> the progress_completed function already logs an error, so you really
> only need to check the return code for it if you are going to handle
> failure in some special way. Since we don't here, I'd delete the
> log.error line.

Fixed.

> You also need to supply a payload to the "progress_completed" call. The
> data returned from this function winds up being the data that will be
> passed to the image builder. I think it needs to contain:
>
> {
> "ppa": "<foo>"
> }
>
> so we can remove the hack from:
>
> <http://bazaar.launchpad.net/~canonical-ci-engineering/ubuntu-ci-services-
> itself/trunk/view/head:/charms/precise/lander-
> jenkins/templates/jobs/lander_master.xml#L170>

Fixed, good catch on removing the hack.

> 2052 + except EnvironmentError as e:
>
> I think I'd just catch "Exception" here. That way nothing slips through
> in a way where we won't call amqp_utils.progress_failed

Fixed

> = upload_package:
>
> 2141 + file_name = source.split('/')[-1]
>
> I think you can just os.path.basename to make it more obvious what your
> intent is:
>
> os.path.basename(source)

Fixed, I also removed the hack to remove the container name from the filename (the data-store module has been updated to just use the filename).

> 2186 + logging.basicConfig(level=logging.INFO, format="%(asctime)s
> %(levelname)s "
> 2187 + "%(message)s")
>
> I think that should be moved to "main" instead. Otherwise you might just
> override the logging config of the process that's using this API.

Fixed, also fixed in watch_ppa.py