-
Notifications
You must be signed in to change notification settings - Fork 591
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
core: read projectId from authClient #1656
Conversation
4a97fb4
to
58d20f8
Compare
b56134d
to
4a4a154
Compare
@callmehiphop I thought I tagged you in this for a review, but I can't find it. Could you take a dive into this one? The architecture here is difficult (see first post), so I'm very open to better solutions :) |
this.authClient = this.makeAuthenticatedRequest.authClient; | ||
this.baseUrl = config.baseUrl; | ||
this.getCredentials = this.makeAuthenticatedRequest.getCredentials; | ||
this.globalInterceptors = arrify(options.interceptors_); | ||
this.interceptors = []; | ||
this.packageJson = config.packageJson; | ||
this.projectId = options.projectId; | ||
this.projectId = options.projectId || '{{projectId}}'; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
902f9e7
to
09aeac1
Compare
@callmehiphop please take another look before I get into the tests. I've updated the docs, which is a good place to start for the review, since it will describe the expectations of this PR. However, we still need a release of Currently, the only way to see that this PR works is by providing a JSON key file as the env var |
try { | ||
reqOpts = util.decorateRequest(reqOpts, { projectId: self.projectId }); | ||
} catch (e) { | ||
stream.destroy(e); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
71f2ca3
to
84317cb
Compare
@callmehiphop Tests have been added-- PTAL! |
Changes Unknown when pulling 84317cb on stephenplusplus:spp--1653 into ** on GoogleCloudPlatform:master**. |
Can we update the google-cloud system tests to conditionally run if and when the environment variables are set? |
84317cb
to
8684e17
Compare
Good point. I ended up removing the |
Changes Unknown when pulling 8684e17 on stephenplusplus:spp--1653 into ** on GoogleCloudPlatform:master**. |
Fixes #1653
To Dos
fromJSON
with a JSON key so the project ID is read by google-auth-library stephenplusplus/google-auto-auth#7This PR assumes
google-auth-library
will auto-detect a projectId when available and make it available as a property on the authClient instance:authClient.projectId
.This PR makes it so we won't require a projectId to be specified at the time of instantiation of a class:
Instead, it will internally default the
projectId
to{{projectId}}
. This token is then looked for when a request is made, and replaced with the value we get from the google-auth-library authClient.In the case a projectId was not found by google-auth-library, the user's callback will be executed with the same error that we previously threw.
This change is complex due to the nature of async vs. sync expectations. Previously, we were able to do everything synchronously at the time of instantiation. Requiring the project ID made making API requests simple, because it's frequently required as part of the URI that we send requests to. In the case of gRPC, it's commonly used in the request option/arguments to a service.