Merge lp://staging/~doanac/ubuntu-ci-services-itself/bug1285401 into lp://staging/ubuntu-ci-services-itself

PASSED: Continuous integration, rev:282
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/243/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/243/rebuild

Excellent !

A few nits (well a single one actually ;):

10 error_msg = 'Exception: {}'.format(e)
11 log.error(error_msg)
12 out_data['error_message'] = error_msg
13 amqp_utils.progress_failed(trigger, out_data)

error_message is used

48 exc_type, val, tb = sys.exc_info()
49 log.exception('Image build failed:')

nothing is used

68 - except Exception as e:
69 - # FIXME: 'message' ??? Is the lander expecting that ? Needs testing --
70 - # vila 2014-02-06
71 + except (KeyboardInterrupt, Exception) as e:
72 results['message'] = str(e)
73 notify = amqp_utils.progress_failed

message is used

So, if the answer to the FIXME is yes, fair enough (mentioning it in the cover letter would help though ;).

Otherwise, why do we use different attributes, different logging reporting (error, expection, none) and are the receivers expecting that ?

Nothing blocking, just curious, good job !

On 03/04/2014 03:01 AM, Vincent Ladeuil wrote:

> 48 exc_type, val, tb = sys.exc_info()
> 49 log.exception('Image build failed:')
>
> nothing is used

This run_worker has diverged from the pattern of the other 2. If you
look in the finally block, you'll see it sets an error message.

> 68 - except Exception as e:
> 69 - # FIXME: 'message' ??? Is the lander expecting that ? Needs testing --
> 70 - # vila 2014-02-06
> 71 + except (KeyboardInterrupt, Exception) as e:
> 72 results['message'] = str(e)
> 73 notify = amqp_utils.progress_failed
>
> message is used
>
> So, if the answer to the FIXME is yes, fair enough (mentioning it in the cover letter would help though ;).

yeah. should have done.

> Otherwise, why do we use different attributes, different logging reporting (error, expection, none) and are the receivers expecting that ?

You always seem to hone in on issues I've got in my mind but haven't
articulated to the team. I think this type of bug fix highlights the
need for some type of new run_worker helper. The helper its could deal
with the nasty exception handling stuff in a consistent way and leave
the real worker logic to only having to think about its real job and the
common exceptions that it *wants* to handle in a certain way.

I'm starting to think its not worth trying for phase-0 and I'm starting
to think for post phase-0 we might want to re-think the whole notion of
how these "run-workers" are used. I feel a bit like purgatory here :)