Skip to content

Conversation

tconley1428
Copy link
Contributor

What was changed

Why?

Checklist

  1. Closes

  2. How was this tested:

  1. Any docs updates needed?

@tconley1428 tconley1428 requested a review from a team as a code owner September 5, 2025 23:00
Copy link
Contributor

@jssmith jssmith left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I'm looking over the rest of the plugin again and wondering whether we should add checks for data converter collisions. This implementation is fine so long as it is the first to set the data converter. Can we enforce that / would it make sense to do so?

@cretz
Copy link
Member

cretz commented Sep 8, 2025

This implementation is fine so long as it is the first to set the data converter. Can we enforce that / would it make sense to do so?

Yes, there is a bug right now where this plugin is clobbering a user payload codec (and failure converter, though those are rare to customize). We should take the given data converter (or default) and replace just the payload converter on it. I have opened #1084. And yes, we should consider erroring if there is already a non-default payload converter on their data converter.

@tconley1428 tconley1428 merged commit 9372d47 into main Sep 8, 2025
16 checks passed
@tconley1428 tconley1428 deleted the openai/replayer_configure branch September 8, 2025 15:54
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