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

Addition of policy and options to reserved names in #824 results in breaking changes to stable libraries #835

Closed
busunkim96 opened this issue Apr 6, 2021 · 7 comments · Fixed by #851
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@busunkim96
Copy link
Contributor

busunkim96 commented Apr 6, 2021

These APIs have field renames resulting from this #824 which expanded the RESERVED_NAMES list. Some of them are GA, and even for beta APIs I'd prefer not to take changes that looks spurious from the user's perspective.

googleapis/python-artifact-registry#12
googleapis/python-cloudbuild#81
googleapis/python-iot#87
googleapis/python-functions#45
googleapis/python-securitycenter#120
googleapis/python-bigquery-connection#57
googleapis/python-containeranalysis#112
googleapis/python-bigtable#280
googleapis/python-tasks#85
googleapis/python-policy-troubleshooter#8
googleapis/python-asset#171
googleapis/python-org-policy#32
googleapis/python-binary-authorization#18
googleapis/python-spanner#303
googleapis/python-secret-manager#100
googleapis/python-firestore#336
googleapis/python-billing#55
googleapis/python-datacatalog#143

policy and options are the two that show up in these APIs.

From looking at the regen, it doesn't look like these need to be added to the reserved names list? f5e9f36 seems to result in the imports being renamed to not conflict. I see from google.iam.v1 import policy_pb2 as giv_policy (example) instead of from google.iam.v1 import policy_pb2 as policy.

Can policy and options be removed from the list? @software-dov

@software-dov
Copy link
Contributor

Options cannot, policy may be. @parthea can chime in for context, I think he's working with the APIs that require these names.

@parthea
Copy link
Contributor

parthea commented Apr 9, 2021

The issue I encountered was with keyword auth in API servicemanagement v1 at this line. We could certainly limit the impact of this change if we only add auth as a reserved name.

Prior to generator version 0.43.2 there was a conflict with auth

from google import auth
from google.api import auth_pb2 as auth  # type: ignore

With the fix in version 0.43.2

from google import auth
from google.api import auth_pb2 as ga_auth  # type: ignore

You can see the change here:
https://github.com/googleapis/googleapis-gen/commit/107ed1217b5e87048263f52cd3911d5f851aca7e#diff-c8fa0e83f42f4ec28657fd35abc265e70911e8487762f7ae1891e4747ba791b9L28

@busunkim96
Copy link
Contributor Author

Given what @parthea shared, is it possible to back out the change?

Or can we tweak the raw imports in the templates to use references that are less likely to conflict?

from google import auth

credentials, _ = auth.default(scopes=self._scopes, quota_project_id=quota_project_id)
import google.auth

credentials, _ = google.auth.default(scopes=self._scopes, quota_project_id=quota_project_id)

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 12, 2021
@parthea
Copy link
Contributor

parthea commented Apr 16, 2021

@software-dov, PTAL

@tswast
Copy link

tswast commented Apr 20, 2021

I think this is breaking generation of the datacatalog library googleapis/python-datacatalog#143 (comment)

@software-dov
Copy link
Contributor

Do we have a sense of how time critical this issue is, or a required timeline?

@busunkim96
Copy link
Contributor Author

@software-dov I'm currently blocking generator updates on the repos listed. So any features since that change (GAPIC metadata and self-signed JWT) will not land in those repos for the time being.

I could technically add synth replaces to avoid the breaking change, but would rather not do so for 10+ repos since it is error prone.

busunkim96 added a commit that referenced this issue Apr 30, 2021
)

Fixes #835.

Breakdown by name that was originally added in #824
- `auth`: `from google import auth` -> `import google.auth`
- `credentials`: `from google.auth import credentials` -> `from google.auth import credentials as ga_credentials`
- `exceptions`: `from google.api_core import exceptions` -> `from google.api_core import exceptions as core_exceptions`
- `future`: skipped, as it is only used in the [generated tests](https://github.com/googleapis/gapic-generator-python/search?q=%22import+future%22) and has a low chance of colliding
- `options` `from google.iam.v1 import options_pb2 as options` -> `from google.iam.v1 import options_pb2`
- `policy` `from google.iam.v1 import policy_pb2 as policy` -> `from google.iam.v1 import policy_pb2`
- `math` skipped as it is only used in [generated tests](https://github.com/googleapis/gapic-generator-python/search?q=%22import+math%22)

For `options` and `policy` there is a small change to `gapic/schema/metadata.py` to not alias `_pb2` types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants