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: Track PG Adapter usage from user-agent headers #1711

Merged
merged 6 commits into from
Feb 28, 2022
Merged

feat: Track PG Adapter usage from user-agent headers #1711

merged 6 commits into from
Feb 28, 2022

Conversation

gauravsnj
Copy link
Contributor

@gauravsnj gauravsnj commented Feb 23, 2022 •
edited by olavloite
Loading

Added the string "pg-adapter" in the list of allowed client library token, so that when PG Adapter connects via client library by passing the string "pg-adapter" in the user-agent header, it doesn't get discarded. Also, added the one test case to cover the same.

Fixes b/213946108

@gauravsnj
Added the string "pg-adapter" in the list of allowed client library t…
1bc6074 
…oken, so that when PG Adapter connects via client library by passing the string "pg-adapter" in the user-agent header, it doesn't get discarded.
@gauravsnj
Added an additional test case for the testSetClientLibToken test.
dab9fce
@gauravsnj gauravsnj requested a review from a team as a code owner February 23, 2022 15:12
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Feb 23, 2022
@gauravsnj gauravsnj changed the title PGAdapter - Track adapter usage from user-agent headers feat: Track PG Adapter usage from user-agent headers Feb 23, 2022
olavloite
olavloite reviewed Feb 23, 2022
View reviewed changes
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

LGTM, with a small nit on the assertion method that is used. I'm going to hold off final approval to give @ansh0l a chance to put in a review as well.

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java Outdated
.setCredentials(NoCredentials.getInstance())
.setClientLibToken(pgAdapterToken)
.build();
assertThat(options.getClientLibToken()).isEqualTo(pgAdapterToken);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we prefer the use of assertEquals(expected, actual) (add import static org.junit.Assert.assertEquals at the top of the file to import it)

I know that it is not consistent with the other tests here, but the general rule of thumb is to use this method for new code, even when it is different from the surrounding code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @olavloite for mentioning this. I've changed it and will remember this in future

@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 23, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 23, 2022
ansh0l
ansh0l approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@ansh0l ansh0l left a comment

Choose a reason for hiding this comment

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

LGTM

@olavloite
Copy link
Collaborator

@ansh0l Do you know why the OwlBot check seems to be hanging? Is that because the PR has been created from a fork instead of a branch directly in the repository? And if so, is that something you can override for this PR?

@thiagotnunes
Copy link
Contributor

@olavloite it does not run by default from forks, we just need to add the label owlbot:run

@thiagotnunes thiagotnunes added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 24, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 24, 2022
@gauravsnj gauravsnj requested a review from a team as a code owner February 24, 2022 21:46
@gauravsnj gauravsnj added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Feb 25, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 25, 2022
@gauravsnj gauravsnj added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 25, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 27, 2022
@olavloite olavloite merged commit cb640ab into googleapis:main Feb 28, 2022
@release-please release-please bot mentioned this pull request Feb 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olavloite olavloite olavloite approved these changes

@ansh0l ansh0l ansh0l approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/java-spanner API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gauravsnj @olavloite @thiagotnunes @ansh0l @yoshi-kokoro