Skip to content
This repository was archived by the owner on Mar 26, 2026. It is now read-only.

feat: Automatically populate uuid4 fields - #1985

Merged
parthea merged 14 commits into
mainfrom
auto-populate-uuid4-fields
Mar 22, 2024
Merged

feat: Automatically populate uuid4 fields#1985
parthea merged 14 commits into
mainfrom
auto-populate-uuid4-fields

Conversation

@parthea

@partheaparthea commented Mar 18, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

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

Towards b/322910372

@product-auto-labelproduct-auto-labelBot added the size: l Pull request size is large. label Mar 18, 2024
@parthea
partheaforce-pushed the auto-populate-uuid4-fields branch 4 times, most recently from 7f93c76 to 03cc1e1CompareMarch 19, 2024 02:59
@product-auto-labelproduct-auto-labelBot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Mar 19, 2024
@partheaparthea changed the title feat: Auto populate uuid4 fieldsfeat: Automatically populate uuid4 fieldsMar 19, 2024
@partheaparthea self-assigned this Mar 19, 2024
@parthea
partheaforce-pushed the auto-populate-uuid4-fields branch 2 times, most recently from 247c62b to 66c4309CompareMarch 20, 2024 18:04
@product-auto-labelproduct-auto-labelBot added size: xl Pull request size is extra large. and removed size: m Pull request size is medium. labels Mar 20, 2024
@parthea
partheaforce-pushed the auto-populate-uuid4-fields branch 2 times, most recently from 497f954 to c2feb1aCompareMarch 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 teamMarch 20, 2024 19:19
@partheaparthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 20, 2024
@gcf-owl-botgcf-owl-botBot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 20, 2024
@parthea
parthea marked this pull request as draft March 20, 2024 19:34
@parthea
partheaforce-pushed the auto-populate-uuid4-fields branch from c2feb1a to 68a79a5CompareMarch 20, 2024 19:48
@partheaparthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 20, 2024
@gcf-owl-botgcf-owl-botBot 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
@partheaparthea mentioned this pull request Mar 26, 2026
Closed
vchudnov-g
vchudnov-g reviewed Mar 20, 2024
View reviewed changes

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

Copy link
Copy Markdown
Contributor

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)

Comment threadgapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 Outdated
Comment threadgapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 Outdated
Comment threadgapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated
Comment threadgapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated
Comment threadgapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2 Outdated
Comment threadgapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2
Comment threadgapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2
Comment threadgapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated
Comment threadgapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated
Comment threadgapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated
@parthea

parthea commented Mar 21, 2024

Copy link
Copy Markdown
ContributorAuthor

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-gMarch 21, 2024 11:55
@parthea

parthea commented Mar 21, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@vchudnov-g Please could you take a look?

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

@vchudnov-gvchudnov-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for doing this!

Comment threadgapic/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 }})

@vchudnov-gvchudnov-gMar 21, 2024

Copy link
Copy Markdown
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)

@partheapartheaMar 22, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed in 70f0c64

Comment threadgapic/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 }})

@vchudnov-gvchudnov-gMar 21, 2024

Copy link
Copy Markdown
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.

@partheapartheaMar 22, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed in 70f0c64

Comment threadgapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2
Comment threadgapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 Outdated
@partheaparthea 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 mainMar 22, 2024
@parthea
parthea deleted the auto-populate-uuid4-fields branch March 22, 2024 10:43
@release-pleaserelease-pleaseBot mentioned this pull request Mar 22, 2024
Merged
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@vchudnov-gvchudnov-gvchudnov-g approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@vchudnov-gvchudnov-g

@dizcologydizcology

@ohmayrohmayr

Labels

size: xlPull 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