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

chore: allow custom routing header metadata #2079

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Jul 27, 2024

Avoid setting the routing metadata field (x-goog-request-params) if it already exists in the passed in metadata, to prevent duplicate routing headers

Fixes #2078

@daniel-sanche daniel-sanche requested a review from a team as a code owner July 27, 2024 00:31
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Jul 27, 2024
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: s Pull request size is small. labels Jul 27, 2024
@parthea parthea assigned daniel-sanche and unassigned parthea Jul 29, 2024
@parthea parthea marked this pull request as draft July 29, 2024 14:38
)),
)
metadata = tuple(metadata)
if all(m[0] != gapic_v1.routing_header.ROUTING_METADATA_KEY for m in metadata):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can you add comments to clarify the reason for the additional logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the comment above this block to make it more clear, let me know what you think

@daniel-sanche
Copy link
Contributor Author

@parthea I'm a little unsure about the failing showcase tests. Looking at the method names, it seems to line up with the methods in _test_mixins. But I don't fully understand that file.

Do I need to duplicate my new tests for each method in _test_mixins by hand? Why is that needed on top of the templates I already added to test_macros?

@daniel-sanche daniel-sanche added the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 2, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 2, 2024
@daniel-sanche daniel-sanche marked this pull request as ready for review August 5, 2024 17:46
@daniel-sanche
Copy link
Contributor Author

Unrelated to this change, a customer reported a separate issue with routing for the async client. We may want to address this at the same time? #2091

@daniel-sanche
Copy link
Contributor Author

Blocked on #2133. I will update this after those changes are merged

@daniel-sanche
Copy link
Contributor Author

daniel-sanche commented Sep 19, 2024

From #2133 (comment):

For #2078 we need to update AIPs first so there is consistent behavior across all language-specific generators: https://google.aip.dev/client-libraries/4222

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support customizing routing metadata
2 participants