Skip to content

Conversation

@TreyE
Copy link
Contributor

@TreyE TreyE commented May 5, 2025

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

@TreyE TreyE force-pushed the sms_outbound_protocol branch from e2df7a1 to 494a953 Compare May 5, 2025 22:37
@TreyE TreyE added the enhancement New feature or request label May 5, 2025
Copy link
Contributor

@raghuramg raghuramg left a 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.

@TreyE
Copy link
Contributor Author

TreyE commented May 12, 2025

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.

@TreyE TreyE force-pushed the sms_outbound_protocol branch from 494a953 to 46754ca Compare May 12, 2025 15:26
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.
@TreyE TreyE force-pushed the sms_outbound_protocol branch from 46754ca to 44a6ca9 Compare May 12, 2025 15:51
Copy link
Contributor

@raghuramg raghuramg left a 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

@TreyE TreyE requested a review from mdkaraman May 14, 2025 13:16
Copy link

@polographer polographer left a 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

@TreyE TreyE merged commit c12269a into trunk May 14, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants