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.

Sign up for GitHub

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

Jump to bottom

feat: Automatically populate uuid4 fields #1985

Merged
merged 14 commits into from
Mar 22, 2024
Merged

feat: Automatically populate uuid4 fields #1985

merged 14 commits into from
Mar 22, 2024

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Mar 18, 2024 •
edited
Loading

Adds support for automatically populating uuid fields as per https://google.aip.dev/client-libraries/4235

Towards b/322910372

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Mar 18, 2024
@parthea parthea force-pushed the auto-populate-uuid4-fields branch 4 times, most recently from 7f93c76 to 03cc1e1 Compare March 19, 2024 02:59
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Mar 19, 2024
@parthea parthea changed the title feat: Auto populate uuid4 fields feat: Automatically populate uuid4 fields Mar 19, 2024
@parthea parthea self-assigned this Mar 19, 2024
@parthea parthea force-pushed the auto-populate-uuid4-fields branch 2 times, most recently from 247c62b to 66c4309 Compare March 20, 2024 18:04
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: m Pull request size is medium. labels Mar 20, 2024
@parthea parthea force-pushed the auto-populate-uuid4-fields branch 2 times, most recently from 497f954 to c2feb1a Compare March 20, 2024 19:19
@parthea parthea marked this pull request as ready for review March 20, 2024 19:19
@parthea parthea requested a review from a team as a code owner March 20, 2024 19:19
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 20, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 20, 2024
@parthea parthea assigned ohmayr, vchudnov-g and dizcology and unassigned parthea Mar 20, 2024
@parthea parthea marked this pull request as draft March 20, 2024 19:34
@parthea parthea force-pushed the auto-populate-uuid4-fields branch from c2feb1a to 68a79a5 Compare March 20, 2024 19:48
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 20, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 20, 2024
@parthea parthea marked this pull request as ready for review March 20, 2024 19:54
@parthea parthea mentioned this pull request Mar 20, 2024
Open
vchudnov-g
vchudnov-g reviewed Mar 20, 2024
View reviewed changes
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Only minor comments in the files, but one overall comment before I approve: could we have some sort of golden file that exercises the uuid tests you added? (i.e. from a proto with an autopopulated field)

gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 Outdated Show resolved Hide resolved
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 Outdated Show resolved Hide resolved
gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved
gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2 Outdated Show resolved Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2 Show resolved Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2 Show resolved Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated Show resolved Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated Show resolved Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated Show resolved Hide resolved
@parthea
Copy link
Contributor Author

parthea commented Mar 21, 2024

could we have some sort of golden file that exercises the uuid tests you added? (i.e. from a proto with an autopopulated field)

This is already being done since we're running tests against gapic-showcase version 0.32.0. See the showcase-unit presubmits.

gapic-generator-python/noxfile.py

Line 197 in 24a23a1

# Install a client library for Showcase.

On an unrelated note, I've opened PR #1991 to add showcase to the goldens directory but this does not add value in terms of unit testing as this is already being done.

@parthea parthea requested a review from vchudnov-g March 21, 2024 11:55
@parthea
Copy link
Contributor Author

parthea commented Mar 21, 2024 •
edited
Loading

@vchudnov-g Please could you take a look?

vchudnov-g
vchudnov-g approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated
{% if method_settings is not none %}
{% for auto_populated_field in method_settings.auto_populated_fields %}
# Ensure that the uuid4 field is set according to AIP 4235
assert re.match(r"[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}", args[0].{{ auto_populated_field }})
Copy link
Contributor

Choose a reason for hiding this comment

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

We have this check in multiple places, so I suggest having this in a macro. Even if we have to have duplicate macros for the ads vs non-ads templates (we can define the Ads macro in this file, IIUC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 70f0c64

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated
{% if method_settings is not none %}
{% for auto_populated_field in method_settings.auto_populated_fields %}
# Ensure that the uuid4 field is set according to AIP 4235
assert re.match(r"[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}", args[0].{{ auto_populated_field }})
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto previous comment: We have this check in multiple places, so I suggest having this in a macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 70f0c64

gapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2 Show resolved Hide resolved
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 Outdated Show resolved Hide resolved
@parthea parthea mentioned this pull request Mar 22, 2024
Closed
@parthea parthea enabled auto-merge (squash) March 22, 2024 10:40
@parthea parthea merged commit eb57e4f into main Mar 22, 2024
67 checks passed
@parthea parthea deleted the auto-populate-uuid4-fields branch March 22, 2024 10:43
@release-please release-please bot mentioned this pull request Mar 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vchudnov-g vchudnov-g vchudnov-g approved these changes

Assignees

@vchudnov-g vchudnov-g

@dizcology dizcology

@ohmayr ohmayr

Labels
size: xl Pull request size is extra large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@parthea @vchudnov-g @dizcology @ohmayr