Skip to content
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

Implement SNS:PublishBatch #274

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

perrydunn
Copy link
Contributor

tests to be added
(already using with a project successfully)

@perrydunn
Copy link
Contributor Author

ref #270

Copy link
Owner

@Admiral-Piett Admiral-Piett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I had a couple minor questions, curious what you think about them when you have time. Let me know about those tests and I'll get it marked for the next feature release!

Thanks for taking this on!

app/gosns/gosns.go Show resolved Hide resolved
app/gosns/gosns.go Outdated Show resolved Hide resolved
app/gosns/gosns.go Show resolved Hide resolved
}

// StringListValue and BinaryListValue is currently not implemented
for _, valueKey := range [...]string{"StringValue", "BinaryValue"} {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone provided both StringValue and BinaryValue would we overwrite StringValue? If so, is that what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the docs this needs refinement. I took this from the SNS:Publish (gosns.Publish) implementation (which may also need some rework I'm not going to touch it here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: Correction, the SNS docs are unclear on the behaviour here. I'll do what the localstack solution does...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I like your pattern here better anyway - it's clearer about what we're doing. We can dig deeper on how exactly it's supposed to work later.

@perrydunn
Copy link
Contributor Author

Last commit requires #277

@Admiral-Piett
Copy link
Owner

Hm, I think we're close here - looks like the tests are failing on assert.Length which I don't see as a function in the testify library? I think that's probably it.

@Admiral-Piett
Copy link
Owner

@perrydunn quick follow up here. Any thoughts on this?

@goddenrich goddenrich mentioned this pull request Jul 12, 2024
@Xifong
Copy link

Xifong commented Oct 30, 2024

Hi, is this issue resolved now that #314 has merged? Additionally, are there any plans for these changes to be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants