feat: add a CLI tool to validate generation configuration by JoeWang1127 · Pull Request #2691 · googleapis/sdk-platform-java · GitHub

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

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

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.

Done.

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.

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.

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.

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?

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.

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.

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.

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).

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.

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.

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.

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.

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

Please retry analysis of this Pull-Request directly on SonarCloud

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 review from a team and 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

Comment thread library_generation/model/generation_config.py

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

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

                       # 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:

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

                   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"

                   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"

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

feat: add a CLI tool to validate generation configuration by JoeWang1127 · Pull Request #2691 · googleapis/sdk-platform-java · GitHub

Uh oh!

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

feat: add a CLI tool to validate generation configuration #2691JoeWang1127 merged 11 commits intomaingoogleapis/sdk-platform-java:mainfrom feat/add-config-checkgoogleapis/sdk-platform-java:feat/add-config-checkCopy head branch name to clipboard

Conversation

JoeWang1127 commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blakeli0 commented Apr 25, 2024

Uh oh!

JoeWang1127 commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blakeli0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeWang1127 commented Apr 26, 2024

Uh oh!

JoeWang1127 commented Apr 26, 2024

Uh oh!

blakeli0 commented Apr 26, 2024

Uh oh!

JoeWang1127 commented Apr 26, 2024

Uh oh!

Uh oh!

blakeli0 left a comment

Choose a reason for hiding this comment

Uh oh!

JoeWang1127 commented Apr 26, 2024

Uh oh!

sonarqubecloud Bot commented Apr 26, 2024

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

Uh oh!

sonarqubecloud Bot commented Apr 26, 2024

Uh oh!

sonarqubecloud Bot commented Apr 26, 2024

Quality Gate passed for 'java_showcase_integration_tests'

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feat: add a CLI tool to validate generation configuration #2691
JoeWang1127 merged 11 commits into
mainfrom
feat/add-config-check

JoeWang1127 commented Apr 25, 2024 •
edited
Loading

JoeWang1127 commented Apr 25, 2024 •
edited
Loading