Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Apr 26, 2024
24 changes: 24 additions & 0 deletions library_generation/cli/entry_point.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
16 changes: 15 additions & 1 deletion library_generation/model/generation_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
"""
Expand All @@ -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 "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

f"{seen_library_names.get(library_name)}"
)
seen_library_names[library_name] = library.name_pretty


def from_yaml(path_to_yaml: str) -> GenerationConfig:
"""
Expand Down Expand Up @@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
return config[key]


Expand Down
35 changes: 34 additions & 1 deletion library_generation/test/cli/entry_point_unit_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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.title(),
"Cloud Asset Inventory Has The Same Library Name With Cloud Asset",
)
30 changes: 30 additions & 0 deletions library_generation/test/model/generation_config_unit_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
),
],
)
Original file line number Diff line number Diff line change
@@ -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
11 changes: 0 additions & 11 deletions library_generation/test/utilities_unit_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 0 additions & 4 deletions library_generation/utils/utilities.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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"
Expand Down
Loading