Skip to content
This repository was archived by the owner on Jul 20, 2023. It is now read-only.

fix: read projectId from retailClient.getProjectId#159

Merged
bcoe merged 10 commits intogoogleapis:mainfrom
t-karasova:main
Apr 13, 2022
Merged

fix: read projectId from retailClient.getProjectId#159
bcoe merged 10 commits intogoogleapis:mainfrom
t-karasova:main

Conversation

@t-karasova
Copy link
Copy Markdown
Contributor

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> 🦕

@t-karasova t-karasova requested review from a team March 24, 2022 22:45
@product-auto-label product-auto-label Bot added the api: retail Issues related to the googleapis/nodejs-retail API. label Mar 24, 2022
@nd-zh nd-zh self-assigned this Mar 24, 2022
const {ProductServiceClient} = require('@google-cloud/retail').v2;
const retailClient = new ProductServiceClient();

const projectId = await retailClient.getProjectId();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks a bit hacky. Any clarification why we cannot use the environment variable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@sofisl sofisl Mar 30, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@t-karasova t-karasova Apr 1, 2022

Choose a reason for hiding this comment

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

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:

https://github.com/googleapis/nodejs-retail/blob/main/samples/interactive-tutorials/test/import-products-gcs.test.js

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

Copy link
Copy Markdown
Contributor

@sofisl sofisl Apr 8, 2022

Choose a reason for hiding this comment

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

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();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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
@kweinmeister kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2022
@kweinmeister kweinmeister added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 28, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 28, 2022
@kweinmeister kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 30, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 30, 2022
@kweinmeister kweinmeister added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Mar 30, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 30, 2022
@kweinmeister kweinmeister added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 30, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 30, 2022
@product-auto-label product-auto-label Bot added the size: m Pull request size is medium. label Apr 5, 2022
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 5, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 5, 2022
@parthea parthea added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Apr 5, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 5, 2022
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 5, 2022
@kweinmeister kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 5, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 5, 2022
@kweinmeister kweinmeister added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Apr 5, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 5, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 5, 2022
@t-karasova t-karasova requested review from chenlei1216 and sofisl April 6, 2022 13:18
@bcoe bcoe added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Apr 13, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 13, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 13, 2022
@bcoe bcoe merged commit f4fa61c into googleapis:main Apr 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: retail Issues related to the googleapis/nodejs-retail API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants