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.

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: generate proto-only repository #2720

Merged
merged 61 commits into from
May 16, 2024
Merged

Conversation

JoeWang1127
Copy link
Collaborator

@JoeWang1127 JoeWang1127 commented May 2, 2024

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.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label May 2, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels May 2, 2024
@JoeWang1127 JoeWang1127 changed the title chore: add generation config feat: generate proto-only repository May 8, 2024
@blakeli0
Copy link
Collaborator

blakeli0 commented May 9, 2024

java-iam/README.md is excluded from templating, should we not generate it?

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 ?

)

# we skip monorepo_postprocessing if not in a monorepo
if not config.is_monorepo():
if not config.is_monorepo() or config.contains_common_protos():
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only perform monorepo post processing if this is a gapic monorepo.

Copy link
Contributor

@diegomarquezp diegomarquezp left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments.

@@ -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"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this one should also be changed to "common proto libraries"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:
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 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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
COMMON_PROTOS = "common-protos"
COMMON_PROTOS_LIBRARY_NAME = "common-protos"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

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(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed this parameter as we already pass the config.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for backfilling this test!

@JoeWang1127 JoeWang1127 requested a review from blakeli0 May 16, 2024 12:55
@@ -250,6 +252,8 @@ def generate_prerequisite_files(
"library_type": library.library_type,
"requires_billing": library.requires_billing,
}
if config.contains_common_protos():
Copy link
Collaborator

Choose a reason for hiding this comment

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

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"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

if config.is_monorepo()
else f"googleapis/{language}-{library_name}"
)
if config.contains_common_protos():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Future consideration: Maybe we want to introduce a repo level config called repo or repo_name to represent this.

Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link

sonarcloud bot commented May 16, 2024

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented May 16, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@JoeWang1127 JoeWang1127 merged commit f7a5161 into main May 16, 2024
48 checks passed
@JoeWang1127 JoeWang1127 deleted the chore/add-generation-config branch May 16, 2024 21:52
lqiu96 pushed a commit that referenced this pull request May 22, 2024
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.
diegomarquezp pushed a commit that referenced this pull request May 31, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>2.41.0</summary>

##
[2.41.0](v2.40.1...v2.41.0)
(2024-05-31)


### Features

* add a github client
([#2747](#2747))
([f8ea0a0](f8ea0a0))
* generate proto-only repository
([#2720](#2720))
([f7a5161](f7a5161))


### Bug Fixes

* [#2785](#2785).
([599f5da](599f5da))


### Dependencies

* update dependency com.google.api-client:google-api-client-bom to
v2.6.0
([#2782](#2782))
([5bc8928](5bc8928))
* update dependency
com.google.cloud.opentelemetry:detector-resources-support to v0.29.0
([#2831](#2831))
([6c1dbfc](6c1dbfc))
* update dependency com.google.code.gson:gson to v2.11.0
([#2786](#2786))
([91f3254](91f3254))
* update dependency com.google.code.gson:gson to v2.11.0
([#2787](#2787))
([e81893c](e81893c))
* update dependency com.google.errorprone:error_prone_annotations to
v2.28.0
([#2835](#2835))
([b8f11b1](b8f11b1))
* update dependency com.google.errorprone:error_prone_annotations to
v2.28.0
([#2838](#2838))
([5c46f3f](5c46f3f))
* update dependency net.bytebuddy:byte-buddy to v1.14.16
([#2797](#2797))
([dfedafc](dfedafc))
* update dependency net.bytebuddy:byte-buddy to v1.14.17
([#2828](#2828))
([6eb9041](6eb9041))
* update dependency org.checkerframework:checker-qual to v3.43.0
([#2730](#2730))
([b7fa736](b7fa736))
* update dependency requests to v2.32.0 [security]
([#2791](#2791))
([c2ea6cc](c2ea6cc))
* update dependency watchdog to v4.0.1
([#2800](#2800))
([d5771dd](d5771dd))
* update google api dependencies
([#2672](#2672))
([6643536](6643536))
* update google http client dependencies to v1.44.2
([#2783](#2783))
([dee7e00](dee7e00))
* update googleapis/java-cloud-bom digest to 59c776b
([#2827](#2827))
([03b3eb4](03b3eb4))
* update netty dependencies to v4.1.110.final
([#2796](#2796))
([d1aaa68](d1aaa68))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
lqiu96 pushed a commit that referenced this pull request Jun 10, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>2.41.0</summary>

##
[2.41.0](v2.40.1...v2.41.0)
(2024-05-31)


### Features

* add a github client
([#2747](#2747))
([f8ea0a0](f8ea0a0))
* generate proto-only repository
([#2720](#2720))
([f7a5161](f7a5161))


### Bug Fixes

* [#2785](#2785).
([599f5da](599f5da))


### Dependencies

* update dependency com.google.api-client:google-api-client-bom to
v2.6.0
([#2782](#2782))
([5bc8928](5bc8928))
* update dependency
com.google.cloud.opentelemetry:detector-resources-support to v0.29.0
([#2831](#2831))
([6c1dbfc](6c1dbfc))
* update dependency com.google.code.gson:gson to v2.11.0
([#2786](#2786))
([91f3254](91f3254))
* update dependency com.google.code.gson:gson to v2.11.0
([#2787](#2787))
([e81893c](e81893c))
* update dependency com.google.errorprone:error_prone_annotations to
v2.28.0
([#2835](#2835))
([b8f11b1](b8f11b1))
* update dependency com.google.errorprone:error_prone_annotations to
v2.28.0
([#2838](#2838))
([5c46f3f](5c46f3f))
* update dependency net.bytebuddy:byte-buddy to v1.14.16
([#2797](#2797))
([dfedafc](dfedafc))
* update dependency net.bytebuddy:byte-buddy to v1.14.17
([#2828](#2828))
([6eb9041](6eb9041))
* update dependency org.checkerframework:checker-qual to v3.43.0
([#2730](#2730))
([b7fa736](b7fa736))
* update dependency requests to v2.32.0 [security]
([#2791](#2791))
([c2ea6cc](c2ea6cc))
* update dependency watchdog to v4.0.1
([#2800](#2800))
([d5771dd](d5771dd))
* update google api dependencies
([#2672](#2672))
([6643536](6643536))
* update google http client dependencies to v1.44.2
([#2783](#2783))
([dee7e00](dee7e00))
* update googleapis/java-cloud-bom digest to 59c776b
([#2827](#2827))
([03b3eb4](03b3eb4))
* update netty dependencies to v4.1.110.final
([#2796](#2796))
([d1aaa68](d1aaa68))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants