praekeltfoundation / vumi

Messaging engine for the delivery of SMS, Star Menu and chat messages to diverse audiences in emerging markets and beyond.
BSD 3-Clause "New" or "Revised" License
421 stars 131 forks source link

Cleanup Vumi bridge and add Junebug support #1028

Closed KaitCrawford closed 8 years ago

hodgestar commented 8 years ago

Left two small comments, and we need to look at why tests are failing, but otherwise looks good.

hodgestar commented 8 years ago

Given that unrelated middleware tests are failing for all builds except the one pinned to Twisted 13.2, I suspect this indicates more tests broken by Twisted 16.0.

rudigiesler commented 8 years ago

@hodgestar Looking at the middleware tests that are failing, looks like there's a twisted import (to do with ssh), that if it fails, the tests aren't run. So it's very possible that the latest twisted fixed that module import, and that test has been broken for a very long time. (Either way, completed unrelated to this PR).

hodgestar commented 8 years ago

Creating #1029 to deal with the manhole middleware test failures.

KaitCrawford commented 8 years ago

@hodgestar @rudigiesler Please give this another look. I'm not sure the last commit is what you meant hodgestar, I might be misunderstanding how web_path gets used. I also think I should probably be updating the status in more places but I'm struggling to identify where.

hodgestar commented 8 years ago

@KaitCrawford Could you merge develop into this branch so we can check if tests pass?

hodgestar commented 8 years ago

Other than removing the TODO, looks good to me!

KaitCrawford commented 8 years ago

Really? \o/ whoohooo!

rudigiesler commented 8 years ago

Looks good to me too. :+1:

hodgestar commented 8 years ago

Oh, I realized something -- do we want to add status events for inbound messages too? Just "good" if we receive a valid request and "bad" if we receive a broken request?

hodgestar commented 8 years ago

I also remembered that we wanted to only publish acks once we received an ack from Vumi Go (so publish a nack if submitting the message to Vumi Go fails and then publish whatever event Vumi Go gives us).

hodgestar commented 8 years ago

Maybe this means we should also re-think the components a bit. Maybe we want separate components for http-requests-to-vumi-go, http-requests-from-vumi-go and events-from-vumi-go (except with better names)?

KaitCrawford commented 8 years ago

@hodgestar the names are probably still horrible :)

hodgestar commented 8 years ago

Idea for better status component names:

Thoughts?

KaitCrawford commented 8 years ago

@hodgestar please re-review

KaitCrawford commented 8 years ago

@hodgestar updated again

hodgestar commented 8 years ago

Left what I hope are the last two minor comments!

KaitCrawford commented 8 years ago

@hodgestar ready for another round

hodgestar commented 8 years ago

:+1: :cake:

Also, I am honoured to present @KaitCrawford with The Monthly :camel: Trophy for continuing with this PR all the way to the end!

KaitCrawford commented 8 years ago

what is a :camel: trophy and should I be scared?

hodgestar commented 8 years ago

It is a trophy for successfully crossing the desert. :)