search

Admiral-Piett / goaws

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

Sns publish batch #314

Open goddenrich opened 1 month ago

goddenrich commented 1 month ago

This is a clone of #274 but with the test assert.Lenght fixed

goddenrich commented 1 month ago

ref #270

goddenrich commented 1 month ago

One thing I noticed is you were adding support for aws json so this may need to be added to the list of things which need aws json support

Admiral-Piett commented 1 month ago

@goddenrich Thanks for working on this! I really appreciate it. As you pointed out we're in the middle of the JSON upgrade. We're not merging any PRs against master until that's out to minimize the confusion. Let's revisit this after that update is complete, it's almost done, but there's a pattern to those changes that we're going to enforce on all future endpoints, so we'll need to adhere to it too. Happy to talk through that with you and provide examples, if you want to get ahead or have any questions about it. Thanks again for taking the initiative!

goddenrich commented 1 month ago

@goddenrich Thanks for working on this! I really appreciate it. As you pointed out we're in the middle of the JSON upgrade. We're not merging any PRs against master until that's out to minimize the confusion. Let's revisit this after that update is complete, it's almost done, but there's a pattern to those changes that we're going to enforce on all future endpoints, so we'll need to adhere to it too. Happy to talk through that with you and provide examples, if you want to get ahead or have any questions about it. Thanks again for taking the initiative!

Sure I'm glad that's getting attention. It's blocking us upgrading aws-sdk-go too so pleased that looks like it will be done soon. I'll take a closer look at the patterns from the other json submissions and may give reworking this to align it so it's ready when the other APIs are implemented. I'll let you know if I have any questions

goddenrich commented 3 weeks ago

fyi @Admiral-Piett Ive mostly refactored things to follow the aws json feature work. I have also rebased this onto that feature branch. I still need to add smoke tests but would appreciate it if you could take a look at some point

goddenrich commented 1 week ago

@Admiral-Piett I have made the changes you have suggested or responded. I have also added the smoke tests (modeled on sns_publish_test.go. Could you take another look. I also notice none of the smoke tests within smoke_tests directory pass but I guess that is a known problem given the state of all the other tests.

Admiral-Piett commented 1 week ago

@Admiral-Piett I have made the changes you have suggested or responded. I have also added the smoke tests (modeled on sns_publish_test.go. Could you take another look. I also notice none of the smoke tests within smoke_tests directory pass but I guess that is a known problem given the state of all the other tests.

Thanks for taking another look at this! I appreciate the thoroughness. I see your one note, and I have one more question about it otherwise I think this is ok to move on with, at least pending the following thought.

What do you mean about the smoke_tests not working? That's not known to me anyway. They work for me when I run them. What error(s) are you getting? What command are you using?

goddenrich commented 1 week ago

@Admiral-Piett I have made the changes you have suggested or responded. I have also added the smoke tests (modeled on sns_publish_test.go. Could you take another look. I also notice none of the smoke tests within smoke_tests directory pass but I guess that is a known problem given the state of all the other tests.

Thanks for taking another look at this! I appreciate the thoroughness. I see your one note, and I have one more question about it otherwise I think this is ok to move on with, at least pending the following thought.

What do you mean about the smoke_tests not working? That's not known to me anyway. They work for me when I run them. What error(s) are you getting? What command are you using?

Ah I have just checked again and noticed the README.md with the bash exports. All other tests are now running and passing the publish batch ones aren't so I will figure that out and let you know. Thanks

goddenrich commented 1 week ago

Thats the smoke tests for publish batch passing

Admiral-Piett commented 1 week ago

Sorry, I actually didn't mean to override you in this thread - https://github.com/Admiral-Piett/goaws/pull/314#discussion_r1717401772. I was trying to get to the root of what AWS does since their documentation is unclear. Did you try it out in AWS? If not, I will, but then you'll be beholden to my availability sadly. Regardless let me know what you think. I think we're very close, and I appreciate you sticking with it here.

goddenrich commented 6 days ago

Sorry, I actually didn't mean to override you in this thread - #314 (comment). I was trying to get to the root of what AWS does since their documentation is unclear. Did you try it out in AWS? If not, I will, but then you'll be beholden to my availability sadly. Regardless let me know what you think. I think we're very close, and I appreciate you sticking with it here.

I don't really have the time to dedicate much more time to this to spin up an instance and check sorry. I've also just been told we might be migration off goaws so feel I've spent all the time that I can commit to this issue already. Sorry

Admiral-Piett commented 1 day ago

@goddenrich Alright, thanks for your help so far! Though that sucks you don't want to use the tool anymore.

I will likely make a branch off of your fork, do some testing and potentially tweaking, and merge when I can, if it's ok with you that I piggy back on what you started. Then this will likely close in favor of that PR. Thanks for getting this started though. I really appreciate it!

goddenrich commented 1 day ago

@goddenrich Alright, thanks for your help so far! Though that sucks you don't want to use the tool anymore.

I will likely make a branch off of your fork, do some testing and potentially tweaking, and merge when I can, if it's ok with you that I piggy back on what you started. Then this will likely close in favor of that PR. Thanks for getting this started though. I really appreciate it!

Yea sounds good. Sorry to drop it upon you without managing to finish it off