Add Action mailbox SES

[ssunday]

~Hot mess of a PR. Very much not done.~

Basically, this is attempting to merge in https://github.com/bobf/action_mailbox_amazon_ingress

- Need to confirm license changes for other folks

• ~Need to finish switching to rspec~
• ~Need to figure out how to get active storage migration in dummy app~
• ~Add docs~
• ~Test it actually works~

~I'm not sure when I'll get to wrapping this up/working on this again...so anyone is free to bring it to the finish line.~

[mullermp]

Thanks for starting this effort. We do support older Rails 5.2. We are not opposed to a major version bump that has a new minimum rails of 7 if it makes things simpler. As mentioned, we're not opposed to rspec either, let's just make sure that the existing test cases are covered. This will probably be a large change and review for sure. Is there a way to do this in manageable bite sized pieces? Maybe we do the rspec conversion of aws-sdk-rails as is, without the action mailbox changes, and then we do that one?

[ssunday]

@mullermp agreed with breaking it out.

[mullermp]

There are a lot of conflicts here. Let me know how you want to proceed before making sweeping changes.

[ssunday]

@mullermp I'm going to rebase and just get the important mailbox stuff in (and its tests). I'm working on it!

[ssunday]

not working yet, routing isn't functional..like the routes aren't being mounted.

[ssunday]

i'll continue looking into it eventually

[mullermp]

@bobf Would you be interested in reviewing and helping get this into aws-sdk-rails?

[ssunday]

Got tests working. I will need to test on an app this works e2e, and we also need to confirm everyone is OK with the license.

[mullermp]

You should add it into sample_app and then a section on the README for how to test with it.

[ssunday]

@mullermp yep, I need to pull in the docs from the other repo.

[ssunday]

@mullermp I'm not sure how to locally test with the sample_app (I mean in this use case specifically).

[bobf]

@bobf Would you be interested in reviewing and helping get this into aws-sdk-rails?

Yep, sure. I'll try to find some time this week.

[ssunday]

I have another speculative fix for the tests/database issue—I'll push it up once I separately do an e2e test on a project that these changes even work 🤡

[bobf]

@ssunday @mullermp I've scanned through the code (admittedly not very thoroughly, but I did read everything) and this all looks good to me.

Is there any specific feedback you want from me, or anything I can do to help this along ?

@ssunday I've lost track of how many times I've thanked you for all this hard work but it still seems insufficient - it's great to see somebody taking charge of this.

[ssunday]

@bobf Early next week I'm going to get this code e2e tested on a real project so that'll test if it actually works in the wild. Once that's done, I just have to get tests working (which I have a speculative change I didn't push up since...I didn't want to push if the e2e test proved things needed more work anyway). So I've got those two things handled/planned for next week.

The thing I'm not sure of is licensing change since other people worked on this and I'm just doing the 'glue' work of getting it into this repo. Do they need to sign-off on it being into this repo with a diff license or is that not necessary with it from an MIT-origin repo?

Thank you for like getting this whole ball rolling and actually doing the hard work haha. This integration has greatly improved QOL on two of my projects. Having it as a proper package is just great for long term maintenance...

[bobf]

I believe I'm able to authorise a licence change. As long as the change is to a similar license (Apache, BSD, etc. then all good with me).

[ssunday]

Looks like it's working e2e.

[mullermp]

Were you able to test locally and is it possibly to add a "how to test" entry and integrate with sample app in this repo?

[ssunday]

@mullermp Well, the code I brought over comes with rspec examples that test the code. I can add those rspec integrations to the sample app....BUT then I'd need to convert the sample app to rspec from mini test.

Beyond that, there's not really anything I can think of—honestly, most of the work in setup is on AWS. You configure two things in the app and then it just works assuming AWS SES/SNS is setup properly.

[bobf]

Agree on this - the gem just expects a JSON payload with the correct structure and signature, so I don't know how else to test it other than with fixtures.

When I've run this locally I've had to use ngrok which is a bit of a pain.

[mullermp]

That seems fine. Maybe an entry in that testing file about why NOT to have that and how fixtures are sufficient. We often do those manual / integration tests when we make "uneasy" changes just to be certain.

[mullermp]

I'll be able to do a deeper review of this tomorrow or Thursday. Thank you both for your efforts.

[ssunday]

@mullermp Cool! I'll hold on doing anything until after you do the deeper review. Thank you so much!

[ssunday]

trying to fix tests still 😦

[ssunday]

Yay, tests passing...except for rails main and jruby. The rails man ones look unrelated or general regression—can we run CI against main, perhaps?

Also, side question-why do we run CI against rails-main? It's not an officially supported release and unstable. It seems like you would not run it/support it and only support cut releases.

[ssunday]

I don't know how to debug jruby/never worked with it, so I'd appreciate help

[ssunday]

I'm wondering if sqlite3 is not compatible with jruby period/doesn't play well (read something that said that).

[ssunday]

Okay, migration change failed

[ssunday]

oh yeah, I removed the migrations, duh, I'll fix 🤦🏻

[ssunday]

The remaining requests are pretty large, not sure when I'll get to it. Anyone else is free to dive in and work on them 👍🏻 .

[ssunday]

Actually, I'll work on the renaming of the routes (amazon -> ses)

[mullermp]

The main branch failures look related to the PR, because main in this repo passes just fine (and the other PR you saw). I think maybe the route is interfering, you can see by the stack trace. I can help take a look at some of the items here.

[ssunday]

@mullermp oooh, I wonder if it's because of the transactional fixtures. I'll try w/o that and see how that goes.

If you could look into the jruby stuff, that'd be great. I'm lost with it haha

[mullermp]

Ok, I spent some time trying to figure out what was going on. The original jruby failure was because of the bundler cache I think, so I reverted that bit, then desperately tried a few things but I believe I got it all working. The JDBC adapters track Rails versions so I had to conditionally pull those, and in the case of Rails 7.1 it was never published for some reason (opened an issue on their repo) and for 7.2 we will have to skip this case now. Is it possible to avoid any kind of database in the dummy for adding this feature?

[ssunday]

@mullermp Is it possible to avoid any kind of database in the dummy for adding this feature? Well, then it's not really testing the inbound emails flow actually works. It feels like a massive hit to testing the feature.

🤯 on the jruby nonsense. Great work on figuring it out. With jruby—is it...that popular? Do we need to be that concerned over thoroughly supporting it versus having the ideal situation for ruby plain?

[ssunday]

Rephrasing: I think it's more important to have the database tests to verify this feature works in some cases than having parity of testing for all ruby versions (if that makes sense).

[ssunday]

@mullermp Okay, re-reading. Not sure order of your comments/commits anymore. It looks like everything's green, so it seems like there actually isn't an issue with the database setup anymore?? It's just the memoization and maybe the fixtures?

[mullermp]

Yeah, I think if we can dynamically get the certificate for security reasons (let me look into this if it's true), and we'll want to memoize the S3 client somehow and also allow for options to be passed in - we want to allow for configuring an S3 client via code. Also @alextwoods is back from vacation and I want him to also review this.

If you can think of anything else, let's clean this up some more.

[ssunday]

@mullermp I can't think of anything else rn—I know there's a request to handle KMS encryption (https://github.com/bobf/action_mailbox_amazon_ingress/pull/8) but that can be a subsequent release.

I'll need to test e2e once all the last changes are made. I'll hold off until Alex reviews and changes are made based on that/the other stuff.

[mullermp]

Sounds good. It would be great if we can get a sample_app test if possible and/or manual test instructions using sample_app.

[ssunday]

Sounds good. It would be great if we can get a sample_app test if possible and/or manual test instructions using sample_app.

I don't really feel like that's going to be that useful. In terms of code, there's only like 2 config things you setup which are already documented. The rest is in AWS.

[mullermp]

Sorry, we had discussed that (I forgot - my bad! - a lot going on)

[ssunday]

@mullermp Absolutely no problem, very much understand!

With the certificate stuff, I think if you look at how the test code is using it, you'll see that the persisted certs are like...whatever. They can be regenerated by anyone and the fixtures we saved without issue. Like they aren't...real in the eyes of AWS. You can make one and regen test fixtures. Like they aren't supposed to be an actual cert from AWS, but in place of via stubbing. That's my take on it.

[mullermp]

Understood and I agree . AWS security flags this kind of stuff and will ultimately bug me about it. Let me just confirm with them that it's ok.

[ssunday]

rebased + force pushed

[ssunday]

I think setting the dummy app action job driver to SQS will cause CI to fail—I'd like to make it test mode. It's failing locally.

[mullermp]

I think setting the dummy app action job driver to SQS will cause CI to fail—I'd like to make it test mode. It's failing locally.

I didn't see this happening when I ran locally

[ssunday]

@mullermp were you running against rails main gemfile?

[ssunday]

CI did fail in the way that I expected

[ssunday]

@mullermp I think it's cause of the rails-main transaction thing with enqueing. It doesn't try to enqueue on non-rails-man, thus no error with the SQS driver. But with rails-man...it does so it blows up as we haven't stubbed sqs and it tries to call out to AWS.

I still say we switch to test mode for the dummy app so features are tested in isolation.