-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add Action mailbox SES #127
Conversation
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? |
@mullermp agreed with breaking it out. |
There are a lot of conflicts here. Let me know how you want to proceed before making sweeping changes. |
@mullermp I'm going to rebase and just get the important mailbox stuff in (and its tests). I'm working on it! |
not working yet, routing isn't functional..like the routes aren't being mounted. |
i'll continue looking into it eventually |
@bobf Would you be interested in reviewing and helping get this into aws-sdk-rails? |
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. |
You should add it into sample_app and then a section on the README for how to test with it. |
@mullermp yep, I need to pull in the docs from the other repo. |
@mullermp I'm not sure how to locally test with the sample_app (I mean in this use case specifically). |
Yep, sure. I'll try to find some time this week. |
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 🤡 |
@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. |
@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... |
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). |
Looks like it's working e2e. |
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? |
I'll be able to help with this tomorrow and hopefully we can get this merged by early next week. |
@mullermp I need to test e2e again with it—I should be able to do that today after I make the feedback changes. |
@mullermp Well, I don't have to, but it seems like it's a good idea since the amount of changes since my last e2e change. |
An e2e test from your side would be great. |
Sorry I've been a bit MIA - I haven't forgotten about this. I may not get to this properly until next week. |
@mullermp I'm going to hold on e2e testing until the PR's basically approved, then. I don't want to spend time e2e testng on a project then have to do more critical changes and then do it again 😅 |
It's basically there other than Alex's comments. I can do another pass potentially this week or next week at the latest. |
@mullermp The rakefile comment seems like it shouldn't be a blocker since it's test/cleanup related (and I don't feel strongly/motivated to actually do anything about it). Still, I'd rather wait 😅 |
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.
Nice work! I think this is finished. I agree that subscribed topics (plural) can come later and any fixture refactors can also be done later. I've moved them into a separate rake task for organization. I think a last end to end review would be appreciated and we can try to publish this or next week.
E2E went fine so far! 👍🏻 (email with attachment handled fine!) So all good IMO |
The description still has "Need to confirm license changes for other folks". Has that been done? What are the license changes/issues? |
I believe so, bobf said it was fine to be Apache which we have here. |
Great work, everyone. @ssunday Let me know if I can send a case of beer or something similar your way, I really do appreciate you getting this all wrapped up. |
@bobf Thank you! The existence of this code in general has greatly improved the QOL projects and infrastructure... less 💦 for me! Now it's in a nice cozy home lol. |
thanks @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 rspecNeed to figure out how to get active storage migration in dummy appAdd docsTest it actually worksI'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.