fix: read projectId from retailClient.getProjectId#159
fix: read projectId from retailClient.getProjectId#159bcoe merged 10 commits intogoogleapis:mainfrom
Conversation
| const {ProductServiceClient} = require('@google-cloud/retail').v2; | ||
| const retailClient = new ProductServiceClient(); | ||
|
|
||
| const projectId = await retailClient.getProjectId(); |
There was a problem hiding this comment.
This looks a bit hacky. Any clarification why we cannot use the environment variable?
There was a problem hiding this comment.
It was requested to get rid off using the environment variable to store the project id, as it is set in the gcloud config. If it is not the right way how to get the project id, could you please help to do it right?
There was a problem hiding this comment.
So for the actual tutorial, I would recommend forcing users to change it, i.e.:
/**
* TODO(developer): Uncomment these variables before running the sample.
*/
// const arg1 = 'default_value_one';
// const arg2 = 1234;
Then in the test, you can pull it up like in line 24.
If it's absolutely necessary for your tutorial to use env variables, you can do so, and then in the test make sure to set it, i.e., process.env.PROJECT_ID = await retailClient.getProjectId()
The goals are twofold: 1) By not having env variables, people know exactly which project and therefore permissions they are using, and 2) when we run tests locally, tests pass without having to search for hidden env variables.
There was a problem hiding this comment.
In some tutorials we ask user to run several samples sequentially, also we expect that users could run one tutorial after another to learn about all Retail search(as an example) features. In such case I think it would be more convenient if user don't need to set the project id each time. Besides, to request the Retail API the google application credentials should be used, SO user first step is to set the 'gcloud config set project project_id'.
As we already have the project id set in the gcloud environment, we can read it and use in each of the code samples.
I really not sure will it be handy if we will ask user to set the gcloud variable and then to set the same value in the code samples.
There was a problem hiding this comment.
In that case, I think we should make sure to set it like you have it now in the test at the very least. At the very least, tests should be able to pass locally without people having to set an env variable.
There was a problem hiding this comment.
to run the tests locally, the user needs to set the project id of his/her own project eather in each test or in some env variable.
Regarding using the env variables in tests, please take a look at thes test:
Here the bucket name is generated in the test,
so the env variable as a name of a bucket (project_id+timestamp) will be used only by a user while he/she going through the tutorial, not in the tests.
Is that is what you are concerned about here? Or I didn't understand you correctly
There was a problem hiding this comment.
Sorry, to put it simply, I recommend doing this for the sample:
//TODO(developer): uncomment and replace with your own variable.
const projectId = 'my-project'
then, in the test, using this:
const projectId = await retailClient.getProjectId();
I would strongly discourage the use of env variables. If you have to use them in the tutorial, then I'd request that in the test, you set it explicitly, i.e.,
process.env.PROJECT_ID = 'long-door-651'
or
process.env.PROJECT_ID = await retailClient.getProjectId()
Here is an example of how our other samples use these variables, sample and test.
Ultimately, since I'm not familiar with the context in which these samples will be shown online, I won't push for anything too strongly; the only thing I would request is that for tests, you are not grabbing an environment variable for the project Id, instead you are using this:
const projectId = await retailClient.getProjectId();
There was a problem hiding this comment.
in all of our tests we already read the projectId like this (already merged in googleapis repo):
let projectId
before(async () => {
projectId = await retailClient.getProjectId();
In this PR we fix one code sample - create-gcs-bucket.js - where this value still was read from env variable.
And we don't need the user to set the projectId in each of code sample, as we can read it in the same way : projectId = await retailClient.getProjectId();
To run the samples the user must set the projectId in the CloudShell, using gcloud command, in order to be authorized (get the service account key) to his own project. So we already have this value and I want to emphasize that it was explicitly requested by our product owner - Riccardo Patana not to ask user to set this value in each code sample, as it is expected the user to go through several tutorials (run several code samples) in one session.
We do not use the env variables in the code samples to store the projectId
Small fix in the delete_product.js
fix: update the bash scripts
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:
Fixes #<issue_number_goes_here> 🦕