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

[gax-php] feat: add REST headers #322

Merged
merged 8 commits into from May 5, 2021
Merged

[gax-php] feat: add REST headers #322

merged 8 commits into from May 5, 2021

Conversation

miraleung
Copy link
Collaborator

This is needed primarily for REGAPIC. However, sending the REST header can also benefit gRPC and REST GAPIC libraries (the default) as well, since we don't support gRPC-only libraries.

Using gax-php's version as the default REST version.

@miraleung miraleung requested a review from a team as a code owner May 3, 2021 21:20
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 3, 2021
@miraleung miraleung requested a review from bshaffer May 3, 2021 21:22
@@ -64,6 +64,7 @@ public static function buildAgentHeader($headerInfo)
// - gapicVersion (gapic/)
// - apiCoreVersion (gax/)
// - grpcVersion (grpc/)
// - restVersion (rest/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to add this to the $headerInfo phpdoc for the method!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

// a REST-only library, then the grpcVersion header should not be set.
if ($this->transport instanceof GrpcFallbackTransport) {
$options['grpcVersion'] = phpversion('grpc');
$options['restVersion'] = Version::getApiCoreVersion();
Copy link
Contributor

Choose a reason for hiding this comment

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

As GrpcFallbackTransport doesn't actually use the grpc extension (or grpc), I'm not sure we want to set the grpc/ header, as it's most likely going to be empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

src/AgentHeader.php Outdated Show resolved Hide resolved
src/AgentHeader.php Show resolved Hide resolved
Co-authored-by: David Supplee <dwsupplee@gmail.com>
@miraleung miraleung merged commit 2d75503 into master May 5, 2021
@miraleung miraleung deleted the dev/diregapic branch May 5, 2021 17:52
@bshaffer bshaffer mentioned this pull request Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants