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: use iam client library to setup test #1173

Merged
merged 11 commits into from Jun 9, 2021

Conversation

xil222
Copy link
Contributor

@xil222 xil222 commented May 17, 2021

This PR is for implementing using IAM client library for setting up externalClient tests, without making any changes to
current tests.
The bug could be tracked from b/185501294.
@bojeil-google

@xil222 xil222 requested a review from a team as a code owner May 17, 2021 23:39
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 17, 2021
@xil222 xil222 changed the title Use iam client library to setup test Fix: Use iam client library to setup test. May 18, 2021
@xil222 xil222 changed the title Fix: Use iam client library to setup test. fix: use iam client library to setup test May 18, 2021
package.json Outdated
@@ -23,6 +23,8 @@
"fast-text-encoding": "^1.0.0",
"gaxios": "^4.0.0",
"gcp-metadata": "^4.2.0",
"google-auth-library": "^7.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

If these are just being used in samples, you would need to add them to samples/package.json. There should already be a reference to google-auth-library there, since it gets linked in as part of the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1.
You just need to add the additional dependency, e.g. googleapis which can probably go here:

"google-auth-library": "^7.1.0",

samples/scripts/externalclient-setup.js Outdated Show resolved Hide resolved
// changes.
// https://cloud.google.com/iam/docs/reference/rest/v1beta/projects.locations.workloadIdentityPools
// https://github.com/googleapis/google-api-nodejs-client/tree/master/src/apis/iam
const authClient = await auth.getClient();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can skip this line entirely and say google.options({auth}); and it will work just the same :)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

package.json Outdated
@@ -23,6 +23,8 @@
"fast-text-encoding": "^1.0.0",
"gaxios": "^4.0.0",
"gcp-metadata": "^4.2.0",
"google-auth-library": "^7.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

+1.
You just need to add the additional dependency, e.g. googleapis which can probably go here:

"google-auth-library": "^7.1.0",

policy: {
bindings,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of the lint errors can be fixed by running npm run fix.
You can also see the list of errors by running npm run lint. This is useful for the errors that require manual fixing.

samples/scripts/externalclient-setup.js Outdated Show resolved Hide resolved
// changes.
// https://cloud.google.com/iam/docs/reference/rest/v1beta/projects.locations.workloadIdentityPools
// https://github.com/googleapis/google-api-nodejs-client/tree/master/src/apis/iam
const authClient = await auth.getClient();
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

},
response = await iam.projects.locations.workloadIdentityPools.create({
parent: `projects/${projectId}/locations/global`,
workloadIdentityPoolId: poolId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add a display name and description: https://github.com/googleapis/google-api-nodejs-client/blob/19bd611ca69f334b342565698a8ec8991e956580/src/apis/iam/v1.ts#L2412

This makes it easy to understand who created this pool (It will also work well in the Cloud Console UI).

Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

Looks good from my end. Leaving it to repo owners for the final say.

@bojeil-google
Copy link
Contributor

@bcoe this is one of the action items from when we added the integration tests before launch. Your input is highly appreciated.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Just a nit, regarding using the IAM library directly. I appreciate this refactor.

@@ -14,6 +14,7 @@
"license": "Apache-2.0",
"dependencies": {
"google-auth-library": "^7.1.0",
"googleapis": "^73.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could I get you to try out https://www.npmjs.com/package/@googleapis/iam, I love to stress test our individual modules, and it should improve install size.

Copy link
Contributor Author

@xil222 xil222 Jun 8, 2021

Choose a reason for hiding this comment

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

Thanks for your feedback. In externalclient-setup.js, I still need to set auth with import from googleapis:
google.options({auth});
I am not able to setup the auth if only install @googleapis/iam for this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

won't this work?

const Iam = require('@googleapis/iam')
const iam = await iam.iam({
    version: 'v1',
    auth
});
const response = await iam.projects.locations.workloadIdentityPools.create({});

You can pass auth to the client itself, it doesn't need to be configured globally (perhaps I'm missing the issue though.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is working. Thank you.

@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 9, 2021
@bcoe bcoe 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 Jun 9, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 9, 2021
@bcoe bcoe merged commit 74ac5db into googleapis:master Jun 9, 2021
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants