-
Notifications
You must be signed in to change notification settings - Fork 1
Add protocol and configuration bindings for outbound SNS. #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e2df7a1 to
494a953
Compare
raghuramg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few comments—could you please review them?
In addition to the new protocol implementation, we also improved error messaging, introduced a base channel proxy, and cleaned up the existing operations. I'm happy with these changes.
Absolutely, basic gist of the comments (which I agree with whole heartedly) is the areas in which you have questions I don't provide sufficient documentation or explain what/why I'm doing what is happening in the code. I will do so before I re-submit. |
494a953 to
46754ca
Compare
This adds protocol and configuration bindings for outbound SNS messaging using AWS, particularly for SMS messages. Testing is still pending, as a working application generating events is needed to verify the implementation.
46754ca to
44a6ca9
Compare
raghuramg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only comment is that I didn't see a re-connection explicitly in the code. I guess that is to keep the current behaviour of the gem; however, for some reason, I think this new kind of connection is more prone to disconnections. But overall, great work
This adds protocol and configuration bindings for outbound SNS messaging using AWS, particularly for SMS messages.
Testing is still pending, as a working application generating events is needed to verify the implementation.
https://app.clickup.com/t/868d7e750