Cleanup Vumi bridge and add Junebug support #1028

KaitCrawford · 2016-03-22T14:07:06Z

No description provided.

… server config classes

hodgestar · 2016-03-22T15:55:54Z

vumi/transports/vumi_bridge/tests/test_vumi_bridge.py

@@ -65,7 +64,7 @@ def get_next_request(self):

 class TestGoConversationTransport(TestGoConversationTransportBase):

-    transport_class = GoConversationClientTransport
+    transport_class = GoConversationTransport


We need to delete these old client transport tests. They're not failing currently because they're accidentally not being run (because the both this class and the server test class are now called TestGoConversationTransport).

hodgestar · 2016-03-22T15:57:59Z

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

hodgestar · 2016-03-22T16:00:36Z

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 · 2016-03-22T16:04:17Z

@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 · 2016-03-22T16:12:27Z

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

KaitCrawford · 2016-03-23T16:35:15Z

@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 · 2016-03-24T09:23:21Z

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

hodgestar · 2016-03-24T11:51:32Z

vumi/transports/vumi_bridge/vumi_bridge.py

+        published status.'''
+        if self.status_detect.check_status(**kw):
+            yield self.publish_status(**kw)
+            # TODO: Notify Junebug


The line above notifies Junebug, so we can remove this TODO. :)

hodgestar · 2016-03-24T11:52:12Z

Other than removing the TODO, looks good to me!

KaitCrawford · 2016-03-24T12:07:00Z

Really? \o/ whoohooo!

rudigiesler · 2016-03-24T12:31:31Z

Looks good to me too. 👍

hodgestar · 2016-03-24T13:36:13Z

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 · 2016-03-24T13:41:40Z

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 · 2016-03-24T13:43:25Z

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 · 2016-03-24T15:23:26Z

@hodgestar the names are probably still horrible :)

hodgestar · 2016-03-24T15:33:12Z

vumi/transports/vumi_bridge/vumi_bridge.py

+    def add_status_bad_req(self, component):
+        return self.update_status(
+            status='down', component=component, type='bad_request',
+            message='Bad request received')


We need to set type and message more appropriately for the different error / success cases (e.g. Bad request received is not correct when we're failing to make requests).

Maybe this means that add_status_good_req and add_status_bad_req should be removed? Or maybe we should have a pair of these methods for each kind of status update?

a pair of them for each kind of status update at this point seems a bit much to me. I vote for removing them rather

hodgestar · 2016-03-24T15:36:32Z

Idea for better status component names:

http-requests-to-vumi-go -> submitted-to-vumi-go
http-requests-from-vumi-go -> received-from-vumi-go
events-from-vumi-go -> sent-by-vumi-go

Thoughts?

KaitCrawford · 2016-03-29T11:32:08Z

@hodgestar please re-review

hodgestar · 2016-03-29T12:00:48Z

vumi/transports/vumi_bridge/vumi_bridge.py

@@ -134,6 +122,9 @@ def handle_outbound_message(self, message):
        if resp.code != http.OK:
            log.warning('Unexpected status code: %s, body: %s' % (
                resp.code, resp.delivered_body))
+            self.update_status(
+                status='down', component='submitted-to-vumi-go',
+                type='bad_request', message='Bad request sent')


Maybe the message here should be Message submission rejected by Vumi Go. (since we don't know if the request was bad -- only that Vumi Go didn't accept it).

KaitCrawford · 2016-03-29T13:23:44Z

@hodgestar updated again

hodgestar · 2016-03-29T15:34:10Z

vumi/transports/vumi_bridge/vumi_bridge.py

+                self.update_status(
+                    status='ok', component='sent-by-vumi-go',
+                    type='vumi_go_sent', message='Sent by Vumi Go')
+            else:


Events could also be delivery reports, so we should do something like elif msg.payload["event_type"] == "nack" here.

@hodgestar Didn't we decide we weren't going to handle delivery reports at this juncture? Although if this is all that's required I'm happy to add them.
What would a nack event_type mean and what should we do when we receive them?

Events come in three kinds -- acks, nacks and delivery reports. Acks are "message successfully put in pillar box at post office". Nacks are "message spat out by pillar box". Currently the code publishes failed statuses for all events that aren't acks. It should only do that for nacks.

hodgestar · 2016-03-29T15:37:20Z

Left what I hope are the last two minor comments!

…atus

KaitCrawford · 2016-03-30T10:58:48Z

@hodgestar ready for another round

hodgestar · 2016-03-30T11:41:21Z

👍 🍰

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

KaitCrawford · 2016-03-30T11:47:34Z

what is a 🐫 trophy and should I be scared?

hodgestar · 2016-03-30T12:48:01Z

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

Kaitlyn Crawford added 2 commits March 22, 2016 15:35

removed GoConversationClientTransport classes and combined client and…

acaf5c4

… server config classes

removed message_path and event_path from the config and added web_path

cfd5517

hodgestar reviewed Mar 22, 2016
View reviewed changes

Kaitlyn Crawford added 4 commits March 23, 2016 12:16

detect and publish status when trying to send messages

04a9620

removed old tests

bd2c8b3

added test to check bad requests trigger a status change

7f25d3a

use an actual string for web_path

65cd299

Merge branch 'develop' into feature/SEED-510-vumi-bridge-cleanup

ffc6c11

hodgestar reviewed Mar 24, 2016
View reviewed changes

removed unnecessary TODO

dcdb378

Kaitlyn Crawford added 3 commits March 24, 2016 15:49

removed now unused client files and tests

9cc3a5e

publish status for incoming messages as well

1cc8cda

publish status for ack and nack events and changed component names

085860e

hodgestar reviewed Mar 24, 2016
View reviewed changes

changed component names

535c6cf

send more specific status updates with accurate types and messages

9bdb4d9

hodgestar reviewed Mar 29, 2016
View reviewed changes

Kaitlyn Crawford added 2 commits March 29, 2016 14:43

added spaces to Vumi Go

d61f627

changed message for vumi-go status updates

d39c87a

hodgestar reviewed Mar 29, 2016
View reviewed changes

added status for delivery reports and better messages to bad event st…

3fc5f02

…atus

Kaitlyn Crawford added 2 commits March 30, 2016 13:06

removed status updates for delivery reports

c86fa6a

update status of event component when a good event is received

a3bd385

KaitCrawford merged commit 767f507 into develop Mar 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup Vumi bridge and add Junebug support #1028

Cleanup Vumi bridge and add Junebug support #1028

KaitCrawford commented Mar 22, 2016

hodgestar Mar 22, 2016

hodgestar commented Mar 22, 2016

hodgestar commented Mar 22, 2016

rudigiesler commented Mar 22, 2016

hodgestar commented Mar 22, 2016

KaitCrawford commented Mar 23, 2016

hodgestar commented Mar 24, 2016

hodgestar Mar 24, 2016

hodgestar commented Mar 24, 2016

KaitCrawford commented Mar 24, 2016

rudigiesler commented Mar 24, 2016

hodgestar commented Mar 24, 2016

hodgestar commented Mar 24, 2016

hodgestar commented Mar 24, 2016

KaitCrawford commented Mar 24, 2016

hodgestar Mar 24, 2016

KaitCrawford Mar 24, 2016

hodgestar commented Mar 24, 2016

KaitCrawford commented Mar 29, 2016

hodgestar Mar 29, 2016

KaitCrawford commented Mar 29, 2016

hodgestar Mar 29, 2016

KaitCrawford Mar 29, 2016

hodgestar Mar 29, 2016 via email

hodgestar commented Mar 29, 2016

KaitCrawford commented Mar 30, 2016

hodgestar commented Mar 30, 2016

KaitCrawford commented Mar 30, 2016

hodgestar commented Mar 30, 2016

Cleanup Vumi bridge and add Junebug support #1028

Cleanup Vumi bridge and add Junebug support #1028

Conversation

KaitCrawford commented Mar 22, 2016

hodgestar Mar 22, 2016

Choose a reason for hiding this comment

hodgestar commented Mar 22, 2016

hodgestar commented Mar 22, 2016

rudigiesler commented Mar 22, 2016

hodgestar commented Mar 22, 2016

KaitCrawford commented Mar 23, 2016

hodgestar commented Mar 24, 2016

hodgestar Mar 24, 2016

Choose a reason for hiding this comment

hodgestar commented Mar 24, 2016

KaitCrawford commented Mar 24, 2016

rudigiesler commented Mar 24, 2016

hodgestar commented Mar 24, 2016

hodgestar commented Mar 24, 2016

hodgestar commented Mar 24, 2016

KaitCrawford commented Mar 24, 2016

hodgestar Mar 24, 2016

Choose a reason for hiding this comment

KaitCrawford Mar 24, 2016

Choose a reason for hiding this comment

hodgestar commented Mar 24, 2016

KaitCrawford commented Mar 29, 2016

hodgestar Mar 29, 2016

Choose a reason for hiding this comment

KaitCrawford commented Mar 29, 2016

hodgestar Mar 29, 2016

Choose a reason for hiding this comment

KaitCrawford Mar 29, 2016

Choose a reason for hiding this comment

hodgestar Mar 29, 2016 via email

Choose a reason for hiding this comment

hodgestar commented Mar 29, 2016

KaitCrawford commented Mar 30, 2016

hodgestar commented Mar 30, 2016

KaitCrawford commented Mar 30, 2016

hodgestar commented Mar 30, 2016