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

fix(aws): change default handler context key #922

Merged

Conversation

dbyron-sf
Copy link
Contributor

@dbyron-sf dbyron-sf commented Nov 10, 2021

The aws sdk places a value in HandlerContextKey.OPERATION_NAME (e.g. GetObject) at least
some of the time, so use a default that's not one of the predefined HandlerContextKey
objects to avoid that.

I noticed this with a test of spectator 1.0.5 against a localstack container. Happy to add a test like that here is it's OK to add a test dependency on https://github.com/testcontainers/testcontainers-java. Sorry I didn't catch this earlier.

The aws sdk places a value in HandlerContextKey.OPERATION_NAME (e.g. GetObject) at least
some of the time, so use a default that's not one of the predefined HandlerContextKey
objects to avoid that.
@brharrington brharrington added this to the 1.0.6 milestone Nov 10, 2021
@brharrington
Copy link
Contributor

Happy to add a test like that here is it's OK to add a test dependency on https://github.com/testcontainers/testcontainers-java

I'm not familiar with this, the main thing I would want to make sure is that it is fairly lightweight, reliable, and wouldn't significantly increase testing times. It can be done as a follow up later if there is sufficient need or interest.

@brharrington brharrington merged commit aa9da40 into Netflix:master Nov 10, 2021
@dbyron-sf dbyron-sf deleted the change-default-handler-context-key branch November 10, 2021 02:50
@dbyron-sf
Copy link
Contributor Author

Happy to add a test like that here is it's OK to add a test dependency on https://github.com/testcontainers/testcontainers-java

I'm not familiar with this, the main thing I would want to make sure is that it is fairly lightweight, reliable, and wouldn't significantly increase testing times. It can be done as a follow up later if there is sufficient need or interest.

Yeah, it would for sure increase testing times...probably significantly...time to pull a docker image, and then run it.

@dbyron-sf
Copy link
Contributor Author

Thanks for merging this and releasing a new version so quickly!

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

Successfully merging this pull request may close these issues.

None yet

2 participants