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

Raise a helpful error if projectID is missing when making a request? #570

Closed
jgeewax opened this issue May 9, 2015 · 19 comments · Fixed by #639
Closed

Raise a helpful error if projectID is missing when making a request? #570

jgeewax opened this issue May 9, 2015 · 19 comments · Fixed by #639
Assignees
Labels
api: datastore Issues related to the Datastore API.

Comments

@jgeewax
Copy link
Contributor

jgeewax commented May 9, 2015

Right now, if I run the following code on an unconfigured machine, I get a scary back-end error...

// (We're not running on App Engine or Compute Engine).
var gcloud = require('gcloud');
var dataset = gcloud.datastore.dataset();  // <-- Here is the issue (projectId === undefined)

dataset.get(datastore.key(['Person', 1], function(result) {
  console.log(result);
});

Seems like if we can't auto-discover the project ID or credentials, we fail with a very long error.

Could we update things so that we raise a specific exception saying "You tried to make a request with a null project ID, meaning we couldn't figure out the project ID and you also didn't tell us the project ID..."

@jgeewax jgeewax added enhancement api: datastore Issues related to the Datastore API. labels May 9, 2015
@jgeewax jgeewax added this to the Datastore Future milestone May 9, 2015
@jgeewax
Copy link
Contributor Author

jgeewax commented May 9, 2015

It might also be useful to automatically pull the project ID from the one set in the Cloud SDK (see ~/.config/gcloud/properties which sometimes has a default project ID in config-file format). This is where the Cloud SDK stores the result of gcloud config set project <projectId> -- right @rdayal?

@stephenplusplus
Copy link
Contributor

I think it would be nice to fail sooner at the var dataset = gcloud.datastore.dataset() point, because by then, we'll know "whatever API request you try, it ain't gonna work."

@jgeewax
Copy link
Contributor Author

jgeewax commented May 14, 2015

OK so you're saying at the time you create a Dataset, if we couldn't figure out the project ID by then, we should raise an exception ? SGTM.

@stephenplusplus
Copy link
Contributor

The actual API call that was made in your example was:

{ method: 'POST',
  uri: 'https://www.googleapis.com/datastore/v1beta2/datasets/{projectId}/lookup',
  headers: { 'Content-Type': 'application/x-protobuf' } }

(note the {projectId})

Even if you ran it in an environment where google-auth-library could have detected the projectId and credentials, it still would fail because that placeholder was never replaced.

Before I figure out the right way to solve this, I want to see if we should stand by this: our docs say a projectId is always required. The project id can be detected by google-auth-library, but we still require it in our library to form API calls to the right path, among various other places.

Do we want to push for a completely projectId-configuration-free experience? I can try to extract the logic from the auth library, or send a PR to expose it.

However, if we're okay to require projectId, then I'll just throw if we don't get one or one wasn't inherited.

@jgeewax
Copy link
Contributor Author

jgeewax commented May 14, 2015

Hm, this might be a good time to clarify the whole project ID / dataset ID thing...

In the future, each project will be able to have multiple dataset IDs (we'll likely call them databases). If you "leave it out" we should just substitute the default Dataset ID (which happens to be the project ID).

If we can guess the project ID, I'd really like that... So, some code...

var gcloud = require('gcloud');
// project ID is either null or auto-detected...
// null is OK here, we could still set it further down the line...

var dataset = gcloud.datastore.dataset();
// By this point we should have a dataset ID == project ID.
// If that's not auto-detected or set by now, we should holler.

If we wanted to explore the idea of being able to configure datastore (which I personally like but, as @ryanseys says, it's a breaking change...), this might be....

var gcloud = require('gcloud');
// Auto-detect a project ID, or leave as null.
// null is OK here, we could still set it further down the line...

var datastore = gcloud.datastore();
// pull the ID out of gcloud, or let users specify which ID it is.
// If there's no ID by this point, we should holler, as I don't think there's anything you can do...

var dataset = datastore.dataset();
// dataset ID should be datastore's project ID or specified by the user.
// No need to holler here as that already happened above.

@stephenplusplus
Copy link
Contributor

The inheritance of a project ID from a datastore to a child dataset makes sense to me.

If we can rely on env vars to tell us if we're running on G[AC]E, then this will be super easy. If anything is async, it's terribly difficult.

  • GAE Dev: No magic.
  • GAE Prod: Env var name: GAE_LONG_APP_ID
  • GCE: ??

How google-auth-library checks if the user is running on GCE: https://github.com/google/google-auth-library-nodejs/blob/ce7f97409908856e2386a650e2fa4de6b2a8211a/lib/auth/googleauth.js#L158

There's a similar endpoint where we can get the projectId. The problem is that it's an async call, which complicates things.

  1. We wouldn't want to do the async call on require('gcloud'), since the user hasn't asked us to do anything yet.
  2. We can't do it at var datastore = gcloud.datastore();, as this makes a formerly non-async call have async side effects.
  3. We have to do it the last possible chance, for example, on datastore.get(). Our code would have to be re-worked quite a bit to inject the new logic (if necessary) before all API calls. And even then, throwing async exceptions aren't ideal (which we would have to do if we couldn't find a projectId).

Summing all of that up, I propose we continue to require a projectId:

var gcloud = require('gcloud');
var dataset = gcloud.datastore.dataset(); // throws.
var dataset = gcloud.datastore.dataset({ projectId: 'asdf' });

@jgeewax
Copy link
Contributor Author

jgeewax commented May 14, 2015

Is this where promises could come in handy? Have a promise for the project ID and evaluate it when needed ?

@ryanseys
Copy link
Contributor

For Google Compute Engine Linux instances that are "v20131120 or newer" we can run:

$ sudo dmidecode -s bios-vendor | grep Google

on the instance and it should output:

Google

Source: https://cloud.google.com/compute/docs/instances#dmi

@jgeewax
Copy link
Contributor Author

jgeewax commented May 18, 2015

Yikes -- sudo required? :-/

@ryanseys
Copy link
Contributor

I tried it without sudo and got command not found so yeah, probably. :(

@ryanseys
Copy link
Contributor

Makes sense to require a projectId. If they want a "configuration free" experience, they can put projectId: GCLOUD_PROJECT_ID in their code, and just make sure their instances have that env var configured always. I don't think this is asking too much. The other advantage to this is there is no surprises as to what project is used when they do run their code. If they forget to specify it, they will get an error. Sounds reasonable to me.

@ludoch
Copy link

ludoch commented May 20, 2015

regarding autodetect, ~/.config/gcloud/properties is only for some Linux
systems...For ex, will not work in devshell, a GCE jenkins instance via
Bitnami and or Windows 32 and Windows 64 bits...
Most/all of them have different ways of find this info if it exists...
Ugly code for now, but this gives you some implementation in Java:
See setupInitialCommands in
https://github.com/ludoch/gcloud-maven-plugin/blob/master/src/main/java/com/google/appengine/gcloudapp/AbstractGcloudMojo.java

On Tue, May 19, 2015 at 1:50 PM, Ryan Seys notifications@github.com wrote:

Makes sense to require a projectId. If they want a "configuration free"
experience, they can put projectId: GCLOUD_PROJECT_ID in their code, and
just make sure their instances have that env var configured always. I don't
think this is asking too much. The other advantage to this is there is
no surprises as to what project is used when they do run their code. If
they forget to specify it, they will get an error. Sounds reasonable to me.


Reply to this email directly or view it on GitHub
#570 (comment)
.

@ludoch
Copy link

ludoch commented May 20, 2015

And getAppId() in this file...

On Tue, May 19, 2015 at 5:33 PM, Ludovic Champenois <
ludovic.champenois@gmail.com> wrote:

regarding autodetect, ~/.config/gcloud/properties is only for some Linux
systems...For ex, will not work in devshell, a GCE jenkins instance via
Bitnami and or Windows 32 and Windows 64 bits...
Most/all of them have different ways of find this info if it exists...
Ugly code for now, but this gives you some implementation in Java:
See setupInitialCommands in
https://github.com/ludoch/gcloud-maven-plugin/blob/master/src/main/java/com/google/appengine/gcloudapp/AbstractGcloudMojo.java

On Tue, May 19, 2015 at 1:50 PM, Ryan Seys notifications@github.com
wrote:

Makes sense to require a projectId. If they want a "configuration free"
experience, they can put projectId: GCLOUD_PROJECT_ID in their code, and
just make sure their instances have that env var configured always. I don't
think this is asking too much. The other advantage to this is there is
no surprises as to what project is used when they do run their code.
If they forget to specify it, they will get an error. Sounds reasonable to
me.


Reply to this email directly or view it on GitHub
#570 (comment)
.

@jgeewax
Copy link
Contributor Author

jgeewax commented May 20, 2015

The use case I'm thinking of here is "hey just take this code and run it in your environment, no changes needed, everything will just work". But I can understand if that's either a) too magical , or b) not feasible.

@ryanseys
Copy link
Contributor

Yeah I mean, we could do it easily synchronously but it would block the event loop and doing it asynchronously doesn't make sense for some of our use cases, meaning we would have to callback errors at the latest possible moment (once we've made a http request or two to determine they aren't on GCE and can't hit a metadata server). I believe the trade-off isn't worth it and the magic is too much. Let's remember that Node.js isn't really about magic. That's some of the appeal of the language platform ( 😨 node isn't a language).

I vote that we document everywhere that we need them to give us a project ID and if they don't, we throw an error. I think it's simple and understandable and no magic.

@jgeewax
Copy link
Contributor Author

jgeewax commented May 21, 2015

I vote that we document everywhere that we need them to give us a project ID and if they don't, we throw an error.

So we're officially saying that the following tweak is completely unacceptable?

"I vote that we document everywhere that we need them to give us a project ID and if they don't, we try to figure it out. If we can't, we throw an error."

@stephenplusplus
Copy link
Contributor

I still say throw if it's not given. @ryanseys has the best solution:

If they want a "configuration free" experience, they can put projectId: process.env.GCLOUD_PROJECT_ID in their code, and just make sure their instances have that env var configured always.

IMO, that's actually better than GCE auto-detecting your projectId, since before I push to GCE, I have to manually specify my projectId if I wish to test my project. So it would actually be an inconvenience to remove what I was doing before I push to prod.

@jgeewax
Copy link
Contributor Author

jgeewax commented May 21, 2015

OK cool - could we put that in the docs somewhere that that's the recommended way to deal with that?

@ryanseys
Copy link
Contributor

Sure, I'll add some docs for it.

@ryanseys ryanseys self-assigned this May 21, 2015
chingor13 pushed a commit that referenced this issue Aug 22, 2022
🤖 I have created a release \*beep\* \*boop\*
---
## [3.20.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.19.0...v3.20.0) (2021-10-19)


### Features

* Update osconfig v1 and v1alpha RecurringSchedule.Frequency with DAILY frequency ([#569](https://www.github.com/googleapis/nodejs-asset/issues/569)) ([af03fd5](https://www.github.com/googleapis/nodejs-asset/commit/af03fd5c4fba4a258acf4c0332991bcb619fa10b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
chingor13 pushed a commit that referenced this issue Aug 22, 2022
🤖 I have created a release \*beep\* \*boop\*
---
## [3.20.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.19.0...v3.20.0) (2021-10-19)


### Features

* Update osconfig v1 and v1alpha RecurringSchedule.Frequency with DAILY frequency ([#569](https://www.github.com/googleapis/nodejs-asset/issues/569)) ([af03fd5](https://www.github.com/googleapis/nodejs-asset/commit/af03fd5c4fba4a258acf4c0332991bcb619fa10b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
chingor13 pushed a commit that referenced this issue Aug 22, 2022
🤖 I have created a release \*beep\* \*boop\*
---
## [3.20.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.19.0...v3.20.0) (2021-10-19)


### Features

* Update osconfig v1 and v1alpha RecurringSchedule.Frequency with DAILY frequency ([#569](https://www.github.com/googleapis/nodejs-asset/issues/569)) ([af03fd5](https://www.github.com/googleapis/nodejs-asset/commit/af03fd5c4fba4a258acf4c0332991bcb619fa10b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this issue Sep 15, 2022
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [jsdoc-fresh](https://github.com/googleapis/jsdoc-fresh) | [`^1.0.1` -> `^2.0.0`](https://renovatebot.com/diffs/npm/jsdoc-fresh/1.1.1/2.0.0) | [![age](https://badges.renovateapi.com/packages/npm/jsdoc-fresh/2.0.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/jsdoc-fresh/2.0.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/jsdoc-fresh/2.0.0/compatibility-slim/1.1.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/jsdoc-fresh/2.0.0/confidence-slim/1.1.1)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>googleapis/jsdoc-fresh</summary>

### [`v2.0.0`](https://github.com/googleapis/jsdoc-fresh/blob/HEAD/CHANGELOG.md#&#8203;200-httpsgithubcomgoogleapisjsdoc-freshcomparev111v200-2022-05-18)

[Compare Source](https://github.com/googleapis/jsdoc-fresh/compare/v1.1.1...v2.0.0)

##### ⚠ BREAKING CHANGES

-   update library to use Node 12 ([#&#8203;108](https://github.com/googleapis/jsdoc-fresh/issues/108))

##### Build System

-   update library to use Node 12 ([#&#8203;108](https://github.com/googleapis/jsdoc-fresh/issues/108)) ([e61c223](https://github.com/googleapis/jsdoc-fresh/commit/e61c2238db8900e339e5fe7fb8aea09642290182))

##### [1.1.1](https://www.github.com/googleapis/jsdoc-fresh/compare/v1.1.0...v1.1.1) (2021-08-11)

##### Bug Fixes

-   **build:** migrate to using main branch ([#&#8203;83](https://www.github.com/googleapis/jsdoc-fresh/issues/83)) ([9474adb](https://www.github.com/googleapis/jsdoc-fresh/commit/9474adbf0d559d319ff207397ba2be6b557999ac))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-redis).
sofisl pushed a commit that referenced this issue Oct 11, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Oct 13, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Nov 10, 2022
…#570)

This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/b205fd33-200c-4298-88b8-18b0d1c79a3e/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@5936421
Source-Link: googleapis/synthtool@89d431f
sofisl pushed a commit that referenced this issue Nov 11, 2022
#570)

- [ ] Regenerate this pull request now.

Committer: @Padmaar
PiperOrigin-RevId: 429111624

Source-Link: googleapis/googleapis@da999a2

Source-Link: googleapis/googleapis-gen@99c5b3e
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiOTljNWIzZTk4YmFhMWRlOTM3NzZhYTRiNWNkNGM3MzYxMzUzZTRmNiJ9
sofisl pushed a commit that referenced this issue Nov 11, 2022
sofisl pushed a commit that referenced this issue Nov 11, 2022
🤖 I have created a release \*beep\* \*boop\*
---
## [3.20.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.19.0...v3.20.0) (2021-10-19)


### Features

* Update osconfig v1 and v1alpha RecurringSchedule.Frequency with DAILY frequency ([#569](https://www.github.com/googleapis/nodejs-asset/issues/569)) ([af03fd5](https://www.github.com/googleapis/nodejs-asset/commit/af03fd5c4fba4a258acf4c0332991bcb619fa10b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this issue Nov 11, 2022
- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 470911839

Source-Link: googleapis/googleapis@3527566

Source-Link: googleapis/googleapis-gen@f16a1d2
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZjE2YTFkMjI0ZjAwYTYzMGVhNDNkNmE5YTFhMzFmNTY2ZjQ1Y2RlYSJ9

feat: accept google-gax instance as a parameter
Please see the documentation of the client constructor for details.

PiperOrigin-RevId: 470332808

Source-Link: googleapis/googleapis@d4a2367

Source-Link: googleapis/googleapis-gen@e97a1ac
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZTk3YTFhYzIwNGVhZDRmZTczNDFmOTFlNzJkYjdjNmFjNjAxNjM0MSJ9
sofisl pushed a commit that referenced this issue Jan 17, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@types/sinon](https://github.com/DefinitelyTyped/DefinitelyTyped) | devDependencies | major | [`^7.5.1` -> `^9.0.0`](https://renovatebot.com/diffs/npm/@types%2fsinon/7.5.2/9.0.0) |

---

### Renovate configuration

:date: **Schedule**: "after 9am and before 3pm" (UTC).

:vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

:recycle: **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-vision).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants