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.

Sign up for GitHub

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

Jump to bottom

feat(gengapic): use gax.BuildHeaders and gax.InsertMetadataIntoOutgoingContext #1368

Merged
merged 5 commits into from
Jul 12, 2023
Merged

feat(gengapic): use gax.BuildHeaders and gax.InsertMetadataIntoOutgoingContext #1368

merged 5 commits into from
Jul 12, 2023

Conversation

quartzmo
Copy link
Member

@quartzmo quartzmo commented Jul 7, 2023 •
edited
Loading

closes: #1300
closes: #1301

@quartzmo quartzmo requested review from a team as code owners July 7, 2023 19:24
noahdietz
noahdietz reviewed Jul 7, 2023
View reviewed changes
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

I think you trying to do grpc in one PR and rest in another - i'd just do them both in the same PR

@quartzmo
Copy link
Member Author

quartzmo commented Jul 7, 2023

I think you trying to do grpc in one PR and rest in another - i'd just do them both in the same PR

I tried, but REST is much more involved. So I opened this PR for gRPC to get feedback. If it looks good, I can add the REST changes, and update the title, etc.

How does this look to you so far?

@noahdietz
Copy link
Collaborator

Ok let me take a deeper look, I just skimmed it before.

@noahdietz noahdietz self-requested a review July 7, 2023 21:00
@quartzmo
Copy link
Member Author

@noahdietz ping

noahdietz
noahdietz reviewed Jul 10, 2023
View reviewed changes
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

looking OK so far, yeah

go.sum Outdated Show resolved Hide resolved
showcase/go.sum Outdated Show resolved Hide resolved
@quartzmo
Copy link
Member Author

quartzmo commented Jul 10, 2023 •
edited
Loading

looking OK so far, yeah

Thanks, I'll go ahead with converting the REST usages. This will involve changing metadata.MD to []string in a number of places.

@quartzmo quartzmo changed the title feat(gengapic): use gax.InsertMetadataIntoOutgoingContext feat(gengapic): use gax.BuildHeaders and gax.InsertMetadataIntoOutgoingContext Jul 11, 2023
@noahdietz noahdietz self-requested a review July 11, 2023 21:44
noahdietz
noahdietz reviewed Jul 11, 2023
View reviewed changes
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

LGTM just one question that isn't really blocking

internal/gengapic/gengapic.go Show resolved Hide resolved
@quartzmo quartzmo merged commit 6f782f9 into googleapis:main Jul 12, 2023
@quartzmo quartzmo deleted the gax-headers branch July 12, 2023 17:37
@release-please release-please bot mentioned this pull request Jul 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codyoss codyoss codyoss left review comments

@tritone tritone tritone left review comments

@noahdietz noahdietz noahdietz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gapic: move buildHeaders helper into gax-go gapic: move insertMetadata helper into gax-go
4 participants
@quartzmo @noahdietz @tritone @codyoss