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

feat: adds support for MixIns. #1240

Merged
merged 29 commits into from
Apr 8, 2022
Merged

feat: adds support for MixIns. #1240

merged 29 commits into from
Apr 8, 2022

Conversation

atulep
Copy link
Contributor

@atulep atulep commented Mar 17, 2022

MixIn methods include LRO, IAM and Location. Python GAPIC currently supports APIs that use LRO and IAM.

In this PR:

  • Expose MixIn methods in GRPC client (including async client).
  • MixIn methods are determined based on the contents of service YAML.
  • IAM mixin will not be added if the service proto contains any of IAM methods (i.e. TestIamPermissions, GetIamPolicy, SetIamPolicy).

This PR notably does not remove support for optis.add_iam_methods flag. I will do so in the subsequent PR to avoid breaking users. Current implementation adds iam methods once only if either the add_iam_methods or it was included in service YAML.

Note: this adds a new dependency to the generator (grpc-google-iam-v1)

@atulep atulep marked this pull request as ready for review March 24, 2022 22:45
@atulep atulep requested review from a team as code owners March 24, 2022 22:45
@atulep
Copy link
Contributor Author

atulep commented Mar 24, 2022

I will fix integration tests after an LGTM - just need to regenerate golden files.

Copy link
Contributor

@vam-google vam-google 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 few comments. Also please regenerate the golden tests.

gapic/schema/api.py Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
gapic/schema/api.py Show resolved Hide resolved
gapic/schema/api.py Show resolved Hide resolved
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM, but please comment out the exposed operation_client as per our discussion (and Noah seems to be ok with it). We can always put it back easily if it turns out to be necessary.

@atulep atulep merged commit 856af2e into googleapis:main Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants