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

JoeWang1127 · web-flow · commit f2ce52478a03 · 2024-04-26T22:46:23.000Z
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
```
diff --git a/library_generation/cli/entry_point.py b/library_generation/cli/entry_point.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import os
+import sys
 
 import click as click
 from library_generation.generate_pr_description import generate_pr_descriptions
@@ -142,5 +143,28 @@ def generate(
     )
 
 
+@main.command()
+@click.option(
+    "--generation-config-path",
+    required=False,
+    type=str,
+    help="""
+    Absolute or relative path to a generation_config.yaml.
+    Default to generation_config.yaml in the current working directory.
+    """,
+)
+def validate_generation_config(generation_config_path: str) -> None:
+    """
+    Validate the given generation configuration.
+    """
+    if generation_config_path is None:
+        generation_config_path = "generation_config.yaml"
+    try:
+        from_yaml(os.path.abspath(generation_config_path))
+    except ValueError as err:
+        print(err)
+        sys.exit(1)
+
+
 if __name__ == "__main__":
     main()
diff --git a/library_generation/model/generation_config.py b/library_generation/model/generation_config.py
@@ -12,13 +12,15 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-
-
 import yaml
-from typing import List, Optional, Dict
+from typing import Optional
 from library_generation.model.library_config import LibraryConfig
 from library_generation.model.gapic_config import GapicConfig
 
+REPO_LEVEL_PARAMETER = "Repo level parameter"
+LIBRARY_LEVEL_PARAMETER = "Library level parameter"
+GAPIC_LEVEL_PARAMETER = "GAPIC level parameter"
+
 
 class GenerationConfig:
     """
@@ -32,8 +34,8 @@ def __init__(
         libraries_bom_version: str,
         owlbot_cli_image: str,
         synthtool_commitish: str,
-        template_excludes: List[str],
-        libraries: List[LibraryConfig],
+        template_excludes: list[str],
+        libraries: list[LibraryConfig],
         grpc_version: Optional[str] = None,
         protoc_version: Optional[str] = None,
     ):
@@ -46,6 +48,7 @@ def __init__(
         self.libraries = libraries
         self.grpc_version = grpc_version
         self.protoc_version = protoc_version
+        self.__validate()
 
     def get_proto_path_to_library_name(self) -> dict[str, str]:
         """
@@ -62,6 +65,19 @@ def get_proto_path_to_library_name(self) -> dict[str, str]:
     def is_monorepo(self) -> bool:
         return len(self.libraries) > 1
 
+    def __validate(self) -> None:
+        seen_library_names = dict()
+        for library in self.libraries:
+            library_name = library.get_library_name()
+            if library_name in seen_library_names:
+                raise ValueError(
+                    f"Both {library.name_pretty} and "
+                    f"{seen_library_names.get(library_name)} have the same "
+                    f"library name: {library_name}, please update one of the "
+                    f"library to have a different library name."
+                )
+            seen_library_names[library_name] = library.name_pretty
+
 
 def from_yaml(path_to_yaml: str) -> GenerationConfig:
     """
@@ -72,15 +88,19 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig:
     with open(path_to_yaml, "r") as file_stream:
         config = yaml.safe_load(file_stream)
 
-    libraries = __required(config, "libraries")
+    libraries = __required(config, "libraries", REPO_LEVEL_PARAMETER)
+    if not libraries:
+        raise ValueError(f"Library is None in {path_to_yaml}.")
 
     parsed_libraries = list()
     for library in libraries:
         gapics = __required(library, "GAPICs")
 
         parsed_gapics = list()
+        if not gapics:
+            raise ValueError(f"GAPICs is None in {library}.")
         for gapic in gapics:
-            proto_path = __required(gapic, "proto_path")
+            proto_path = __required(gapic, "proto_path", GAPIC_LEVEL_PARAMETER)
             new_gapic = GapicConfig(proto_path)
             parsed_gapics.append(new_gapic)
 
@@ -114,27 +134,40 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig:
         parsed_libraries.append(new_library)
 
     parsed_config = GenerationConfig(
-        gapic_generator_version=__required(config, "gapic_generator_version"),
+        gapic_generator_version=__required(
+            config, "gapic_generator_version", REPO_LEVEL_PARAMETER
+        ),
         grpc_version=__optional(config, "grpc_version", None),
         protoc_version=__optional(config, "protoc_version", None),
-        googleapis_commitish=__required(config, "googleapis_commitish"),
-        libraries_bom_version=__required(config, "libraries_bom_version"),
-        owlbot_cli_image=__required(config, "owlbot_cli_image"),
-        synthtool_commitish=__required(config, "synthtool_commitish"),
-        template_excludes=__required(config, "template_excludes"),
+        googleapis_commitish=__required(
+            config, "googleapis_commitish", REPO_LEVEL_PARAMETER
+        ),
+        libraries_bom_version=__required(
+            config, "libraries_bom_version", REPO_LEVEL_PARAMETER
+        ),
+        owlbot_cli_image=__required(config, "owlbot_cli_image", REPO_LEVEL_PARAMETER),
+        synthtool_commitish=__required(
+            config, "synthtool_commitish", REPO_LEVEL_PARAMETER
+        ),
+        template_excludes=__required(config, "template_excludes", REPO_LEVEL_PARAMETER),
         libraries=parsed_libraries,
     )
 
     return parsed_config
 
 
-def __required(config: Dict, key: str):
+def __required(config: dict, key: str, level: str = LIBRARY_LEVEL_PARAMETER):
     if key not in config:
-        raise ValueError(f"required key {key} not found in yaml")
+        message = (
+            f"{level}, {key}, is not found in {config} in yaml."
+            if level != REPO_LEVEL_PARAMETER
+            else f"{level}, {key}, is not found in yaml."
+        )
+        raise ValueError(message)
     return config[key]
 
 
-def __optional(config: Dict, key: str, default: any):
+def __optional(config: dict, key: str, default: any):
     if key not in config:
         return default
     return config[key]
diff --git a/library_generation/test/cli/entry_point_unit_tests.py b/library_generation/test/cli/entry_point_unit_tests.py
@@ -11,9 +11,13 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import os
 import unittest
 from click.testing import CliRunner
-from library_generation.cli.entry_point import generate
+from library_generation.cli.entry_point import generate, validate_generation_config
+
+script_dir = os.path.dirname(os.path.realpath(__file__))
+test_resource_dir = os.path.join(script_dir, "..", "resources", "test-config")
 
 
 class EntryPointTest(unittest.TestCase):
@@ -44,3 +48,32 @@ def test_entry_point_with_baseline_without_current_raise_file_exception(self):
             "current_generation_config is not specified when "
             "baseline_generation_config is specified.",
         )
+
+    def test_validate_generation_config_succeeds(
+        self,
+    ):
+        runner = CliRunner()
+        # noinspection PyTypeChecker
+        result = runner.invoke(
+            validate_generation_config,
+            [f"--generation-config-path={test_resource_dir}/generation_config.yaml"],
+        )
+        self.assertEqual(0, result.exit_code)
+
+    def test_validate_generation_config_with_duplicate_library_name_raise_file_exception(
+        self,
+    ):
+        runner = CliRunner()
+        # noinspection PyTypeChecker
+        result = runner.invoke(
+            validate_generation_config,
+            [
+                f"--generation-config-path={test_resource_dir}/generation_config_with_duplicate_library_name.yaml"
+            ],
+        )
+        self.assertEqual(1, result.exit_code)
+        self.assertEqual(SystemExit, result.exc_info[0])
+        self.assertRegex(
+            result.output,
+            "have the same library name",
+        )
diff --git a/library_generation/test/model/generation_config_unit_test.py b/library_generation/test/model/generation_config_unit_test.py
@@ -133,3 +133,153 @@ def test_is_monorepo_with_two_libraries_returns_true(self):
             libraries=[library_1, library_2],
         )
         self.assertTrue(config.is_monorepo())
+
+    def test_validate_with_duplicate_library_name_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "the same library name",
+            GenerationConfig,
+            gapic_generator_version="",
+            googleapis_commitish="",
+            libraries_bom_version="",
+            owlbot_cli_image="",
+            synthtool_commitish="",
+            template_excludes=[],
+            libraries=[
+                LibraryConfig(
+                    api_shortname="secretmanager",
+                    name_pretty="Secret API",
+                    product_documentation="",
+                    api_description="",
+                    gapic_configs=list(),
+                ),
+                LibraryConfig(
+                    api_shortname="another-secret",
+                    name_pretty="Another Secret API",
+                    product_documentation="",
+                    api_description="",
+                    gapic_configs=list(),
+                    library_name="secretmanager",
+                ),
+            ],
+        )
+
+    def test_from_yaml_without_gapic_generator_version_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Repo level parameter, gapic_generator_version",
+            from_yaml,
+            f"{test_config_dir}/config_without_generator.yaml",
+        )
+
+    def test_from_yaml_without_googleapis_commitish_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Repo level parameter, googleapis_commitish",
+            from_yaml,
+            f"{test_config_dir}/config_without_googleapis.yaml",
+        )
+
+    def test_from_yaml_without_libraries_bom_version_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Repo level parameter, libraries_bom_version",
+            from_yaml,
+            f"{test_config_dir}/config_without_libraries_bom_version.yaml",
+        )
+
+    def test_from_yaml_without_owlbot_cli_image_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Repo level parameter, owlbot_cli_image",
+            from_yaml,
+            f"{test_config_dir}/config_without_owlbot.yaml",
+        )
+
+    def test_from_yaml_without_synthtool_commitish_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Repo level parameter, synthtool_commitish",
+            from_yaml,
+            f"{test_config_dir}/config_without_synthtool.yaml",
+        )
+
+    def test_from_yaml_without_template_excludes_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Repo level parameter, template_excludes",
+            from_yaml,
+            f"{test_config_dir}/config_without_temp_excludes.yaml",
+        )
+
+    def test_from_yaml_without_libraries_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Repo level parameter, libraries",
+            from_yaml,
+            f"{test_config_dir}/config_without_libraries.yaml",
+        )
+
+    def test_from_yaml_without_api_shortname_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Library level parameter, api_shortname",
+            from_yaml,
+            f"{test_config_dir}/config_without_api_shortname.yaml",
+        )
+
+    def test_from_yaml_without_api_description_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            r"Library level parameter, api_description.*'api_shortname': 'apigeeconnect'.*",
+            from_yaml,
+            f"{test_config_dir}/config_without_api_description.yaml",
+        )
+
+    def test_from_yaml_without_name_pretty_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            r"Library level parameter, name_pretty.*'api_shortname': 'apigeeconnect'.*",
+            from_yaml,
+            f"{test_config_dir}/config_without_name_pretty.yaml",
+        )
+
+    def test_from_yaml_without_product_documentation_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            r"Library level parameter, product_documentation.*'api_shortname': 'apigeeconnect'.*",
+            from_yaml,
+            f"{test_config_dir}/config_without_product_docs.yaml",
+        )
+
+    def test_from_yaml_without_gapics_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Library level parameter, GAPICs.*'api_shortname': 'apigeeconnect'.*",
+            from_yaml,
+            f"{test_config_dir}/config_without_gapics_key.yaml",
+        )
+
+    def test_from_yaml_without_proto_path_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "GAPIC level parameter, proto_path",
+            from_yaml,
+            f"{test_config_dir}/config_without_proto_path.yaml",
+        )
+
+    def test_from_yaml_with_zero_library_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            "Library is None",
+            from_yaml,
+            f"{test_config_dir}/config_without_library_value.yaml",
+        )
+
+    def test_from_yaml_with_zero_proto_path_raise_exception(self):
+        self.assertRaisesRegex(
+            ValueError,
+            r"GAPICs is None in.*'api_shortname': 'apigeeconnect'.*",
+            from_yaml,
+            f"{test_config_dir}/config_without_gapics_value.yaml",
+        )
diff --git a/library_generation/test/resources/test-config/config_without_gapics_key.yaml b/library_generation/test/resources/test-config/config_without_gapics_key.yaml
@@ -0,0 +1,3 @@
+gapic_generator_version: 2.34.0
+libraries:
+  - api_shortname: apigeeconnect
diff --git a/library_generation/test/resources/test-config/config_without_gapics_value.yaml b/library_generation/test/resources/test-config/config_without_gapics_value.yaml
@@ -0,0 +1,4 @@
+gapic_generator_version: 2.34.0
+libraries:
+  - api_shortname: apigeeconnect
+    GAPICs:
diff --git a/library_generation/test/resources/test-config/config_without_library_value.yaml b/library_generation/test/resources/test-config/config_without_library_value.yaml
@@ -1,3 +1,2 @@
 gapic_generator_version: 2.34.0
 libraries:
-  random_key:
diff --git a/library_generation/test/resources/test-config/generation_config_with_duplicate_library_name.yaml b/library_generation/test/resources/test-config/generation_config_with_duplicate_library_name.yaml
diff --git a/library_generation/test/utilities_unit_tests.py b/library_generation/test/utilities_unit_tests.py
diff --git a/library_generation/utils/utilities.py b/library_generation/utils/utilities.py

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+gapic_generator_version: 2.34.0`
	`2`	`+libraries:`
	`3`	`+ - api_shortname: apigeeconnect`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,2 @@`
`1`	`1`	`gapic_generator_version: 2.34.0`
`2`	`2`	`libraries:`
`3`		`- random_key:`