Feature/sns identiy notifications for ses

drewblas / aws-ses

Provides an easy ruby DSL & interface to AWS SES

MIT License

549 stars 109 forks source link

Feature/sns identiy notifications for ses #62

Closed oma closed 7 years ago

oma commented 7 years ago

Identity Notifications:

[x] get identity notifications (SNS topics for SES bounces etc)
[x] register identity notifications in topics Bounce | Complaint | Delivery
[x] delete identity notifications
[x] unit tests
[x] production tested
list multiple identities notifications (don't find it useful although API supports it)

Identity

[x] list email identities
[x] list domain identities
[x] unit tests
[x] production tested
in CRUD I only did the R, didn't need more.

drewblas commented 7 years ago

This certainly looks very useful. I'd be willing to merge it in. But we can't change the version stuff. Let's merge in just the identities support.

For removing the version there are several issues:

You say it's "not used". You're correct it's not used internally. But the point of the class is for consumers of the gem to be able to programatically query which version of the gem they have loaded. You've removed it and not replaced it with an alternative that accomplishes that goal.
If there is a "less complex" approach that accomplishes that goal, we still can't make a breaking change without a major version change. Anyone relying on calling AWS::SES::VERSION would have broken code for no reason.

oma commented 7 years ago

Ok, I hear you. Currently gemspec says 0.6.0 while the lib/aws/ses/version.rb file says 0.4.4, and there's a third one, a top level VERSION file also saying 0.6.0, they should be the same.

Normally the version.rb file is read to set the gemspec version so it's a single source of truth. I can do that, but then I'll need to update the version file. To 0.6.0 or to 0.7.0 ? Or do you want to do that? Same for me, I didn't plan to contribute here I just needed SNS listeners for SES. Happy to be helpful and I appreciate the work you've done here.

oma commented 7 years ago

I reverted and changed to use the version file. It's 0.4.4, so if anybody depend on it I think it's a mistake. At least I'm confused, would be good to clean up :)

I can't build the gemspec or the gem and I ran out of time. For now I use gem 'aws-ses', github: 'paladinsoftware/aws-ses', branch: 'feature/sns_identiy_notifications_for_ses', require: 'aws/ses'