feat: add support for reading google.api.api_version #1999

parthea · 2024-03-23T16:28:15Z

Towards b/330968465

conventional-commit-lint-gcf · 2024-04-08T21:03:24Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

gapic/templates/setup.py.j2

gapic/ads-templates/setup.py.j2

setup.py

gapic/templates/testing/_default_constraints.j2

gapic/templates/testing/constraints-3.7.txt.j2

vchudnov-g

Not a review; I know this is a draft at the moment. Just a fly-by comment.

vchudnov-g · 2024-04-10T18:29:41Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2

@@ -388,6 +391,12 @@ class {{ service.async_client_name }}:
        )
        {% endif %}

+        {% if service.version %}


It would be good if we could call this code in the _client_macros.j2 file, so we reduce duplication. Maybe a separate macro call set_version_header(service.version), and the macro could take care of the conditional.

Fixed in 009fc71

… the one in googleads/google-ads-python

Co-authored-by: Victor Chudnovsky <[email protected]>

gapic/templates/%namespace/%name_%version/%sub/services/%service/_shared_macros.j2

ohmayr · 2024-05-07T19:26:26Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+    api_core_major, api_core_minor = [int(part) for part in api_core_version.__version__.split(".")[0:2]]
+    if api_core_major > 2 or (api_core_major == 2 and api_core_minor >= 19):


We need to guard our tests like this every time a new feature is dependent on a version of api-core which is not the minimum version. This will also add a lot of toil for future clean up.

Instead, we should decide a generalized way of doing this and also keep track of the things that we need to clean up.

I would suggest using a helper function as a decorator for this. Something like:

def is_supported_version(major_version, minor_version): def decorator(func): @wraps(func) def wrapper(*args, **kwargs): api_core_major, api_core_minor = [int(part) for part in api_core_version.__version__.split(".")[0:2]] if api_core_major > major_version or (api_core_major == major_version and api_core_minor >= minor_version): return func(*args, **kwargs) else: print("Skipping test due to unsupported version ...") return wrapper return decorator

Let me know what you think!

A good idea. See my related comment about being able to just have a helper function to get the version parts. The decorator could be done as part of that work.

I filed #2023 to clean up the function as a follow up task so we don't block this PR indefinitely with improvements

ohmayr · 2024-05-07T19:41:04Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2

@@ -24,6 +25,7 @@ from google.api_core import retry_async as retries
 from google.auth import credentials as ga_credentials   # type: ignore
 from google.oauth2 import service_account              # type: ignore

+{{ shared_macros.add_google_api_core_version_header_import(service) }}


Why not just pass down service.version here? Same is true for add_api_version_header_to_metadata if that's the only attribute we need from service for these two macros.

Fixed in f150e67 and 220f873

vchudnov-g

Looks fine. My comments are minor, except for the one about the intended symlink not yet being a symlink; it looks like a regular file (which means we have the same file duplicated).

vchudnov-g · 2024-05-07T19:13:43Z

gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2

+        except ImportError:  # pragma: NO COVER
+            HAS_GOOGLE_API_CORE_VERSION_HEADER = False
+#}
+    {% if service.version %}


This does not show up as a symlink; see comment below.

vchudnov-g · 2024-05-07T19:20:13Z

gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/_shared_macros.j2

@@ -0,0 +1,69 @@
+{#


This is a regular file, not a symlink:

git ls-files -s gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/_shared_macros.j2 100644 7094d3039af53ead4b844ba9952d7084f0526bd2 0 gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/_shared_macros.j2

See the meaning of Git modes. (Credit: SO answer)

Good catch and thanks for the link! Fixed in 7957ded

(py39) partheniou@partheniou-vm-3:~/git/gapic-generator-python$ git ls-files -s gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/_shared_macros.j2 120000 6884f2def889f3f4f2a5b19b54e76a52c2826174 0 gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/_shared_macros.j2

I reverted this change in 542e4e1 and filed #2028 to investigate if symlinks can be used.

If the symlinks don't work (weird, they should), can you specify a template outside the current directory (something like {% include ../templates/foo/bar %}

If that also doesn't work, I would suggest keeping the duplicate files but adding a prominent header to the ads one saying that this is a copy of the one in the standard templates, intended to be a symlink. My rationale: we have duplicate code either way, and the macro file9s) makes them more organized....and it paves the way for the symlink once we can figure it out.

Oh, I see. You reverted back to the duplicate file, like I said in that last paragraph. SG. Could you add a note that this is intended to be a symlink to the other file, and for now is a duplicate?

gapic/templates/%namespace/%name_%version/%sub/services/%service/_shared_macros.j2

gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2

vchudnov-g · 2024-05-07T19:40:39Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2

+{{ shared_macros.add_google_api_core_version_header_import(service) }}
 try:


Do we need blank lines before and/or after?

Suggested change

{{ shared_macros.add_google_api_core_version_header_import(service) }}

try:

{{ shared_macros.add_google_api_core_version_header_import(service) }}

try:

I applied this change but reverted it in 16402e1 as it caused unnecessary blank lines to be added to the golden files as the import statements in shared_macros.add_google_api_core_version_header_import(service) are guarded by conditional logic

OK. Hopefully the generated files are nicely formatted through all code paths.

tests/system/test_streams.py

vchudnov-g · 2024-05-07T19:56:55Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+    api_core_major, api_core_minor = [int(part) for part in api_core_version.__version__.split(".")[0:2]]
+    if api_core_major > 2 or (api_core_major == 2 and api_core_minor >= 19):


A good idea. See my related comment about being able to just have a helper function to get the version parts. The decorator could be done as part of that work.

…ce/_shared_macros.j2 Co-authored-by: Victor Chudnovsky <[email protected]>

This reverts commit 7957ded.

…ce/client.py.j2 Co-authored-by: Victor Chudnovsky <[email protected]>

…s/%service/client.py.j2" This reverts commit 0f68169.

vchudnov-g

Just one suggestion to flag the intended symlink.

Thanks for working on this and responding to the review comments!

vchudnov-g · 2024-05-07T21:03:14Z

gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/_shared_macros.j2

@@ -38,32 +38,32 @@
 {% endwith %}{# method_settings #}
 {% endmacro %}

-{% macro add_google_api_core_version_header_import(service) %}
+{% macro add_google_api_core_version_header_import(service_version) %}


Could you add a note at the top that this is intended to be a symlink to the other file, and for now is a duplicate (link to the issue you filed)?

Fixed in a62b313

product-auto-label bot added the size: s Pull request size is small. label Mar 23, 2024

parthea force-pushed the add-support-for-reading-api-version branch from d002b39 to 950e89b Compare April 8, 2024 21:03

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Apr 8, 2024

parthea force-pushed the add-support-for-reading-api-version branch 2 times, most recently from bf409e6 to 816a2cf Compare April 10, 2024 14:10

parthea commented Apr 10, 2024

View reviewed changes

parthea force-pushed the add-support-for-reading-api-version branch 3 times, most recently from 5a57275 to c61464c Compare April 10, 2024 15:29

feat: add support for reading google.api.api_version

4e3138c

parthea force-pushed the add-support-for-reading-api-version branch from c61464c to 4e3138c Compare April 10, 2024 17:14

parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 10, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 10, 2024

vchudnov-g reviewed Apr 10, 2024

View reviewed changes

add showcase tests; clean up code

009fc71

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 29, 2024

parthea added 3 commits April 29, 2024 21:45

remove noise in diff

b574ed1

remove noise in diff

dd96aa4

remove noise in diff

de4ba22

parthea mentioned this pull request Apr 29, 2024

feat(server): echo request headers in response over REST transport googleapis/gapic-showcase#1509

Merged

parthea added 7 commits April 29, 2024 22:14

use gapic-showcase 0.35.0

28e6991

update comment

fca5b55

add constraints to ads templates

8cd648a

add new line

35755b7

update google-api-core and googleapis-common-protos setup.py to match…

ea5666f

… the one in googleads/google-ads-python

update grpc in setup.py to match the one in googleads/google-ads-python

79f1767

style

7eb5c92

parthea and others added 5 commits May 2, 2024 12:13

update copyright year

2e666ea

Co-authored-by: Victor Chudnovsky <[email protected]>

address review feedback

1e6e96e

fix comment

6416902

revert constraints files for ads

68b4010

remove test code

178c964

parthea mentioned this pull request May 2, 2024

Create a macro utility function to parse the version of dependencies #2023

Open

parthea added 4 commits May 4, 2024 15:12

address review feedback

a5d6109

address review feedback

109edd4

revert WORKSPACE

12ff101

clean up

ca95a6b

parthea mentioned this pull request May 6, 2024

Improve asserts when checking metadata in autogenerated unit tests #2024

Open

parthea requested a review from vchudnov-g May 6, 2024 16:18

ohmayr reviewed May 7, 2024

View reviewed changes

vchudnov-g reviewed May 7, 2024

View reviewed changes

Update gapic/templates/%namespace/%name_%version/%sub/services/%servi…

0e669ab

…ce/_shared_macros.j2 Co-authored-by: Victor Chudnovsky <[email protected]>

ohmayr approved these changes May 7, 2024

View reviewed changes

parthea added 2 commits May 7, 2024 20:16

convert to symlink

7957ded

Revert "convert to symlink"

542e4e1

This reverts commit 7957ded.

parthea mentioned this pull request May 7, 2024

Determine if symlinks can be used with jinja templates #2028

Open

parthea and others added 6 commits May 7, 2024 16:32

Update gapic/templates/%namespace/%name_%version/%sub/services/%servi…

0f68169

…ce/client.py.j2 Co-authored-by: Victor Chudnovsky <[email protected]>

address review feedback

921f20b

Revert "Update gapic/templates/%namespace/%name_%version/%sub/service…

16402e1

…s/%service/client.py.j2" This reverts commit 0f68169.

fix build

555cff7

Address review feedback

f150e67

Address review feedback

220f873

vchudnov-g approved these changes May 7, 2024

View reviewed changes

address review feedback

a62b313

parthea merged commit b2486e5 into main May 8, 2024
67 checks passed

parthea deleted the add-support-for-reading-api-version branch May 8, 2024 17:48

release-please bot mentioned this pull request May 8, 2024

chore(main): release 1.18.0 #2029

Merged

		api_core_major, api_core_minor = [int(part) for part in api_core_version.__version__.split(".")[0:2]]
		if api_core_major > 2 or (api_core_major == 2 and api_core_minor >= 19):

		{{ shared_macros.add_google_api_core_version_header_import(service) }}
		try:

feat: add support for reading google.api.api_version #1999

feat: add support for reading google.api.api_version #1999

Uh oh!

Conversation

parthea commented Mar 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conventional-commit-lint-gcf bot commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthea May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

parthea commented Mar 23, 2024 •

edited

Loading

conventional-commit-lint-gcf bot commented Apr 8, 2024 •

edited

Loading

parthea May 7, 2024 •

edited

Loading