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

core: read projectId from authClient #1656

Merged
merged 5 commits into from
Dec 2, 2016

Conversation

stephenplusplus
Copy link
Contributor

@stephenplusplus stephenplusplus commented Sep 30, 2016

Fixes #1653

To Dos

This 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:

// Previously:
var bigquery = require('@google-cloud/bigquery')({ no projectId provided })
// throws error

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.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 30, 2016
@stephenplusplus stephenplusplus added status: blocked Resolving the issue is dependent on other work. don't merge labels Sep 30, 2016
@stephenplusplus stephenplusplus changed the title core: read projectId from SDK config file core: read projectId from authClient Oct 3, 2016
@stephenplusplus stephenplusplus removed the status: blocked Resolving the issue is dependent on other work. label Oct 19, 2016
@stephenplusplus
Copy link
Contributor Author

@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.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@stephenplusplus stephenplusplus added the status: blocked Resolving the issue is dependent on other work. label Nov 14, 2016
@stephenplusplus stephenplusplus removed the status: blocked Resolving the issue is dependent on other work. label Nov 15, 2016
@stephenplusplus
Copy link
Contributor Author

@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 google-auth-library which supports automatic project ID detection in DAC environments. Our code won't have to change to compensate.

Currently, the only way to see that this PR works is by providing a JSON key file as the env var GCLOUD_TESTS_KEY and unsetting the process.env.GCLOUD_TESTS_PROJECT_ID 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.

This comment was marked as spam.

@stephenplusplus
Copy link
Contributor Author

@callmehiphop Tests have been added-- PTAL!

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 84317cb on stephenplusplus:spp--1653 into ** on GoogleCloudPlatform:master**.

@callmehiphop
Copy link
Contributor

Can we update the google-cloud system tests to conditionally run if and when the environment variables are set?

@stephenplusplus
Copy link
Contributor Author

stephenplusplus commented Nov 29, 2016

Good point. I ended up removing the system-test directory altogether. The services' system tests themselves will fail as they should when the environment lacks an appropriate level of authentication.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 8684e17 on stephenplusplus:spp--1653 into ** on GoogleCloudPlatform:master**.

@stephenplusplus stephenplusplus merged commit f396c9f into googleapis:master Dec 2, 2016
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. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants