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

fix: set projectId to a default value in emulator case #1345

Closed
wants to merge 5 commits into from

Conversation

dmivankov
Copy link

@dmivankov dmivankov commented Aug 12, 2021

Spanner client wants to have a projectId option set, by default it also tries to pick it up from lots of places
For emulator case none of those may be present as it could be that neither gcloud nor default application credentials are configured, but emulator is accessible nevertheless.

Set default project id in SpannerOptions, value doesn't matter so much as emulator would be happy with any value it seems.
Note that alternatively it could be improved in base ServiceOptions, but not sure about assumptions there.

failure example in case where emulatorHost is set without projectId

  java.lang.IllegalArgumentException: A project ID is required for this service but could not be determined from the builder or the environment.  Please set a project ID using the builder.
  at com.google.common.base.Preconditions.checkArgument(Preconditions.java:142)
  at com.google.cloud.ServiceOptions.<init>(ServiceOptions.java:304)
  at com.google.cloud.spanner.SpannerOptions.<init>(SpannerOptions.java:533)
  at com.google.cloud.spanner.SpannerOptions.<init>(SpannerOptions.java:77)
  at com.google.cloud.spanner.SpannerOptions$Builder.build(SpannerOptions.java:1029)

Note that in non-emulator case one usually doesn't need to set projectId as it could be extracted from credentials

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

Spanner client wants to have a projectId option set, by default it also tries to pick it up from [lots of places](https://github.com/googleapis/java-core/blob/375983090b3700b3fb6a1953626db7efca49cc51/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java#L372)
For emulator case none of those may be present as it could be that neither gcloud nor default application credentials are configured, but emulator is accessible nevertheless.

Set default project id in SpannerOptions, value doesn't matter so much as emulator would be happy with any value it seems.
Note that alternatively it could be improved in base ServiceOptions, but not sure about assumptions there.
@dmivankov dmivankov requested a review from as a code owner Aug 12, 2021
@product-auto-label product-auto-label bot added the api: spanner label Aug 12, 2021
@google-cla google-cla bot added the cla: yes label Aug 12, 2021
@dmivankov dmivankov changed the title Set projectId to a default value in emulator case fix: set projectId to a default value in emulator case Aug 12, 2021
Copy link
Contributor

@olavloite olavloite left a comment

Hi @dmivankov Thanks for your contribution. I think this is a good idea. I've left a couple of textual suggestions.
Also, there seems to be a compile error. Would you mind take a look at that?

Also, would you be able to add a test that verifies that it will actually use this, and that it does not fail before it gets this far? If not, then that's OK as well. I can take a look at a later moment. It might be quite tricky as such a test would depend on a lot of different environment variables.

@thiagotnunes Do you have any additional comments on this?

thiagotnunes and others added 2 commits Aug 13, 2021
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

There is a compilation error, but LGTM.

After fixing that we should be good to go.

@@ -1134,6 +1134,11 @@ public SpannerOptions build() {
this.setChannelConfigurator(ManagedChannelBuilder::usePlaintext);
// As we are using plain text, we should never send any credentials.
this.setCredentials(NoCredentials.getInstance());
// Default project id resolution might fail, but the emulator accepts any
// project id, so we can safely use a dummy id.
if (this.getProjectId() == null) {
Copy link
Author

@dmivankov dmivankov Aug 13, 2021

Choose a reason for hiding this comment

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

olavloite added a commit that referenced this issue Aug 19, 2021
Use a dummy emulator-project when no default project is set for the environment
and the SPANNER_EMULATOR_HOST environment variable has been set.

Replaces #1345
@olavloite
Copy link
Contributor

@olavloite olavloite commented Aug 19, 2021

@dmivankov I've created a new PR that uses a slightly different approach to achieve the same goal: #1363

thiagotnunes pushed a commit that referenced this issue Aug 19, 2021
Use a dummy emulator-project when no default project is set for the environment
and the SPANNER_EMULATOR_HOST environment variable has been set.

Replaces #1345
@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Aug 19, 2021

Closed in favour of #1363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants