feat: generate proto-only repository #2720
Conversation
product-auto-label
bot
added
size: m
JoeWang1127
changed the title
I don't think it makes sense for both java-iam and java-common-protos to have READMEs, I think we can remove them and exclude them from generation. WDYT @suztomo ? |
| @@ -61,6 +63,15 @@ def get_proto_path_to_library_name(self) -> dict[str, str]: | |||
| def is_monorepo(self) -> bool: | |||
| return len(self.libraries) > 1 | |||
|
|
|||
| def contains_common_protos(self) -> bool: | |||
There was a problem hiding this comment.
I added this method to determine whether there are common protos in the configuration.
We can't just have one is_gapic_monorepo method since we have three cases:
- sdk-platform-java,
contains_common_protosreturnsTrue - google-cloud-java,
contains_common_protosreturnsFalseandis_monoreporeturnsTrue - handwritten libraries,
contains_common_protosreturnsFalseandis_monoreporeturnsFalse
There was a problem hiding this comment.
We have one method to set the value and one to compute, plus we store a variable in the config object. Maybe we can put the logic of the setter in the getter method and compute the value once in the field we create in the constructor. That would leave us with one less method.
There was a problem hiding this comment.
I removed the setter method.
| ) | ||
|
|
||
| # we skip monorepo_postprocessing if not in a monorepo | ||
| if not config.is_monorepo(): | ||
| if not config.is_monorepo() or config.contains_common_protos(): |
There was a problem hiding this comment.
Only perform monorepo post processing if this is a gapic monorepo.
library_generation/generate_repo.py
Outdated
| @@ -18,6 +18,8 @@ | |||
| from library_generation.model.library_config import LibraryConfig | |||
| from library_generation.utils.monorepo_postprocessor import monorepo_postprocessing | |||
|
|
|||
| PROTO_ONLY_LIBRARIES = ["common-protos"] | |||
There was a problem hiding this comment.
I believe this one should also be changed to "common proto libraries"
There was a problem hiding this comment.
This variable is actually not used, I removed it.
| @@ -61,6 +63,15 @@ def get_proto_path_to_library_name(self) -> dict[str, str]: | |||
| def is_monorepo(self) -> bool: | |||
| return len(self.libraries) > 1 | |||
|
|
|||
| def contains_common_protos(self) -> bool: | |||
There was a problem hiding this comment.
We have one method to set the value and one to compute, plus we store a variable in the config object. Maybe we can put the logic of the setter in the getter method and compute the value once in the field we create in the constructor. That would leave us with one less method.
| @@ -20,6 +20,7 @@ | |||
| REPO_LEVEL_PARAMETER = "Repo level parameter" | |||
| LIBRARY_LEVEL_PARAMETER = "Library level parameter" | |||
| GAPIC_LEVEL_PARAMETER = "GAPIC level parameter" | |||
| COMMON_PROTOS = "common-protos" | |||
There was a problem hiding this comment.
| COMMON_PROTOS = "common-protos" | |
| COMMON_PROTOS_LIBRARY_NAME = "common-protos" |
library_generation/generate_repo.py
Outdated
| generate_composed_library( | ||
| config_path=config_path, | ||
| config=config, | ||
| library_path=library_path, | ||
| library=library, | ||
| output_folder=repo_config.output_folder, | ||
| versions_file=repo_config.versions_file, | ||
| gapic_repo=not config.contains_common_protos(), |
There was a problem hiding this comment.
Why do we call this gapic_repo? I see that config is already being passed around, can we just use config.contains_common_protos()? I feel it is easier to understand.
There was a problem hiding this comment.
I removed this parameter as we already pass the config.
There was a problem hiding this comment.
Thanks for backfilling this test!
| @@ -250,6 +252,8 @@ def generate_prerequisite_files( | |||
| "library_type": library.library_type, | |||
| "requires_billing": library.requires_billing, | |||
| } | |||
| if config.contains_common_protos(): | |||
There was a problem hiding this comment.
nit: Can we add a comment saying that this is for java-common-protos and java-iam modules in sdk-platform-java? And/or change the logic to if repo == "googleapis/sdk-platform-java"?
| if config.is_monorepo() | ||
| else f"googleapis/{language}-{library_name}" | ||
| ) | ||
| if config.contains_common_protos(): |
There was a problem hiding this comment.
Future consideration: Maybe we want to introduce a repo level config called repo or repo_name to represent this.
|
|
In this PR: - Enable hermetic build script to generate `java-common-protos` and `java-iam` in this repo. - Change `libraries_bom_version` to optional. - Refactor unit tests. - Remove `java-common-protos/codecov.yaml` and `java-iam/codecov.yaml` as they are no longer needed.
In this PR:
java-common-protosandjava-iamin this repo.libraries_bom_versionto optional.java-common-protos/codecov.yamlandjava-iam/codecov.yamlas they are no longer needed.