feat: add a CLI tool to validate generation configuration #2691

JoeWang1127 · 2024-04-25T19:37:27Z

In this PR:

Add a CLI tool to validate generation config.

Downstream libraries, e.g., google-cloud-java, can write a workflow job like:

docker run ...
 python /src/cli/entry_point.py \
 validate-generation-config \
 --generation-config-path=path/to/generation_config.yaml

JoeWang1127 · 2024-04-25T20:18:46Z

library_generation/utils/utilities.py

@@ -130,10 +130,6 @@ def prepare_repo(
        # use absolute path because docker requires absolute path
        # in volume name.
        absolute_library_path = str(Path(library_path).resolve())
-        if absolute_library_path in libraries:


We don't need this check because the Generation object is validated when reaches this point.

blakeli0 · 2024-04-25T21:21:39Z

library_generation/model/generation_config.py

+            library_name = library.get_library_name()
+            if library_name in seen_library_names:
+                raise ValueError(
+                    f"{library.name_pretty} has the same library name with "


Can we make the error message more verbose and actionable? Something like

f"Both {library.name_pretty} and {seen_library_names.get(library_name)} have the same library_name: {library_name}, please update one of the library to have a different library_name."

Another edge case is that two libraries may have the same name_pretty, but I think the error message is already good enough, so we may not have to consider it.

blakeli0 · 2024-04-25T21:26:32Z

library_generation/model/generation_config.py

@@ -130,7 +142,9 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig:

 def __required(config: Dict, key: str):
    if key not in config:
-        raise ValueError(f"required key {key} not found in yaml")
+        raise ValueError(
+            f"required key {key} not found in {config} " f"when parsing yaml"


Is {config} going to print out the whole config? If yes, I don't think we want to do that. Something like

f"Required repo level config {key} not found"

should be good enough.

Separately, there are a few library level required field, and I don't see any validation for them yet. They can be addressed in a separate PR.

I remove config in the err message.

Separately, there are a few library level required field, and I don't see any validation for them yet. They can be addressed in a separate PR.

Library level required fields are tests in utilities_unit_tests.py. I moved it to generation_config_unit_test.py.

I see that library level config is validated in the same way as repo level config now. However, it makes the error message not clear for library level config. e.g. If api-shortname is missing for a specific library, we don't know which library is missing api-shortname from required key {key} not found.

blakeli0 · 2024-04-25T21:30:10Z

How do we plan to use it? I think we can add it to the CI as a required check for every PR, as this should be a very lightweight check.

JoeWang1127 · 2024-04-25T23:23:40Z

How do we plan to use it? I think we can add it to the CI as a required check for every PR, as this should be a very lightweight check.

Yes, the check should be finished quickly. I tested with generation_config.yaml in google-cloud-java and it finished in 0.3s:

(.venv) joewa-macbookpro:sdk-platform-java joewa$ time python library_generation/cli/entry_point.py validate-generation-config --generation-config-path=../google-cloud-java/generation_config.yaml

real    0m0.299s
user    0m0.161s
sys     0m0.052s

I added how to use the check in the description.

blakeli0

Can the command just be

docker run ...
 python /src/cli/entry_point.py validate-generation-config

without specifying the yaml location? I think it should be fine since we have a default location?

blakeli0 · 2024-04-26T17:38:55Z

library_generation/test/model/generation_config_unit_test.py

+    # be raised.
+    @parameterized.expand(
+        [
+            ("libraries", f"{test_config_dir}/config_without_libraries.yaml"),


I'm still not a fan of parameterized tests, but I know this test was pre-existing, and in this case, the name of test yamls are kind of self-explanatory, so it's fine.

I replaced this parameterized test to several individual tests.

blakeli0 · 2024-04-26T17:51:25Z

library_generation/model/generation_config.py

@@ -130,7 +142,9 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig:

 def __required(config: Dict, key: str):
    if key not in config:
-        raise ValueError(f"required key {key} not found in yaml")
+        raise ValueError(
+            f"required key {key} not found in {config} " f"when parsing yaml"


I see that library level config is validated in the same way as repo level config now. However, it makes the error message not clear for library level config. e.g. If api-shortname is missing for a specific library, we don't know which library is missing api-shortname from required key {key} not found.

JoeWang1127 · 2024-04-26T18:00:37Z

Can the command just be
docker run ...
 python /src/cli/entry_point.py validate-generation-config
without specifying the yaml location? I think it should be fine since we have a default location?

Yes, if the yaml location is not specified, the default value is generation_config.yaml in the current working directory.

JoeWang1127 · 2024-04-26T18:32:55Z

I see that library level config is validated in the same way as repo level config now. However, it makes the error message not clear for library level config. e.g. If api-shortname is missing for a specific library, we don't know which library is missing api-shortname from required key {key} not found.

I added a parameter, level, so that library config is shown in the raised exception. Missing a repo level parameter will not print the whole config (existing behavior).

blakeli0 · 2024-04-26T19:00:30Z

I see that library level config is validated in the same way as repo level config now. However, it makes the error message not clear for library level config. e.g. If api-shortname is missing for a specific library, we don't know which library is missing api-shortname from required key {key} not found.

I added a parameter, level, so that library config is shown in the raised exception. Missing a repo level parameter will not print the whole config (existing behavior).

The main concern is not that we don't know if the config is at library or repo level, the concern is that we don't know which library is missing a config if it's a library level config.

JoeWang1127 · 2024-04-26T19:21:03Z

The main concern is not that we don't know if the config is at library or repo level, the concern is that we don't know which library is missing a config if it's a library level config.

When parsing from yaml, we don't know which parameter maybe missing so I think the best we can do is printing the library dict and hopefully some of the values can let us find which library is missing parameter.

library_generation/model/generation_config.py

blakeli0

LGTM other than one minor issue for unit test. Separately I think we should error out on unknown parameters, which I don't think we are doing currently.

JoeWang1127 · 2024-04-26T22:04:35Z

Separately I think we should error out on unknown parameters, which I don't think we are doing currently.

I'll address this issue in a follow-up PR.

sonarcloud · 2024-04-26T22:10:06Z

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

sonarcloud · 2024-04-26T22:10:06Z

Please retry analysis of this Pull-Request directly on SonarCloud

sonarcloud · 2024-04-26T22:13:16Z

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

🤖 I have created a release *beep* *boop* --- <details><summary>2.40.0</summary> ## [2.40.0](v2.39.0...v2.40.0) (2024-05-02) ### Features * [common-protos] add `Weight` to common types for Shopping APIs to be used for accounts bundle ([#2699](#2699)) ([5bb9770](5bb9770)) * add a CLI tool to validate generation configuration ([#2691](#2691)) ([f2ce524](f2ce524)) * Parser to consume the api-versioning value from proto ([#2630](#2630)) ([40711fd](40711fd)) * Update Gapic generator and Gax to emit api-versioning via header ([#2671](#2671)) ([e63d1b4](e63d1b4)) ### Bug Fixes * change folder prefix for adding headers ([#2688](#2688)) ([4e92be8](4e92be8)) * Log HttpJson's async thread pool core size ([#2697](#2697)) ([34b4bc3](34b4bc3)) * replace `cfg = "host"` with `cfg = "exec"` ([#2637](#2637)) ([6d673f3](6d673f3)) * Return resolved endpoint from StubSettings' Builder ([#2715](#2715)) ([32c9995](32c9995)) ### Dependencies * Make opentelemetry-api an optional dependency. ([#2681](#2681)) ([3967a19](3967a19)) * update dependency absl-py to v2.1.0 ([#2659](#2659)) ([cae6d79](cae6d79)) * update dependency gitpython to v3.1.43 ([#2656](#2656)) ([208bef4](208bef4)) * update dependency lxml to v5.2.1 ([#2661](#2661)) ([b95ad49](b95ad49)) * update dependency net.bytebuddy:byte-buddy to v1.14.14 ([#2703](#2703)) ([87069bc](87069bc)) * update dependency typing to v3.10.0.0 ([#2663](#2663)) ([7fb5653](7fb5653)) * update gapic-showcase to v0.33.0 ([#2653](#2653)) ([0a71cbf](0a71cbf)) ### Documentation * Add contributing guidelines to PR and issue templates ([#2682](#2682)) ([42526dc](42526dc)) </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>

In this PR: - Add a CLI tool to validate generation config. Downstream libraries, e.g., google-cloud-java, can write a workflow job like: ``` docker run ... python /src/cli/entry_point.py \ validate-generation-config \ --generation-config-path=path/to/generation_config.yaml ```

🤖 I have created a release *beep* *boop* --- <details><summary>2.40.0</summary> ## [2.40.0](v2.39.0...v2.40.0) (2024-05-02) ### Features * [common-protos] add `Weight` to common types for Shopping APIs to be used for accounts bundle ([#2699](#2699)) ([5bb9770](5bb9770)) * add a CLI tool to validate generation configuration ([#2691](#2691)) ([f2ce524](f2ce524)) * Parser to consume the api-versioning value from proto ([#2630](#2630)) ([40711fd](40711fd)) * Update Gapic generator and Gax to emit api-versioning via header ([#2671](#2671)) ([e63d1b4](e63d1b4)) ### Bug Fixes * change folder prefix for adding headers ([#2688](#2688)) ([4e92be8](4e92be8)) * Log HttpJson's async thread pool core size ([#2697](#2697)) ([34b4bc3](34b4bc3)) * replace `cfg = "host"` with `cfg = "exec"` ([#2637](#2637)) ([6d673f3](6d673f3)) * Return resolved endpoint from StubSettings' Builder ([#2715](#2715)) ([32c9995](32c9995)) ### Dependencies * Make opentelemetry-api an optional dependency. ([#2681](#2681)) ([3967a19](3967a19)) * update dependency absl-py to v2.1.0 ([#2659](#2659)) ([cae6d79](cae6d79)) * update dependency gitpython to v3.1.43 ([#2656](#2656)) ([208bef4](208bef4)) * update dependency lxml to v5.2.1 ([#2661](#2661)) ([b95ad49](b95ad49)) * update dependency net.bytebuddy:byte-buddy to v1.14.14 ([#2703](#2703)) ([87069bc](87069bc)) * update dependency typing to v3.10.0.0 ([#2663](#2663)) ([7fb5653](7fb5653)) * update gapic-showcase to v0.33.0 ([#2653](#2653)) ([0a71cbf](0a71cbf)) ### Documentation * Add contributing guidelines to PR and issue templates ([#2682](#2682)) ([42526dc](42526dc)) </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>

JoeWang1127 added 2 commits April 25, 2024 15:36

feat: validate generation config

26d4888

add unit tests

58c05eb

product-auto-label bot added the size: m Pull request size is medium. label Apr 25, 2024

fix unit tests

8130ead

JoeWang1127 commented Apr 25, 2024

View reviewed changes

JoeWang1127 marked this pull request as ready for review April 25, 2024 20:20

JoeWang1127 requested a review from a team as a code owner April 25, 2024 20:20

JoeWang1127 requested a review from blakeli0 April 25, 2024 20:20

JoeWang1127 changed the title ~~feat: add config check~~ feat: add a generation config validation CLI tool Apr 25, 2024

JoeWang1127 changed the title ~~feat: add a generation config validation CLI tool~~ feat: add a CLI tool to validate generation configuration Apr 25, 2024

blakeli0 reviewed Apr 25, 2024

View reviewed changes

JoeWang1127 added 2 commits April 25, 2024 19:07

change err message

a3652b3

refactor unit tests

8b0b8e1

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 25, 2024

JoeWang1127 requested a review from blakeli0 April 25, 2024 23:26

JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 26, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 26, 2024

JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 26, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 26, 2024

Merge branch 'main' into feat/add-config-check

1674469

JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 26, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 26, 2024

blakeli0 reviewed Apr 26, 2024

View reviewed changes

specify level

8aac838

add gapic level

f2dc50f

blakeli0 reviewed Apr 26, 2024

View reviewed changes

library_generation/model/generation_config.py Show resolved Hide resolved

blakeli0 approved these changes Apr 26, 2024

View reviewed changes

JoeWang1127 added 3 commits April 26, 2024 17:13

test library dict in error message

1cd4b95

refactor

8dca4ed

Merge branch 'main' into feat/add-config-check

bd53ac4

JoeWang1127 enabled auto-merge (squash) April 26, 2024 22:14

JoeWang1127 merged commit f2ce524 into main Apr 26, 2024
29 checks passed

JoeWang1127 deleted the feat/add-config-check branch April 26, 2024 22:46

release-please bot mentioned this pull request Apr 26, 2024

chore(main): release 2.40.0 #2679

Merged

JoeWang1127 mentioned this pull request Apr 28, 2024

chore: bake synthtool and owl-bot into library_generation docker image #2615

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add a CLI tool to validate generation configuration #2691

feat: add a CLI tool to validate generation configuration #2691

JoeWang1127 commented Apr 25, 2024 •

edited

Loading

JoeWang1127 Apr 25, 2024

blakeli0 Apr 25, 2024

blakeli0 Apr 25, 2024

JoeWang1127 Apr 25, 2024

blakeli0 Apr 25, 2024

blakeli0 Apr 25, 2024

JoeWang1127 Apr 25, 2024

blakeli0 Apr 26, 2024

blakeli0 commented Apr 25, 2024

JoeWang1127 commented Apr 25, 2024 •

edited

Loading

blakeli0 left a comment

blakeli0 Apr 26, 2024

JoeWang1127 Apr 26, 2024

blakeli0 Apr 26, 2024

JoeWang1127 commented Apr 26, 2024

JoeWang1127 commented Apr 26, 2024

blakeli0 commented Apr 26, 2024

JoeWang1127 commented Apr 26, 2024

blakeli0 left a comment

JoeWang1127 commented Apr 26, 2024

sonarcloud bot commented Apr 26, 2024

sonarcloud bot commented Apr 26, 2024

sonarcloud bot commented Apr 26, 2024

feat: add a CLI tool to validate generation configuration #2691

feat: add a CLI tool to validate generation configuration #2691

Conversation

JoeWang1127 commented Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 commented Apr 25, 2024

JoeWang1127 commented Apr 25, 2024 • edited Loading

blakeli0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeWang1127 commented Apr 26, 2024

JoeWang1127 commented Apr 26, 2024

blakeli0 commented Apr 26, 2024

JoeWang1127 commented Apr 26, 2024

blakeli0 left a comment

Choose a reason for hiding this comment

JoeWang1127 commented Apr 26, 2024

sonarcloud bot commented Apr 26, 2024

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

sonarcloud bot commented Apr 26, 2024

sonarcloud bot commented Apr 26, 2024

Quality Gate passed for 'java_showcase_integration_tests'

JoeWang1127 commented Apr 25, 2024 •

edited

Loading

JoeWang1127 commented Apr 25, 2024 •

edited

Loading