Skip to content
This repository was archived by the owner on Nov 18, 2025. It is now read-only.

Conversation

@xiaozhenliu-gg5
Copy link
Contributor

Supporting IAM service in Gax-nodejs. Nodejs-kms and nodejs-pubsub are the two APIs that require IAM Client to be part of the methods [getIamPolicy, setIamPolicy, testIamPermission].

Monolith generator has a reroute_to_grpc_interface setting in GAPIC yaml to include extra services when generating client libraries. While this is not supported in micro-generator today. And IAM service is the only one referenced in that way but it does not have a generated client library [nodejs-IAM] for easy use. This is a pain point for every micro-generator and it makes post-processing script (synth.py in Typescript Microgenerator) really messy.

related issue: https://github.com/googleapis/gapic-generator-typescript/issues/315

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 27, 2020
@xiaozhenliu-gg5 xiaozhenliu-gg5 changed the title feat: add Iam_service feat: support Iam_service in Gax Mar 27, 2020
@codecov
Copy link

codecov bot commented Mar 27, 2020

Codecov Report

Merging #762 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     googleapis/gapic-generator-typescript#762   +/-   ##
=======================================
  Coverage   88.23%   88.23%           
=======================================
  Files          47       47           
  Lines        7965     7965           
  Branches      488      488           
=======================================
  Hits         7028     7028           
  Misses        934      934           
  Partials        3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a86191d...a86191d. Read the comment docs.

@xiaozhenliu-gg5
Copy link
Contributor Author

Current problem:

  1. System-test runs successfully at local, but fails here.
  2. 2 unit tests for longrunnning fail if IamService.ts unit test is added.

Questions: can we expose IamProtos from '../../protos/iam_service'? since from microgenerator, we need the type definition file for request/response type of methods (like protos.google.iam.v1.getIamPolicyRequest) @alexander-fenster Thank you

@alexander-fenster
Copy link
Contributor

runs successfully on local but fails here

Easy! rm -rf node_modules build package-lock.json && npm install :) It will likely start failing locally the same way then.

As for exposing - yes, sure, we can expose whatever we need there.

@xiaozhenliu-gg5
Copy link
Contributor Author

@alexander-fenster yes I removed all dependencies and build files, but still it passes the tests :(

Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

LGTM with nits. Would be great to have a system test covering this functionality, but I'm not sure how hard would it be to get one.

@xiaozhenliu-gg5 xiaozhenliu-gg5 requested a review from bcoe April 8, 2020 23:53
@xiaozhenliu-gg5 xiaozhenliu-gg5 changed the title feat: support Iam_service in Gax [DO NOT MERGE] feat: support Iam_service in Gax Apr 9, 2020
@xiaozhenliu-gg5
Copy link
Contributor Author

TO DO: once KMS is migrated using --iam-service option to include IAM methods, add one kms system-test to test this feature.

Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexander-fenster alexander-fenster changed the title [DO NOT MERGE] feat: support Iam_service in Gax feat: support Iam_service in Gax Apr 9, 2020
@xiaozhenliu-gg5 xiaozhenliu-gg5 merged commit a4c0dd3 into master Apr 9, 2020
yoshi-automation added a commit that referenced this pull request Apr 10, 2020
Source-Repo: googleapis/gax-nodejs
Source-Sha: a4c0dd3
Source-Link: a4c0dd3
Author: Xiaozhen Liu <xiaozhenliu@google.com>
Date: Thu Apr 9 16:43:17 2020 -0700
Original-Commit-Message: feat: support Iam_service in Gax (#762)

* add IAM service

* add unit test

* only iam

* sinon, reslove circular dependency

* test

* test

* test

* copyright

* test

* test

* test

* test

* debug

* fix cannot find module failure

* tests pass!

* sample/system-test pass locally

* unit tests pass without IAM serive test

* fix: proto copy

* proper unit test

* copyright

* export IAM protos

* init before calling IAM service

* compile-iam-protos & rename protosTypes

* comments

* import iamClient for unit test

* update iam protos by compile script

* add license to protos

* move expoted common interface to separat file

* header for clientInterface.ts

* accident

* export IamClient interface

* using Callback, unit test

* lint

Co-authored-by: Alexander Fenster <fenster@google.com>
yoshi-automation added a commit that referenced this pull request Apr 12, 2020
Source-Repo: googleapis/gax-nodejs
Source-Sha: a4c0dd3
Source-Link: a4c0dd3
Author: Xiaozhen Liu <xiaozhenliu@google.com>
Date: Thu Apr 9 16:43:17 2020 -0700
Original-Commit-Message: feat: support Iam_service in Gax (#762)

* add IAM service

* add unit test

* only iam

* sinon, reslove circular dependency

* test

* test

* test

* copyright

* test

* test

* test

* test

* debug

* fix cannot find module failure

* tests pass!

* sample/system-test pass locally

* unit tests pass without IAM serive test

* fix: proto copy

* proper unit test

* copyright

* export IAM protos

* init before calling IAM service

* compile-iam-protos & rename protosTypes

* comments

* import iamClient for unit test

* update iam protos by compile script

* add license to protos

* move expoted common interface to separat file

* header for clientInterface.ts

* accident

* export IamClient interface

* using Callback, unit test

* lint

Co-authored-by: Alexander Fenster <fenster@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants