From 26d48885b2de586c37994fb6157cf3027c6c049c Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Thu, 25 Apr 2024 15:36:45 -0400 Subject: [PATCH 1/9] feat: validate generation config --- library_generation/cli/entry_point.py | 24 ++++++++++++ library_generation/model/generation_config.py | 16 +++++++- ...on_config_with_duplicate_library_name.yaml | 38 +++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 library_generation/test/resources/test-config/generation_config_with_duplicate_library_name.yaml diff --git a/library_generation/cli/entry_point.py b/library_generation/cli/entry_point.py index ccaf631e85..a8c9c3ee0d 100644 --- 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 index 650f4a45ba..dbc48ecdea 100644 --- a/library_generation/model/generation_config.py +++ b/library_generation/model/generation_config.py @@ -46,6 +46,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 +63,17 @@ 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"{library.name_pretty} has the same library name with " + f"{seen_library_names.get(library_name)}" + ) + seen_library_names[library_name] = library.name_pretty + def from_yaml(path_to_yaml: str) -> GenerationConfig: """ @@ -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" + ) return config[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 new file mode 100644 index 0000000000..8930e982da --- /dev/null +++ b/library_generation/test/resources/test-config/generation_config_with_duplicate_library_name.yaml @@ -0,0 +1,38 @@ +gapic_generator_version: 2.34.0 +protoc_version: 25.2 +googleapis_commitish: 1a45bf7393b52407188c82e63101db7dc9c72026 +libraries_bom_version: 26.37.0 +owlbot_cli_image: sha256:623647ee79ac605858d09e60c1382a716c125fb776f69301b72de1cd35d49409 +synthtool_commitish: 6612ab8f3afcd5e292aecd647f0fa68812c9f5b5 +template_excludes: + - ".github/*" + - ".kokoro/*" + - "samples/*" + - "CODE_OF_CONDUCT.md" + - "CONTRIBUTING.md" + - "LICENSE" + - "SECURITY.md" + - "java.header" + - "license-checks.xml" + - "renovate.json" + - ".gitignore" +libraries: + - api_shortname: cloudasset + name_pretty: Cloud Asset Inventory + product_documentation: "https://cloud.google.com/resource-manager/docs/cloud-asset-inventory/overview" + api_description: "provides inventory services based on a time series database." + library_name: "asset" + client_documentation: "https://cloud.google.com/java/docs/reference/google-cloud-asset/latest/overview" + distribution_name: "com.google.cloud:google-cloud-asset" + release_level: "stable" + issue_tracker: "https://issuetracker.google.com/issues/new?component=187210&template=0" + api_reference: "https://cloud.google.com/resource-manager/docs/cloud-asset-inventory/overview" + codeowner_team: "@googleapis/analytics-dpe" + excluded_poms: proto-google-iam-v1-bom,google-iam-policy,proto-google-iam-v1 + excluded_dependencies: google-iam-policy + GAPICs: + - proto_path: google/cloud/asset/v1 + - proto_path: google/cloud/asset/v1p1beta1 + - proto_path: google/cloud/asset/v1p2beta1 + - proto_path: google/cloud/asset/v1p5beta1 + - proto_path: google/cloud/asset/v1p7beta1 From 58c05eb6451cd9245ca4b58ec1956ed9dbd5e0bb Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Thu, 25 Apr 2024 15:37:03 -0400 Subject: [PATCH 2/9] add unit tests --- .../test/cli/entry_point_unit_tests.py | 33 ++++++++++++++++++- .../test/model/generation_config_unit_test.py | 30 +++++++++++++++++ ...on_config_with_duplicate_library_name.yaml | 15 ++++++++- 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/library_generation/test/cli/entry_point_unit_tests.py b/library_generation/test/cli/entry_point_unit_tests.py index ae168e9671..716bd3193a 100644 --- a/library_generation/test/cli/entry_point_unit_tests.py +++ b/library_generation/test/cli/entry_point_unit_tests.py @@ -13,7 +13,7 @@ # limitations under the License. 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 class EntryPointTest(unittest.TestCase): @@ -44,3 +44,34 @@ 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, + [ + "--generation-config-path=../resources/test-config/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, + [ + "--generation-config-path=../resources/test-config/generation_config_with_duplicate_library_name.yaml" + ], + ) + self.assertEqual(1, result.exit_code) + self.assertEqual(SystemExit, result.exc_info[0]) + self.assertRegex( + result.output.title(), + "Cloud Asset Inventory Has The Same Library Name With Cloud Asset", + ) diff --git a/library_generation/test/model/generation_config_unit_test.py b/library_generation/test/model/generation_config_unit_test.py index fd7608bb4f..9ff4724c63 100644 --- a/library_generation/test/model/generation_config_unit_test.py +++ b/library_generation/test/model/generation_config_unit_test.py @@ -133,3 +133,33 @@ 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", + ), + ], + ) 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 index 8930e982da..c5613f4308 100644 --- 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 @@ -18,7 +18,7 @@ template_excludes: - ".gitignore" libraries: - api_shortname: cloudasset - name_pretty: Cloud Asset Inventory + name_pretty: Cloud Asset product_documentation: "https://cloud.google.com/resource-manager/docs/cloud-asset-inventory/overview" api_description: "provides inventory services based on a time series database." library_name: "asset" @@ -36,3 +36,16 @@ libraries: - proto_path: google/cloud/asset/v1p2beta1 - proto_path: google/cloud/asset/v1p5beta1 - proto_path: google/cloud/asset/v1p7beta1 + + - api_shortname: another-cloudasset + name_pretty: Cloud Asset Inventory + product_documentation: "https://cloud.google.com/resource-manager/docs/cloud-asset-inventory/overview" + api_description: "provides inventory services based on a time series database." + library_name: "asset" + client_documentation: "https://cloud.google.com/java/docs/reference/google-cloud-asset/latest/overview" + distribution_name: "com.google.cloud:google-cloud-asset" + release_level: "stable" + issue_tracker: "https://issuetracker.google.com/issues/new?component=187210&template=0" + api_reference: "https://cloud.google.com/resource-manager/docs/cloud-asset-inventory/overview" + GAPICs: + - proto_path: google/cloud/asset/v1 From 8130ead9e17908db73ce5e978c6b140a1c05c5e8 Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Thu, 25 Apr 2024 16:12:30 -0400 Subject: [PATCH 3/9] fix unit tests --- library_generation/test/cli/entry_point_unit_tests.py | 10 ++++++---- library_generation/test/utilities_unit_tests.py | 11 ----------- library_generation/utils/utilities.py | 4 ---- 3 files changed, 6 insertions(+), 19 deletions(-) diff --git a/library_generation/test/cli/entry_point_unit_tests.py b/library_generation/test/cli/entry_point_unit_tests.py index 716bd3193a..121a6bb7cd 100644 --- a/library_generation/test/cli/entry_point_unit_tests.py +++ b/library_generation/test/cli/entry_point_unit_tests.py @@ -11,10 +11,14 @@ # 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, 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): def test_entry_point_without_config_raise_file_exception(self): @@ -52,9 +56,7 @@ def test_validate_generation_config_succeeds( # noinspection PyTypeChecker result = runner.invoke( validate_generation_config, - [ - "--generation-config-path=../resources/test-config/generation_config.yaml" - ], + [f"--generation-config-path={test_resource_dir}/generation_config.yaml"], ) self.assertEqual(0, result.exit_code) @@ -66,7 +68,7 @@ def test_validate_generation_config_with_duplicate_library_name_raise_file_excep result = runner.invoke( validate_generation_config, [ - "--generation-config-path=../resources/test-config/generation_config_with_duplicate_library_name.yaml" + f"--generation-config-path={test_resource_dir}/generation_config_with_duplicate_library_name.yaml" ], ) self.assertEqual(1, result.exit_code) diff --git a/library_generation/test/utilities_unit_tests.py b/library_generation/test/utilities_unit_tests.py index 5e5869d0c1..9b8f829876 100644 --- a/library_generation/test/utilities_unit_tests.py +++ b/library_generation/test/utilities_unit_tests.py @@ -344,17 +344,6 @@ def test_prepare_repo_monorepo_success(self): ["java-bare-metal-solution", "java-secretmanager"], library_path ) - def test_prepare_repo_monorepo_duplicated_library_name_failed(self): - gen_config = self.__get_a_gen_config(3) - self.assertRaisesRegex( - ValueError, - "secretmanager", - util.prepare_repo, - gen_config, - gen_config.libraries, - f"{resources_dir}/misc", - ) - def test_prepare_repo_monorepo_failed(self): gen_config = self.__get_a_gen_config(2) self.assertRaises( diff --git a/library_generation/utils/utilities.py b/library_generation/utils/utilities.py index 1456ede48f..71c58f0455 100755 --- a/library_generation/utils/utilities.py +++ b/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: - # check whether the java_library is unique among all libraries - # because two libraries should not go to the same destination. - raise ValueError(f"{absolute_library_path} already exists.") libraries[absolute_library_path] = library # remove existing .repo-metadata.json json_name = ".repo-metadata.json" From a3652b3fc6ef054bb163ab7bde6db4429b54af27 Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Thu, 25 Apr 2024 19:07:29 -0400 Subject: [PATCH 4/9] change err message --- library_generation/model/generation_config.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/library_generation/model/generation_config.py b/library_generation/model/generation_config.py index dbc48ecdea..63fde9ab55 100644 --- a/library_generation/model/generation_config.py +++ b/library_generation/model/generation_config.py @@ -69,8 +69,10 @@ def __validate(self) -> None: 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 " - f"{seen_library_names.get(library_name)}" + 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 @@ -142,9 +144,7 @@ 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 {config} " f"when parsing yaml" - ) + raise ValueError(f"required key {key} is not found in yaml.") return config[key] From 8b0b8e138a9887ae61adef34a0bddb3a5e53c4e9 Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Thu, 25 Apr 2024 19:15:03 -0400 Subject: [PATCH 5/9] refactor unit tests --- .../test/cli/entry_point_unit_tests.py | 4 +- .../test/model/generation_config_unit_test.py | 50 +++++++++++++++++++ .../test/utilities_unit_tests.py | 50 ------------------- 3 files changed, 52 insertions(+), 52 deletions(-) diff --git a/library_generation/test/cli/entry_point_unit_tests.py b/library_generation/test/cli/entry_point_unit_tests.py index 121a6bb7cd..ace2794684 100644 --- a/library_generation/test/cli/entry_point_unit_tests.py +++ b/library_generation/test/cli/entry_point_unit_tests.py @@ -74,6 +74,6 @@ def test_validate_generation_config_with_duplicate_library_name_raise_file_excep self.assertEqual(1, result.exit_code) self.assertEqual(SystemExit, result.exc_info[0]) self.assertRegex( - result.output.title(), - "Cloud Asset Inventory Has The Same Library Name With Cloud Asset", + 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 index 9ff4724c63..8f195bed3c 100644 --- a/library_generation/test/model/generation_config_unit_test.py +++ b/library_generation/test/model/generation_config_unit_test.py @@ -14,6 +14,7 @@ import os import unittest from pathlib import Path +from parameterized import parameterized from library_generation.model.generation_config import from_yaml, GenerationConfig from library_generation.model.library_config import LibraryConfig @@ -163,3 +164,52 @@ def test_validate_with_duplicate_library_name_raise_exception(self): ), ], ) + + # parameterized tests need to run from the class, see + # https://github.com/wolever/parameterized/issues/37 for more info. + # This test confirms that a ValueError with an error message about a + # missing key (specified in the first parameter of each `parameterized` + # tuple) when parsing a test configuration yaml (second parameter) will + # be raised. + @parameterized.expand( + [ + ("libraries", f"{test_config_dir}/config_without_libraries.yaml"), + ("GAPICs", f"{test_config_dir}/config_without_gapics.yaml"), + ("proto_path", f"{test_config_dir}/config_without_proto_path.yaml"), + ("api_shortname", f"{test_config_dir}/config_without_api_shortname.yaml"), + ( + "api_description", + f"{test_config_dir}/config_without_api_description.yaml", + ), + ("name_pretty", f"{test_config_dir}/config_without_name_pretty.yaml"), + ( + "product_documentation", + f"{test_config_dir}/config_without_product_docs.yaml", + ), + ( + "gapic_generator_version", + f"{test_config_dir}/config_without_generator.yaml", + ), + ( + "googleapis_commitish", + f"{test_config_dir}/config_without_googleapis.yaml", + ), + ( + "libraries_bom_version", + f"{test_config_dir}/config_without_libraries_bom_version.yaml", + ), + ("owlbot_cli_image", f"{test_config_dir}/config_without_owlbot.yaml"), + ("synthtool_commitish", f"{test_config_dir}/config_without_synthtool.yaml"), + ( + "template_excludes", + f"{test_config_dir}/config_without_temp_excludes.yaml", + ), + ] + ) + def test_from_yaml_without_key_fails(self, error_message_contains, path_to_yaml): + self.assertRaisesRegex( + ValueError, + error_message_contains, + from_yaml, + path_to_yaml, + ) diff --git a/library_generation/test/utilities_unit_tests.py b/library_generation/test/utilities_unit_tests.py index 9b8f829876..7e8eccbcf8 100644 --- a/library_generation/test/utilities_unit_tests.py +++ b/library_generation/test/utilities_unit_tests.py @@ -26,7 +26,6 @@ from library_generation.model.gapic_config import GapicConfig from library_generation.model.generation_config import GenerationConfig from library_generation.model.gapic_inputs import parse as parse_build_file -from library_generation.model.generation_config import from_yaml from library_generation.model.library_config import LibraryConfig from library_generation.test.test_utils import FileComparator from library_generation.test.test_utils import cleanup @@ -114,55 +113,6 @@ def test_eprint_valid_input_succeeds(self): # print() appends a `\n` each time it's called self.assertEqual(test_input + "\n", result) - # parameterized tests need to run from the class, see - # https://github.com/wolever/parameterized/issues/37 for more info. - # This test confirms that a ValueError with an error message about a - # missing key (specified in the first parameter of each `parameterized` - # tuple) when parsing a test configuration yaml (second parameter) will - # be raised. - @parameterized.expand( - [ - ("libraries", f"{test_config_dir}/config_without_libraries.yaml"), - ("GAPICs", f"{test_config_dir}/config_without_gapics.yaml"), - ("proto_path", f"{test_config_dir}/config_without_proto_path.yaml"), - ("api_shortname", f"{test_config_dir}/config_without_api_shortname.yaml"), - ( - "api_description", - f"{test_config_dir}/config_without_api_description.yaml", - ), - ("name_pretty", f"{test_config_dir}/config_without_name_pretty.yaml"), - ( - "product_documentation", - f"{test_config_dir}/config_without_product_docs.yaml", - ), - ( - "gapic_generator_version", - f"{test_config_dir}/config_without_generator.yaml", - ), - ( - "googleapis_commitish", - f"{test_config_dir}/config_without_googleapis.yaml", - ), - ( - "libraries_bom_version", - f"{test_config_dir}/config_without_libraries_bom_version.yaml", - ), - ("owlbot_cli_image", f"{test_config_dir}/config_without_owlbot.yaml"), - ("synthtool_commitish", f"{test_config_dir}/config_without_synthtool.yaml"), - ( - "template_excludes", - f"{test_config_dir}/config_without_temp_excludes.yaml", - ), - ] - ) - def test_from_yaml_without_key_fails(self, error_message_contains, path_to_yaml): - self.assertRaisesRegex( - ValueError, - error_message_contains, - from_yaml, - path_to_yaml, - ) - @parameterized.expand( [ ("BUILD_no_additional_protos.bazel", " "), From 8aac8388320666dee5cbd11db53e38e64dec2d68 Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Fri, 26 Apr 2024 14:29:53 -0400 Subject: [PATCH 6/9] specify level --- library_generation/model/generation_config.py | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/library_generation/model/generation_config.py b/library_generation/model/generation_config.py index 63fde9ab55..2e417cb1c7 100644 --- a/library_generation/model/generation_config.py +++ b/library_generation/model/generation_config.py @@ -19,6 +19,9 @@ from library_generation.model.library_config import LibraryConfig from library_generation.model.gapic_config import GapicConfig +LIBRARY_LEVEL_PARAMETER = "Library-level parameter" +REPO_LEVEL_PARAMETER = "Repo-level parameter" + class GenerationConfig: """ @@ -86,7 +89,7 @@ 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) parsed_libraries = list() for library in libraries: @@ -128,23 +131,36 @@ 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} is not found in yaml.") + message = ( + f"{level}, {key}, is not found in {config} in yaml." + if level == LIBRARY_LEVEL_PARAMETER + else f"{level}, {key}, is not found in yaml." + ) + raise ValueError(message) return config[key] From f2dc50fcb106992bff3909fdce141b5a17c6a7c0 Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Fri, 26 Apr 2024 15:18:25 -0400 Subject: [PATCH 7/9] add gapic level --- library_generation/model/generation_config.py | 9 +- .../test/model/generation_config_unit_test.py | 144 ++++++++++++------ 2 files changed, 104 insertions(+), 49 deletions(-) diff --git a/library_generation/model/generation_config.py b/library_generation/model/generation_config.py index 2e417cb1c7..a4b3ca32a1 100644 --- a/library_generation/model/generation_config.py +++ b/library_generation/model/generation_config.py @@ -19,8 +19,9 @@ from library_generation.model.library_config import LibraryConfig from library_generation.model.gapic_config import GapicConfig -LIBRARY_LEVEL_PARAMETER = "Library-level parameter" -REPO_LEVEL_PARAMETER = "Repo-level parameter" +REPO_LEVEL_PARAMETER = "Repo level parameter" +LIBRARY_LEVEL_PARAMETER = "Library level parameter" +GAPIC_LEVEL_PARAMETER = "GAPIC level parameter" class GenerationConfig: @@ -97,7 +98,7 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig: parsed_gapics = list() 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) @@ -157,7 +158,7 @@ def __required(config: Dict, key: str, level: str = LIBRARY_LEVEL_PARAMETER): if key not in config: message = ( f"{level}, {key}, is not found in {config} in yaml." - if level == LIBRARY_LEVEL_PARAMETER + if level != REPO_LEVEL_PARAMETER else f"{level}, {key}, is not found in yaml." ) raise ValueError(message) diff --git a/library_generation/test/model/generation_config_unit_test.py b/library_generation/test/model/generation_config_unit_test.py index 8f195bed3c..0fa0ae4de5 100644 --- a/library_generation/test/model/generation_config_unit_test.py +++ b/library_generation/test/model/generation_config_unit_test.py @@ -14,7 +14,6 @@ import os import unittest from pathlib import Path -from parameterized import parameterized from library_generation.model.generation_config import from_yaml, GenerationConfig from library_generation.model.library_config import LibraryConfig @@ -165,51 +164,106 @@ def test_validate_with_duplicate_library_name_raise_exception(self): ], ) - # parameterized tests need to run from the class, see - # https://github.com/wolever/parameterized/issues/37 for more info. - # This test confirms that a ValueError with an error message about a - # missing key (specified in the first parameter of each `parameterized` - # tuple) when parsing a test configuration yaml (second parameter) will - # be raised. - @parameterized.expand( - [ - ("libraries", f"{test_config_dir}/config_without_libraries.yaml"), - ("GAPICs", f"{test_config_dir}/config_without_gapics.yaml"), - ("proto_path", f"{test_config_dir}/config_without_proto_path.yaml"), - ("api_shortname", f"{test_config_dir}/config_without_api_shortname.yaml"), - ( - "api_description", - f"{test_config_dir}/config_without_api_description.yaml", - ), - ("name_pretty", f"{test_config_dir}/config_without_name_pretty.yaml"), - ( - "product_documentation", - f"{test_config_dir}/config_without_product_docs.yaml", - ), - ( - "gapic_generator_version", - f"{test_config_dir}/config_without_generator.yaml", - ), - ( - "googleapis_commitish", - f"{test_config_dir}/config_without_googleapis.yaml", - ), - ( - "libraries_bom_version", - f"{test_config_dir}/config_without_libraries_bom_version.yaml", - ), - ("owlbot_cli_image", f"{test_config_dir}/config_without_owlbot.yaml"), - ("synthtool_commitish", f"{test_config_dir}/config_without_synthtool.yaml"), - ( - "template_excludes", - f"{test_config_dir}/config_without_temp_excludes.yaml", - ), - ] - ) - def test_from_yaml_without_key_fails(self, error_message_contains, path_to_yaml): + def test_from_yaml_without_gapic_generator_version_raise_exception(self): self.assertRaisesRegex( ValueError, - error_message_contains, + "Repo level parameter, gapic_generator_version", from_yaml, - path_to_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, + "Library level parameter, api_description", + from_yaml, + f"{test_config_dir}/config_without_api_description.yaml", + ) + + def test_from_yaml_without_name_pretty_raise_exception(self): + self.assertRaisesRegex( + ValueError, + "Library level parameter, name_pretty", + from_yaml, + f"{test_config_dir}/config_without_name_pretty.yaml", + ) + + def test_from_yaml_without_product_documentation_raise_exception(self): + self.assertRaisesRegex( + ValueError, + "Library level parameter, product_documentation", + 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", + from_yaml, + f"{test_config_dir}/config_without_gapics.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", ) From 1cd4b95f42cd832952fdadd7ae1e25d58bda5b6b Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Fri, 26 Apr 2024 17:13:19 -0400 Subject: [PATCH 8/9] test library dict in error message --- library_generation/model/generation_config.py | 4 +++ .../test/model/generation_config_unit_test.py | 26 +++++++++++++++---- .../config_without_gapics_key.yaml | 3 +++ .../config_without_gapics_value.yaml | 4 +++ ...yaml => config_without_library_value.yaml} | 1 - 5 files changed, 32 insertions(+), 6 deletions(-) create mode 100644 library_generation/test/resources/test-config/config_without_gapics_key.yaml create mode 100644 library_generation/test/resources/test-config/config_without_gapics_value.yaml rename library_generation/test/resources/test-config/{config_without_gapics.yaml => config_without_library_value.yaml} (75%) diff --git a/library_generation/model/generation_config.py b/library_generation/model/generation_config.py index a4b3ca32a1..e92b7bfc47 100644 --- a/library_generation/model/generation_config.py +++ b/library_generation/model/generation_config.py @@ -91,12 +91,16 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig: config = yaml.safe_load(file_stream) 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", GAPIC_LEVEL_PARAMETER) new_gapic = GapicConfig(proto_path) diff --git a/library_generation/test/model/generation_config_unit_test.py b/library_generation/test/model/generation_config_unit_test.py index 0fa0ae4de5..2c8b3fc93d 100644 --- a/library_generation/test/model/generation_config_unit_test.py +++ b/library_generation/test/model/generation_config_unit_test.py @@ -231,7 +231,7 @@ def test_from_yaml_without_api_shortname_raise_exception(self): def test_from_yaml_without_api_description_raise_exception(self): self.assertRaisesRegex( ValueError, - "Library level parameter, api_description", + r"Library level parameter, api_description.*'api_shortname': 'apigeeconnect'.*", from_yaml, f"{test_config_dir}/config_without_api_description.yaml", ) @@ -239,7 +239,7 @@ def test_from_yaml_without_api_description_raise_exception(self): def test_from_yaml_without_name_pretty_raise_exception(self): self.assertRaisesRegex( ValueError, - "Library level parameter, name_pretty", + r"Library level parameter, name_pretty.*'api_shortname': 'apigeeconnect'.*", from_yaml, f"{test_config_dir}/config_without_name_pretty.yaml", ) @@ -247,7 +247,7 @@ def test_from_yaml_without_name_pretty_raise_exception(self): def test_from_yaml_without_product_documentation_raise_exception(self): self.assertRaisesRegex( ValueError, - "Library level parameter, product_documentation", + r"Library level parameter, product_documentation.*'api_shortname': 'apigeeconnect'.*", from_yaml, f"{test_config_dir}/config_without_product_docs.yaml", ) @@ -255,9 +255,9 @@ def test_from_yaml_without_product_documentation_raise_exception(self): def test_from_yaml_without_gapics_raise_exception(self): self.assertRaisesRegex( ValueError, - "Library level parameter, GAPICs", + "Library level parameter, GAPICs.*'api_shortname': 'apigeeconnect'.*", from_yaml, - f"{test_config_dir}/config_without_gapics.yaml", + f"{test_config_dir}/config_without_gapics_key.yaml", ) def test_from_yaml_without_proto_path_raise_exception(self): @@ -267,3 +267,19 @@ def test_from_yaml_without_proto_path_raise_exception(self): 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 new file mode 100644 index 0000000000..739a4d9239 --- /dev/null +++ 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 new file mode 100644 index 0000000000..ec49e4a669 --- /dev/null +++ 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_gapics.yaml b/library_generation/test/resources/test-config/config_without_library_value.yaml similarity index 75% rename from library_generation/test/resources/test-config/config_without_gapics.yaml rename to library_generation/test/resources/test-config/config_without_library_value.yaml index 0f0c49fc48..174a293000 100644 --- a/library_generation/test/resources/test-config/config_without_gapics.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: From 8dca4ed4ff67f7fb1c46a41329a3678914e4ef77 Mon Sep 17 00:00:00 2001 From: Joe Wang Date: Fri, 26 Apr 2024 18:02:36 -0400 Subject: [PATCH 9/9] refactor --- library_generation/model/generation_config.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/library_generation/model/generation_config.py b/library_generation/model/generation_config.py index e92b7bfc47..2a5d453981 100644 --- a/library_generation/model/generation_config.py +++ b/library_generation/model/generation_config.py @@ -12,10 +12,8 @@ # 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 @@ -36,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, ): @@ -158,7 +156,7 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig: return parsed_config -def __required(config: Dict, key: str, level: str = LIBRARY_LEVEL_PARAMETER): +def __required(config: dict, key: str, level: str = LIBRARY_LEVEL_PARAMETER): if key not in config: message = ( f"{level}, {key}, is not found in {config} in yaml." @@ -169,7 +167,7 @@ def __required(config: Dict, key: str, level: str = LIBRARY_LEVEL_PARAMETER): 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]