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: add generator option proto-plus-dep #1596

Merged
merged 28 commits into from
Mar 17, 2023
Merged

feat: add generator option proto-plus-dep #1596

merged 28 commits into from
Mar 17, 2023

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Feb 14, 2023 •
edited
Loading

fix: resolve ImportError when GAPIC depends on another GAPIC

Fixes b/266837434
Fixes googleapis/google-cloud-python#10828
Fixes #1611

Googlers see go/fix-gapic-depends-on-gapic-with-proto-plus-types

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Feb 14, 2023
@parthea parthea force-pushed the fix-proto-plus-dependencies-bazel branch from 43a510b to 5f5f581 Compare February 28, 2023 19:28
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 1, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 1, 2023
@parthea parthea marked this pull request as ready for review March 1, 2023 14:05
@parthea parthea requested review from a team as code owners March 1, 2023 14:05
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 1, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 1, 2023
vchudnov-g
vchudnov-g requested changes Mar 3, 2023
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.

Don't know much about proto-plus so I can't comment on those aspects of it, but otherwise this looks good.

Two important issues: making the regex more robust, and capturing an item to reduce the hard-coding tech debt for specific API×version.

gapic/schema/metadata.py Outdated Show resolved Hide resolved
gapic/templates/setup.py.j2 Outdated Show resolved Hide resolved
gapic/templates/setup.py.j2 Outdated Show resolved Hide resolved
gapic/templates/testing/constraints-3.7.txt.j2 Outdated Show resolved Hide resolved
gapic/schema/metadata.py Outdated Show resolved Hide resolved
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Mar 6, 2023
@parthea parthea requested a review from vchudnov-g March 8, 2023 13:42
vchudnov-g
vchudnov-g approved these changes Mar 8, 2023
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.

One test suggestion and one question, but otherwise LGTM

tests/unit/schema/test_api.py Outdated Show resolved Hide resolved
gapic/templates/setup.py.j2 Show resolved Hide resolved
This was referenced Mar 14, 2023
Merged
Closed
@parthea
Copy link
Contributor Author

parthea commented Mar 14, 2023

Waiting on approval in go/fix-gapic-depends-on-gapic-with-proto-plus-types

@parthea parthea added the status: blocked Resolving the issue is dependent on other work. label Mar 14, 2023
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 14, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 14, 2023
atulep
atulep reviewed Mar 16, 2023
View reviewed changes
gapic/schema/metadata.py Outdated Show resolved Hide resolved
atulep
atulep reviewed Mar 16, 2023
View reviewed changes
gapic/schema/metadata.py Outdated Show resolved Hide resolved
@parthea parthea removed the status: blocked Resolving the issue is dependent on other work. label Mar 17, 2023
@parthea parthea enabled auto-merge (squash) March 17, 2023 10:47
@parthea parthea merged commit 1c4de67 into main Mar 17, 2023
@parthea parthea deleted the fix-proto-plus-dependencies-bazel branch March 17, 2023 10:57
@release-please release-please bot mentioned this pull request Mar 17, 2023
Merged
This was referenced Mar 17, 2023
Closed
Merged
Closed
@parthea parthea mentioned this pull request Aug 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atulep atulep atulep left review comments

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

Assignees
No one assigned
Labels
size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect assumption in gapic-generator-python for dependencies cannot run maps-fleetengine-delivery-v1
3 participants
@parthea @vchudnov-g @atulep