Admiral-Piett / goaws

AWS (SQS/SNS) Clone for Development testing
MIT License
767 stars 144 forks source link

[aws-json] Migrate SNS CreateTopic #305

Closed kojisaikiAtSony closed 10 hours ago

kojisaikiAtSony commented 1 month ago

Hi @Admiral-Piett, this is ready for review but there are some discussion points.

Admiral-Piett commented 2 weeks ago

Sooo - I've done some work here with the SubscribeV1 endpoint. I have some additional thoughts around what we've done here. Give me a minute to get it out? It'll affect what you've got going here, thank you!

I am also seeing what you're talking about, in terms of the SDK being behind on the SNS JSON. Based on what I can see - if we move over the rest of the SQS stuff we'll be up to date. I'm thinking about what to do here, since I can't find "future state" docs on what their API will look like in JSON mode. I'm leaning toward just focusing on the last 2 SQS bits and then releasing what we have. Then we can continue to work on updating the models behind the scenes and when AWS gets to SNS json, we can proceed with updating the models and just testing against that format. Give me a bit to decide though? I want to make sure we're not spinning our wheels, and not guessing at what they MIGHT do, and that I'm being sensitive of your time with this.

Thanks again as always for your help here! You've been awesome!

kojisaikiAtSony commented 2 weeks ago

@Admiral-Piett No problem! 👍 The SNS work will wait for a while. I will continue with the remaining SQS work (SendMessageBatch/DeleteMessageBatch) and prepare a PR as soon as it is ready!

Admiral-Piett commented 2 weeks ago

Ok my friend, I got the PR out for SubscribeV1 - take a look when you can? It's a lot (I know, sorry), but most of it is name changes and/or just other clean up. Feel free to take a look at any or all that interest you, but the things that are relevant to our chats here I think are things like this:

https://github.com/Admiral-Piett/goaws/pull/308/files#diff-a5d83c83bae68ac249d8bcd0787cb15c7ab089eba854bdbbfd3b04089845bf1cR5-R27

https://github.com/Admiral-Piett/goaws/pull/308/files#diff-b3ff3a2f881b5d4e79b1f16a222d88e909e9574c3454fc8fa946ee02b054df3dR80-R84

https://github.com/Admiral-Piett/goaws/pull/308/files#diff-8d91dca3390f4e1ec17f8b11e22da981111ec1f99936a9830aac8585012761f2R1

https://github.com/Admiral-Piett/goaws/pull/308/files#diff-d1057aa8116e3cb99eaef5b6f4ebd728f6bf026b1274a172d9f8fc96988f1f9cR1

Just trying to break this stuff up and organize it however it fits most clearly. I'll give this a couple days, for you to have a look at, but if you want me to merge it, just say so? Also, if you have any other ideas open to discussion as always.

I say at this point, let's merge the SubscribeV1 and then this CreateTopic piece, then we can knock out the last of the SQS routes, merge our feature branch and release it. Then continue to merge the SNS "on the new pattern" commits into master as a fast follow. I'll hold all other PRs in limbo until we're done in and we're all tightened up. Thoughts?

kojisaikiAtSony commented 2 weeks ago

@Admiral-Piett OK, I'll take a look #308 soon.

I say at this point, let's merge the SubscribeV1 and then this CreateTopic piece, then we can knock out the last of the SQS routes, merge our feature branch and release it. ...

So it's time to finish the feature-aws-json branch and continue the "aws-json development" on master branch, right? Sounds good to me! 👍 The new architecture seems stable, and the basic SQS APIs are now in place, so I agree we can deliver this to users!

Admiral-Piett commented 2 weeks ago

@kojisaikiAtSony awesome! That's our plan then. I saw your comments, I'll respond to those soon, thank you for those! Have you started on the batch endpoints yet? Is there one that you prefer that I take?

kojisaikiAtSony commented 2 weeks ago

@Admiral-Piett SendMessageBatch is almost completed and DeleteMessageBatch is ongoing as well. It should be not so late 👍

kojisaikiAtSony commented 1 week ago

Hi @Admiral-Piett, I updated the branch!

kojisaikiAtSony commented 6 days ago

@Admiral-Piett updated!