Implement message attribute parsing for PublishBatch

[toastwaffle]

Factor out attribute parsing for SendMessage, SendMessageBatch, and Publish in the process.

I believe this change is adequately covered by the smoke tests, but am happy to be convinced otherwise

[Admiral-Piett]

One small thing, but otherwise this looks great! Thanks for cleaning up a lot of my duplicate code!

[Admiral-Piett]

@toastwaffle Hm, looks like the tests are failing sadly. Would you mind having a look? Also, would you mind squashing your commits into a single one when you're done? It's easier for me to figure out each change in retrospect that way. Thank you for taking this on!

[toastwaffle]

I believe that test is flaky on master:

for i in {1..10}; do go test -cover -race ./app/gosqs -count 1 -run ^TestDeadLetterQueue$; done
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.045s  coverage: 26.7% of statements
time="2024-11-15T22:53:45Z" level=info msg="Creating Queue: testing-deadletter"
time="2024-11-15T22:53:45Z" level=info msg="2024-11-15 22:53:45: Queue: testing-deadletter, Message: 1\n"
--- FAIL: TestDeadLetterQueue (4.00s)
    gosqs_test.go:314: expected no message in testing-deadletter
FAIL
coverage: 26.3% of statements
FAIL    github.com/Admiral-Piett/goaws/app/gosqs        4.049s
FAIL
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.046s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.053s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.045s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.045s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.056s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.043s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.057s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.040s  coverage: 26.7% of statements

Note that running it with -count 10 makes it fail every test after the first - I think that flag runs all the tests within the same process, and the test is leaving some global state around. -count 1 is required in the for-loop to prevent Go from caching the test

I ran it on this PR branch and it actually passed every time:

for i in {1..10}; do go test -cover -race ./app/gosqs -count 1 -run ^TestDeadLetterQueue$; done
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.058s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.040s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.051s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.040s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.051s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.055s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.051s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.054s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.057s  coverage: 26.7% of statements
ok      github.com/Admiral-Piett/goaws/app/gosqs        5.043s  coverage: 26.7% of statements

[toastwaffle]

Actually, seq -w 0 999 | parallel -P 0 go test -cover -race ./app/gosqs -count 1 -run ^TestDeadLetterQueue$ is a better command for running the tests - that way you get some parallelism instead of it being fully sequential (which is a pain when the test itself takes 5s). With that I got 24 test failures out of 1000 on master

[Admiral-Piett]

@toastwaffle Yeah, that's a known issue, we're not ready to go into parallelism on the tests. There's a lot of memory management and sharing going on that parallel tests would just cause parallel tests to implode on themselves. Like I said though, known.

I also know the Dead Letter queue is dicey. If you're blocked on this. I will try to look at it when I can and you can rebase after I fix that one up. Up to you.

[toastwaffle]

We basically have this PR as an internal patch, so we're not exactly blocked. We'd prefer to not have the patch at all, but it's not doing any harm right now. I'm happy to wait until you have time to look into the flaky test if you'd rather not merge this before fixing it.

[Admiral-Piett]

Makes sense, thanks for understanding! I'll try to have a look at that test as soon as possible, so we can get this rolling. Thank you!

[Admiral-Piett]

@toastwaffle Ok, thanks for hanging in there. I think I got a few of those problematic tests sorted for now. A couple others I'll migrate, but for now I hope you're clear. Would you mind doing a quick rebase when you have a chance? I can't seem to fire the PR checks again without it.

[toastwaffle]

I've rebased, but Test_CreateTopicV1_json_success seems to be failing on master (on my machine at least) with an error like operation error SNS: CreateTopic, get identity: get credentials: failed to refresh cached credentials, no EC2 IMDS role found, operation error ec2imds: GetMetadata, request canceled, context deadline exceeded

aside: it actually fails with a segfault; I had to change the test to

	require.NoError(t, err)
	assert.Contains(t, *sdkResponse.TopicArn, topicName)

To be able to see the error message (assert.Nil/require.Nil outputs &smithy.OperationError{ServiceID:"SNS", OperationName:"CreateTopic", Err:(*fmt.wrapError)(0xc0004240c0)} without the actual error message)

[Admiral-Piett]

I don't think the credentials thing is something to worry about for now, unless we absolutely have to. I think Goaws is supposed to be avoiding dealing with any credentials, other than just blindly accepting them, and I think there may a few disconnects in some of the SDKs when they see some scenarios. Though so far I have been able to get work arounds. If I need to make that more robust I will but it's going to take some significant investigation, since I have so far been avoiding it like the plague.

The tests seem to pass now and that's good enough for me, so let's merge this. Feel free to follow up with me though if you have issues once I get this into the new version? 😄