p4tin
Sns publish batch
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
@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 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
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
@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 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?
@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
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.
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
@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 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
@Admiral-Piett I have quickly merged changes from the json feature branch and master into this PR and reset it to merge into master rather than the feature branch
@Admiral-Piett I have quickly merged changes from the json feature branch and master into this PR and reset it to merge into master rather than the feature branch
Thanks @goddenrich - could you also squash your commits? Otherwise I'll get lost in the sauce so to speak. Thanks!
Sorry I am going to have to copy this into a new branch. The way we this kind of merged in will wreak havoc in the master branch. No sweat though, I appreciate the start. I'll pull it over and finish it off. Thanks @goddenrich !