-
Notifications
You must be signed in to change notification settings - Fork 123
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
feat: Track PG Adapter usage from user-agent headers #1711
feat: Track PG Adapter usage from user-agent headers #1711
Conversation
…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.
There was a problem hiding this 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.
.setCredentials(NoCredentials.getInstance()) | ||
.setClientLibToken(pgAdapterToken) | ||
.build(); | ||
assertThat(options.getClientLibToken()).isEqualTo(pgAdapterToken); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@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? |
@olavloite it does not run by default from forks, we just need to add the label |
…vsnj/java-spanner into pg-adapter-header-addition
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