-
Notifications
You must be signed in to change notification settings - Fork 85
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.
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
feat: Add discogapic support for GAPICBazel generation #459
Conversation
@chingor13 PTAL |
@chingor13 The build is green now! |
synthtool/gcp/gapic_bazel.py
Outdated
source_repo = None | ||
source_repo_name = None | ||
if discogapic: | ||
source_repo = self._clone_discovery_artifact_manager() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"source" is still kinda ambiguous. Could rename these variables:
source_repo_with_protos
name_of_source_repo_with_protos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to rename it, but _proto
suffix would not work for the discovery-artifact-manager
repo, because it is a repo with discovery docs (jsons). Maybe something like api_definitions_repo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like api_definitions_repo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to change the behavior of the DiscogapicGenerator class which was already being used to use bazel instead? We can extract bazel functionality into a separate module at some point if we see lots of duplication?
Edit: We can refactor later, this is not a huge deal to me.
@chingor13 It is more of a philosophical question. With bazel, there should be no difference which language it is or in which way APIs are declared. We simply call a bazel target with a specified name and "magic happens". I.e. the target state we should aim for is to not have any special classes for different ways of generating things in synthtool. We just call a target in bazel, always in the same way. The only thing which is different per client is the bazel target name itself. The other clasess (GAPICGenerator, DiscoOGAPICGenerator, GAPICMicrogenerator etc) should go away eventually. That is why I reused GAPICBazel class instead. It was very little work (basically the only thing I had to add was a new repo), everything else was reused as is (plus some fixes, not related to discogapic part). |
synthtool/gcp/gapic_bazel.py
Outdated
source_repo = None | ||
source_repo_name = None | ||
if discogapic: | ||
source_repo = self._clone_discovery_artifact_manager() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like api_definitions_repo
.
This is blocking generation for a few libraries. We will refactor separately if necessary. |
No description provided.