chore(generator): move read_environment_variables to _compat.py.j2 - #17997
chore(generator): move read_environment_variables to _compat.py.j2#17997hebaalazzeh wants to merge 4 commits into
read_environment_variables to _compat.py.j2#17997Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the environment variable reading logic by moving _read_environment_variables from the service client template into a shared helper function read_environment_variables in _compat.py.j2, and adds corresponding unit tests. The review feedback highlights critical issues: a missing closing parenthesis in _compat.py.j2 that will cause a syntax error, and missing imports for MutualTLSChannelError in both _compat.py.j2 and test_compat.py.j2 which will lead to runtime NameErrors.
| google.auth.exceptions.MutualTLSChannelError: If GOOGLE_API_USE_MTLS_ENDPOINT | ||
| is not any of ["auto", "never", "always"]. | ||
| """ | ||
| use_client_cert = should_use_client_cert() |
There was a problem hiding this comment.
MutualTLSChannelError is raised in read_environment_variables but is not imported in this file. Please import it from google.auth.exceptions to avoid a NameError at runtime.
from google.auth.exceptions import MutualTLSChannelError
use_client_cert = should_use_client_cert()
| {% set package_path = api.naming.module_namespace|join('.') + "." + api.naming.versioned_module_name %} | ||
| from {{package_path}}._compat import transcode_request | ||
| from {{package_path}}._compat import get_universe_domain, get_api_endpoint, get_default_mtls_endpoint, should_use_client_cert | ||
| from {{package_path}}._compat import get_universe_domain, get_api_endpoint, get_default_mtls_endpoint, should_use_client_cert, read_environment_variables |
There was a problem hiding this comment.
MutualTLSChannelError is used in test_read_environment_variables but is not imported in this test file. Please import it from google.auth.exceptions to avoid a NameError during test execution.
from {{package_path}}._compat import get_universe_domain, get_api_endpoint, get_default_mtls_endpoint, should_use_client_cert, read_environment_variables
from google.auth.exceptions import MutualTLSChannelError
|
|
||
| {# TODO(https://github.com/googleapis/google-cloud-python/issues/17883): | ||
| Defer to google.auth.transport.mtls.should_use_mtls_endpoint to parse | ||
| GOOGLE_API_USE_MTLS_ENDPOINT when available in minimum supported google-auth. #} |
There was a problem hiding this comment.
Can you add the minimum supported version here?
| "Environment variable `GOOGLE_API_USE_MTLS_ENDPOINT` must be `never`," | ||
| " `auto` or `always`" | ||
| ) | ||
| return use_client_cert, use_mtls_endpoint, universe_domain_env |
There was a problem hiding this comment.
it seems like we could return a bool for use_mtls_endpoint, to align with the auth implementaiton.
IIUC, the only place this value is used, we convert it to a bool anyway:
use_mtls=self._use_mtls_endpoint == "always" or (
self._use_mtls_endpoint == "auto" and self._client_cert_source
),
2 participants
We move
_read_environment_variablesand its associated unit tests into_compat.py.j2asread_environment_variables.This reduces code duplication in packages that have multiple services. In the future, once
should_use_mtls_endpointis added togoogle-auth, we can defer parsingGOOGLE_API_USE_MTLS_ENDPOINTupstream.Towards: #17883