Code review comment for lp:~doanac/ubuntu-ci-services-itself/runworker-basic

Revision history for this message
Vincent Ladeuil (vila) wrote :

78 + if not store or not logger:
79 + return

I think we also want to not store the logger if there is nothing there. So:

  log_content = logger.getvalue()
  if not log_content:
      return

102 + amqp_cb, ret = self.handle_request(logger, params)
103 + self._store_worker_log(store, logger, ret)
104 + amqp_cb(trigger, ret)

Pfew, that's dense, took me some back and forth to understand:

ampq_cb is the callback for [n]acking the msg
ret is something I can't define by reading the MP ;-p

Can you document that in handle_request() ? That's what the AMQPWorker users
will need.

The try/except/finally in _on_message() is still big but it makes more sense now. I would love some more comments as this code is quite complex but it's not a blocker.

And that's all I have to say ?

Wow, I had to re-read a few bits but really the above are nits ;)

Nice.

+1

review: Approve

« Back to merge proposal