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..2a5d453981 100644 --- 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 index ae168e9671..ace2794684 100644 --- 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 index fd7608bb4f..2c8b3fc93d 100644 --- 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 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: 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..c5613f4308 --- /dev/null +++ b/library_generation/test/resources/test-config/generation_config_with_duplicate_library_name.yaml @@ -0,0 +1,51 @@ +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 + 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 + + - 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 diff --git a/library_generation/test/utilities_unit_tests.py b/library_generation/test/utilities_unit_tests.py index 5e5869d0c1..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", " "), @@ -344,17 +294,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"