feat(generator): gapic generator centralization routing#17816
feat(generator): gapic generator centralization routing#17816hebaalazzeh wants to merge 11 commits into
Conversation
![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 introduces a new compatibility template _compat.py.j2 to provide fallback implementations for get_default_mtls_endpoint, get_api_endpoint, and get_universe_domain when they are not available in google.api_core.universe. The client templates are updated to delegate these operations to the new compatibility module, and redundant unit tests are removed from the generated test suites. The review feedback highlights that both get_api_endpoint in _compat.py.j2 and _get_api_endpoint in client.py.j2 can return None under certain conditions, and recommends updating their return type annotations to Optional[str] for better type safety.
b8ed069 to
b7054c2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a compatibility module template _compat.py.j2 to centralize fallback logic for older versions of google-api-core, updating the generator and client templates to import and use these shared utilities. Feedback on the changes highlights two critical issues regarding generated files: first, the new mock.patch("os.path.exists", ...) in the golden files is missing from the generator's template test_%service.py.j2; second, the new test file test__compat.py was added directly to a golden directory without a corresponding template, which will cause integration test failures.
| config_file_content = json.dumps(config_data) | ||
| m = mock.mock_open(read_data=config_file_content) | ||
| with mock.patch("builtins.open", m): | ||
| with mock.patch("builtins.open", m), mock.patch("os.path.exists", side_effect=lambda path: os.path.basename(path) == config_filename): |
There was a problem hiding this comment.
The addition of mock.patch("os.path.exists", ...) is present in the golden files (such as this one) but is missing from the generator template test_%service.py.j2. Since golden files are entirely generated, any manual changes to them will be overwritten the next time the generator is run. Please update the test_%service.py.j2 template to include this mock patch so that it is correctly generated for all clients.
References
- Do not suggest manual code modifications, optimizations, or style fixes for generated files, as any changes should be implemented in the generator or templates to prevent them from being overwritten.
| from google.cloud.storagebatchoperations_v1 import _compat | ||
|
|
||
|
|
||
| def test_setup_request_id_fallback(): |
There was a problem hiding this comment.
This test file test__compat.py is added directly to the storagebatchoperations golden directory, but there is no corresponding test__compat.py.j2 template in gapic/templates/tests/unit/gapic/. If this file is not generated by the generator, the integration tests (which compare the generated output with the golden directory) will fail due to the extra file in the golden directory. If _compat.py needs to be tested, we should either create a test__compat.py.j2 template so it is generated for all clients, or move this test to the generator's own unit tests.
References
- Do not suggest manual code modifications, optimizations, or style fixes for generated files, as any changes should be implemented in the generator or templates to prevent them from being overwritten.
f61bfa7 to
a89c8ca
Compare
1 participant
This PR updates the generator templates to rely on the centralized endpoint routing and universe domain logic from
google-api-core.Since PR #17799 successfully centralized these helpers (
get_api_endpoint,get_universe_domain,get_default_mtls_endpoint) intogoogle/api_core/universe.py, we no longer need the generator to produce fallback implementations for them. The generated_compat.pywill now import these directly fromuniverse.py.(Note: This replaces the previous PR #17746 which was auto-closed due to a deleted base branch).