Skip to content

Support for pushing data to external services - #5686

Merged
garethbowen merged 18 commits into
masterfrom
5499_outbound_rest_feature
Jun 19, 2019
Merged

Support for pushing data to external services#5686
garethbowen merged 18 commits into
masterfrom
5499_outbound_rest_feature

Conversation

@SCdF

@SCdF SCdF commented May 22, 2019

Copy link
Copy Markdown
Contributor

Basic support for pushing arbitrary JSON data to a REST endpoint based
on data in a data_record. Support for Muso SIH's authentication method
as well as Basic Auth.

Documentation: https://github.com/medic/medic-docs/blob/master/configuration/outbound.md

#5499

@SCdF
SCdF requested a review from kennsippell May 22, 2019 12:34
@SCdF
SCdF marked this pull request as ready for review May 22, 2019 12:44

@kennsippell kennsippell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feedback is mostly about variable naming and interfaces.

Comment thread sentinel/src/schedule/outbound.js Outdated
Comment thread sentinel/src/schedule/outbound.js Outdated
Comment thread sentinel/src/schedule/outbound.js Outdated
Comment thread sentinel/src/schedule/outbound.js Outdated
Comment thread shared-libs/transitions/src/transitions/mark_for_outbound.js Outdated
Comment thread sentinel/tests/unit/schedule/outbound.js
Comment thread sentinel/tests/unit/schedule/outbound.js
Comment thread sentinel/tests/unit/schedule/outbound.js
Comment thread sentinel/tests/unit/schedule/outbound.js
Comment thread sentinel/tests/unit/schedule/outbound.js Outdated
@SCdF
SCdF requested a review from kennsippell May 23, 2019 18:53
@SCdF

SCdF commented May 24, 2019

Copy link
Copy Markdown
Contributor Author

@kennsippell I also added this last commit: bb6dc19 (and me fixing the test: f94a155 )

Which extracts out the singlePush function as discussed, but also adding a todo that I forgot earlier, which is adding a mark to the -info doc on success.

@kennsippell kennsippell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating all the nit-picky stuff... I think this is getting close!

Would you also resolve merge conflicts for the next one?

Comment thread sentinel/src/schedule/outbound.js Outdated
Comment thread sentinel/src/schedule/outbound.js Outdated
Comment thread sentinel/src/schedule/outbound.js
Comment thread sentinel/src/schedule/outbound.js Outdated
Comment thread sentinel/src/schedule/outbound.js Outdated
Comment thread sentinel/tests/unit/schedule/outbound.js
Comment thread sentinel/tests/unit/schedule/outbound.js
Comment thread sentinel/tests/unit/schedule/outbound.js Outdated
Comment thread shared-libs/transitions/src/transitions/mark_for_outbound.js
Comment thread shared-libs/transitions/src/transitions/mark_for_outbound.js
@kennsippell

kennsippell commented May 27, 2019

Copy link
Copy Markdown
Member

It's obviously a bit late for this, but I did want to make a note on some architecture feedback for this PR. I wasn't involved in the design of this feature - so I'm coming at this with hindsight only and perhaps an slightly incomplete understanding of the requirements. It's likely not actionable at this point, but just tossing my hat into the ring for how we evolve this feature over time.

The first thing we're making here is a pretty simple queuing system which uses CouchDB docs to track objects in the queue. There are already some comments about potential optimisations [1] [2]. And some questions about next steps in review -- removing items after they fail x times, alerting about failed items, etc. imo -- queues are easy to build a first version, but costly to scale and perfect. Instead of pushing a doc to into CouchDB, this use-case seems like a good candidate for systems like AWS SQS or alike. Low-cost, tested, high-scale, parallel ready. imo by re-using instead of re-building, we could get more by building less.

The second is our new extensible system for transforming documents into the format expected by whatever service we integrate with. Extensibility is awesome! But instead of provided a single core transition which we hope will fit all integrations, we could think about have partners be able to write their own plug-ins or sentinel transitions. We have seen SIH-specific requirements creep into this first iteration already. I just think it would be a more robust tool to provide to partners which is less code and less platform for us to maintain. If the above SQS suggestion is liked, AWS Lambda could be an excellent mechanism to allow partners to process their queue.

@SCdF
SCdF force-pushed the 5499_outbound_rest_feature branch 3 times, most recently from 2def1b3 to da85e64 Compare May 30, 2019 14:05
@SCdF
SCdF requested a review from kennsippell May 30, 2019 14:50
Comment thread sentinel/src/schedule/outbound.js
Comment thread sentinel/src/schedule/outbound.js Outdated
@garethbowen
garethbowen force-pushed the 5499_outbound_rest_feature branch from d161a74 to a6d105f Compare June 19, 2019 20:59
@garethbowen
garethbowen merged commit 0ea69a8 into master Jun 19, 2019
@garethbowen
garethbowen deleted the 5499_outbound_rest_feature branch June 19, 2019 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants