From e508ae69e13455ba2f5c325e0c33d9ff2704e3b1 Mon Sep 17 00:00:00 2001 From: Joe Wang <106995533+JoeWang1127@users.noreply.github.com> Date: Mon, 10 Jun 2024 16:46:54 -0400 Subject: [PATCH] feat: generate pr description from configuration comparison result (#2841) In this PR: - Generate pull request description from configuration comparison result. - The pull request description contains repo-level changes, if applicable, and googleapis commit from baseline config (exclusive) to current config (inclusive). An example of pr description with repo-level change: ``` This pull request is generated with proto changes between [googleapis/googleapis@3b6f144](https://github.com/googleapis/googleapis/commit/3b6f144d47b0a1d2115ab2445ec06e80cc324a44) (exclusive) and [googleapis/googleapis@0cea717](https://github.com/googleapis/googleapis/commit/0cea7170404bec3d994f43db4fa292f5034cbe9a) (inclusive). BEGIN_COMMIT_OVERRIDE BEGIN_NESTED_COMMIT fix(deps): update the Java code generator (gapic-generator-java) to 1.2.3 END_NESTED_COMMIT BEGIN_NESTED_COMMIT chore: update the libraries_bom version to 2.3.4 END_NESTED_COMMIT BEGIN_NESTED_COMMIT feat: Make Layout Parser generally available in V1 PiperOrigin-RevId: 638924855 Source Link: [googleapis/googleapis@0cea717](https://github.com/googleapis/googleapis/commit/0cea7170404bec3d994f43db4fa292f5034cbe9a) END_NESTED_COMMIT END_COMMIT_OVERRIDE ``` --- .../hermetic_library_generation.yaml | 2 +- library_generation/cli/entry_point.py | 3 +- library_generation/generate_pr_description.py | 68 +++++---- library_generation/model/generation_config.py | 6 +- .../generate_pr_description_unit_tests.py | 144 +++++++++++++----- .../goldens/pr_description-golden.txt | 17 +++ .../commit_message_formatter_unit_tests.py | 48 +++++- .../utils/commit_message_formatter.py | 59 +++++-- 8 files changed, 262 insertions(+), 85 deletions(-) create mode 100644 library_generation/test/resources/goldens/pr_description-golden.txt diff --git a/.github/workflows/hermetic_library_generation.yaml b/.github/workflows/hermetic_library_generation.yaml index da7dd74272..218f6751f5 100644 --- a/.github/workflows/hermetic_library_generation.yaml +++ b/.github/workflows/hermetic_library_generation.yaml @@ -34,7 +34,7 @@ jobs: -f .cloudbuild/library_generation/library_generation.Dockerfile \ -t gcr.io/cloud-devrel-public-resources/java-library-generation:latest \ . - - name: Install gapic-generator-java-pom-parent + - name: Install all modules shell: bash run: | mvn -V -B -ntp clean install -DskipTests diff --git a/library_generation/cli/entry_point.py b/library_generation/cli/entry_point.py index 0bb499e9f2..dbcc18b267 100644 --- a/library_generation/cli/entry_point.py +++ b/library_generation/cli/entry_point.py @@ -135,8 +135,7 @@ def generate( target_library_names=config_change.get_changed_libraries(), ) generate_pr_descriptions( - config=config_change.current_config, - baseline_commit=config_change.baseline_config.googleapis_commitish, + config_change=config_change, description_path=repository_path, ) diff --git a/library_generation/generate_pr_description.py b/library_generation/generate_pr_description.py index e4852b6eab..75d6671e43 100644 --- a/library_generation/generate_pr_description.py +++ b/library_generation/generate_pr_description.py @@ -17,10 +17,12 @@ import shutil from typing import Dict from git import Commit, Repo -from library_generation.model.generation_config import GenerationConfig + +from library_generation.model.config_change import ConfigChange from library_generation.utils.proto_path_utils import find_versioned_proto_path from library_generation.utils.commit_message_formatter import ( format_commit_message, + format_repo_level_change, commit_link, ) from library_generation.utils.commit_message_formatter import wrap_override_commit @@ -29,42 +31,38 @@ def generate_pr_descriptions( - config: GenerationConfig, - baseline_commit: str, + config_change: ConfigChange, description_path: str, repo_url: str = "https://github.com/googleapis/googleapis.git", ) -> None: """ - Generate pull request description from baseline_commit (exclusive) to the - googleapis commit (inclusive) in the given generation config. + Generate pull request description from configuration comparison result. + + The pull request description contains repo-level changes, if applicable, + and googleapis commit from baseline config (exclusive) to current config + (inclusive). The pull request description will be generated into description_path/pr_description.txt. - If baseline_commit is the same as googleapis commit in the given generation - config, no pr_description.txt will be generated. + No pr_description.txt will be generated if no changes in the configurations. - :param config: a GenerationConfig object. The googleapis commit in this - configuration is the latest commit, inclusively, from which the commit - message is considered. - :param baseline_commit: The baseline (oldest) commit, exclusively, from - which the commit message is considered. This commit should be an ancestor - of googleapis commit in configuration. + :param config_change: a ConfigChange object, containing changes in baseline + and current generation configurations. :param description_path: the path to which the pull request description file goes. :param repo_url: the GitHub repository from which retrieves the commit history. """ - if baseline_commit == config.googleapis_commitish: - return - - paths = config.get_proto_path_to_library_name() - description = get_commit_messages( + repo_level_message = format_repo_level_change(config_change) + paths = config_change.current_config.get_proto_path_to_library_name() + description = get_repo_level_commit_messages( repo_url=repo_url, - current_commit_sha=config.googleapis_commitish, - baseline_commit_sha=baseline_commit, + current_commit_sha=config_change.current_config.googleapis_commitish, + baseline_commit_sha=config_change.baseline_config.googleapis_commitish, paths=paths, - is_monorepo=config.is_monorepo(), + is_monorepo=config_change.current_config.is_monorepo(), + repo_level_message=repo_level_message, ) if description == EMPTY_MESSAGE: @@ -77,12 +75,13 @@ def generate_pr_descriptions( f.write(description) -def get_commit_messages( +def get_repo_level_commit_messages( repo_url: str, current_commit_sha: str, baseline_commit_sha: str, paths: Dict[str, str], is_monorepo: bool, + repo_level_message: list[str] = None, ) -> str: """ Combine commit messages of a repository from latest_commit to @@ -97,10 +96,13 @@ def get_commit_messages( selecting commit message. This commit should be an ancestor of :param paths: a mapping from file paths to library_name. :param is_monorepo: whether to generate commit messages in a monorepo. + :param repo_level_message: commit messages regarding repo-level changes. :return: commit messages. :raise ValueError: if current_commit is older than or equal to baseline_commit. """ + if current_commit_sha == baseline_commit_sha: + return EMPTY_MESSAGE tmp_dir = "/tmp/repo" shutil.rmtree(tmp_dir, ignore_errors=True) os.mkdir(tmp_dir) @@ -134,6 +136,7 @@ def get_commit_messages( baseline_commit=baseline_commit, commits=qualified_commits, is_monorepo=is_monorepo, + repo_level_message=repo_level_message, ) @@ -160,20 +163,19 @@ def __combine_commit_messages( baseline_commit: Commit, commits: Dict[Commit, str], is_monorepo: bool, + repo_level_message: list[str], ) -> str: - messages = [ - f"This pull request is generated with proto changes between {commit_link(baseline_commit)} (exclusive) " - f"and {commit_link(current_commit)} (inclusive).", - "", + description = [ + f"This pull request is generated with proto changes between " + f"{commit_link(baseline_commit)} (exclusive) " + f"and {commit_link(current_commit)} (inclusive).\n", ] - - messages.extend( - wrap_override_commit( - format_commit_message(commits=commits, is_monorepo=is_monorepo) - ) + commit_message = repo_level_message + commit_message.extend( + format_commit_message(commits=commits, is_monorepo=is_monorepo) ) - - return "\n".join(messages) + description.extend(wrap_override_commit(commit_message)) + return "\n".join(description) def __get_commit_timestamp(commit: Commit) -> int: diff --git a/library_generation/model/generation_config.py b/library_generation/model/generation_config.py index 4503daf163..3a924600e5 100644 --- a/library_generation/model/generation_config.py +++ b/library_generation/model/generation_config.py @@ -21,6 +21,8 @@ LIBRARY_LEVEL_PARAMETER = "Library level parameter" GAPIC_LEVEL_PARAMETER = "GAPIC level parameter" COMMON_PROTOS_LIBRARY_NAME = "common-protos" +GAPIC_GENERATOR_VERSION = "gapic_generator_version" +LIBRARIES_BOM_VERSION = "libraries_bom_version" class GenerationConfig: @@ -144,14 +146,14 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig: parsed_config = GenerationConfig( gapic_generator_version=__required( - config, "gapic_generator_version", REPO_LEVEL_PARAMETER + config, GAPIC_GENERATOR_VERSION, REPO_LEVEL_PARAMETER ), googleapis_commitish=__required( config, "googleapis_commitish", REPO_LEVEL_PARAMETER ), grpc_version=__optional(config, "grpc_version", None), protoc_version=__optional(config, "protoc_version", None), - libraries_bom_version=__optional(config, "libraries_bom_version", None), + libraries_bom_version=__optional(config, LIBRARIES_BOM_VERSION, None), libraries=parsed_libraries, ) diff --git a/library_generation/test/generate_pr_description_unit_tests.py b/library_generation/test/generate_pr_description_unit_tests.py index 1a86770ac0..b727cc3da9 100644 --- a/library_generation/test/generate_pr_description_unit_tests.py +++ b/library_generation/test/generate_pr_description_unit_tests.py @@ -13,12 +13,23 @@ # limitations under the License. import os import unittest +from filecmp import cmp from library_generation.generate_pr_description import ( - get_commit_messages, + get_repo_level_commit_messages, generate_pr_descriptions, ) +from library_generation.model.config_change import ( + ConfigChange, + ChangeType, + LibraryChange, +) +from library_generation.model.gapic_config import GapicConfig from library_generation.model.generation_config import GenerationConfig +from library_generation.model.library_config import LibraryConfig + +script_dir = os.path.dirname(os.path.realpath(__file__)) +resources_dir = os.path.join(script_dir, "resources", "goldens") class GeneratePrDescriptionTest(unittest.TestCase): @@ -30,7 +41,7 @@ def test_get_commit_messages_current_is_older_raise_exception(self): self.assertRaisesRegex( ValueError, "newer than", - get_commit_messages, + get_repo_level_commit_messages, "https://github.com/googleapis/googleapis.git", current_commit, baseline_commit, @@ -38,36 +49,43 @@ def test_get_commit_messages_current_is_older_raise_exception(self): True, ) - def test_get_commit_messages_current_and_baseline_are_same_raise_exception(self): + def test_get_commit_messages_with_same_current_and_baseline_returns_empty_message( + self, + ): # committed on April 1st, 2024 current_commit = "36441693dddaf0ed73951ad3a15c215a332756aa" baseline_commit = "36441693dddaf0ed73951ad3a15c215a332756aa" - self.assertRaisesRegex( - ValueError, - "newer than", - get_commit_messages, - "https://github.com/googleapis/googleapis.git", - current_commit, - baseline_commit, - {}, - True, + self.assertEqual( + "", + get_repo_level_commit_messages( + "https://github.com/googleapis/googleapis.git", + current_commit, + baseline_commit, + {}, + True, + ), ) - def test_generate_pr_description_with_same_googleapis_commits(self): + def test_generate_pr_description_with_no_change_in_config(self): commit_sha = "36441693dddaf0ed73951ad3a15c215a332756aa" - cwd = os.getcwd() + config = GenerationConfig( + gapic_generator_version="", + googleapis_commitish=commit_sha, + libraries_bom_version="", + # use empty libraries to make sure no qualified commit between + # two commit sha. + libraries=[], + ) + pr_description_path = os.path.join(os.getcwd(), "no_config_change") generate_pr_descriptions( - config=GenerationConfig( - gapic_generator_version="", - googleapis_commitish=commit_sha, - grpc_version="", - protoc_version="", - libraries=[], + config_change=ConfigChange( + change_to_libraries={}, + baseline_config=config, + current_config=config, ), - baseline_commit=commit_sha, - description_path=cwd, + description_path=pr_description_path, ) - self.assertFalse(os.path.isfile(f"{cwd}/pr_description.txt")) + self.assertFalse(os.path.isfile(f"{pr_description_path}/pr_description.txt")) def test_generate_pr_description_does_not_create_pr_description_without_qualified_commit( self, @@ -76,19 +94,77 @@ def test_generate_pr_description_does_not_create_pr_description_without_qualifie old_commit_sha = "30717c0b0c9966906880703208a4c820411565c4" # committed on May 23rd, 2024 new_commit_sha = "eeed69d446a90eb4a4a2d1762c49d637075390c1" + pr_description_path = os.path.join(os.getcwd(), "no_qualified_commit") + generate_pr_descriptions( + config_change=ConfigChange( + change_to_libraries={}, + baseline_config=GenerationConfig( + gapic_generator_version="", + googleapis_commitish=old_commit_sha, + # use empty libraries to make sure no qualified commit between + # two commit sha. + libraries=[], + ), + current_config=GenerationConfig( + gapic_generator_version="", + googleapis_commitish=new_commit_sha, + # use empty libraries to make sure no qualified commit between + # two commit sha. + libraries=[], + ), + ), + description_path=pr_description_path, + ) + self.assertFalse(os.path.isfile(f"{pr_description_path}/pr_description.txt")) + + def test_generate_pr_description_with_combined_message( + self, + ): + # no other commits between these two commits. + baseline_commit_sha = "3b6f144d47b0a1d2115ab2445ec06e80cc324a44" + documentai_commit_sha = "0cea7170404bec3d994f43db4fa292f5034cbe9a" cwd = os.getcwd() + library = LibraryConfig( + api_shortname="documentai", + api_description="", + name_pretty="", + product_documentation="", + gapic_configs=[GapicConfig(proto_path="google/cloud/documentai/v1")], + ) generate_pr_descriptions( - config=GenerationConfig( - gapic_generator_version="", - googleapis_commitish=new_commit_sha, - libraries_bom_version="", - grpc_version="", - protoc_version="", - # use empty libraries to make sure no qualified commit between - # two commit sha. - libraries=[], + config_change=ConfigChange( + change_to_libraries={ + ChangeType.REPO_LEVEL_CHANGE: [ + LibraryChange( + changed_param="gapic_generator_version", + current_value="1.2.3", + ), + LibraryChange( + changed_param="libraries_bom_version", current_value="2.3.4" + ), + ], + ChangeType.GOOGLEAPIS_COMMIT: [], + }, + baseline_config=GenerationConfig( + gapic_generator_version="", + googleapis_commitish=baseline_commit_sha, + libraries=[library], + ), + current_config=GenerationConfig( + gapic_generator_version="1.2.3", + googleapis_commitish=documentai_commit_sha, + libraries_bom_version="2.3.4", + libraries=[library], + ), ), - baseline_commit=old_commit_sha, description_path=cwd, ) - self.assertFalse(os.path.isfile(f"{cwd}/pr_description.txt")) + self.assertTrue(os.path.isfile(f"{cwd}/pr_description.txt")) + self.assertTrue( + cmp( + f"{resources_dir}/pr_description-golden.txt", + f"{cwd}/pr_description.txt", + ), + "The generated PR description does not match the expected golden file", + ) + os.remove(f"{cwd}/pr_description.txt") diff --git a/library_generation/test/resources/goldens/pr_description-golden.txt b/library_generation/test/resources/goldens/pr_description-golden.txt new file mode 100644 index 0000000000..1a0f874936 --- /dev/null +++ b/library_generation/test/resources/goldens/pr_description-golden.txt @@ -0,0 +1,17 @@ +This pull request is generated with proto changes between [googleapis/googleapis@3b6f144](https://github.com/googleapis/googleapis/commit/3b6f144d47b0a1d2115ab2445ec06e80cc324a44) (exclusive) and [googleapis/googleapis@0cea717](https://github.com/googleapis/googleapis/commit/0cea7170404bec3d994f43db4fa292f5034cbe9a) (inclusive). + +BEGIN_COMMIT_OVERRIDE +BEGIN_NESTED_COMMIT +fix(deps): update the Java code generator (gapic-generator-java) to 1.2.3 +END_NESTED_COMMIT +BEGIN_NESTED_COMMIT +chore: update the libraries_bom version to 2.3.4 +END_NESTED_COMMIT +BEGIN_NESTED_COMMIT +feat: Make Layout Parser generally available in V1 + +PiperOrigin-RevId: 638924855 + +Source Link: [googleapis/googleapis@0cea717](https://github.com/googleapis/googleapis/commit/0cea7170404bec3d994f43db4fa292f5034cbe9a) +END_NESTED_COMMIT +END_COMMIT_OVERRIDE \ No newline at end of file diff --git a/library_generation/test/utils/commit_message_formatter_unit_tests.py b/library_generation/test/utils/commit_message_formatter_unit_tests.py index 0148214dfb..16e3fffdfc 100644 --- a/library_generation/test/utils/commit_message_formatter_unit_tests.py +++ b/library_generation/test/utils/commit_message_formatter_unit_tests.py @@ -14,13 +14,24 @@ import unittest from unittest.mock import patch +from library_generation.model.config_change import ( + ConfigChange, + ChangeType, + LibraryChange, +) +from library_generation.model.generation_config import GenerationConfig from library_generation.utils.commit_message_formatter import ( format_commit_message, commit_link, + format_repo_level_change, ) -from library_generation.utils.commit_message_formatter import wrap_nested_commit +from library_generation.utils.commit_message_formatter import wrap_googleapis_commit from library_generation.utils.commit_message_formatter import wrap_override_commit +gen_config = GenerationConfig( + gapic_generator_version="1.2.3", googleapis_commitish="123abc", libraries=[] +) + class CommitMessageFormatterTest(unittest.TestCase): def test_format_commit_message_should_add_library_name_for_conventional_commit( @@ -130,7 +141,7 @@ def test_wrap_nested_commit_success(self): "Source Link: [googleapis/googleapis@1234567](https://github.com/googleapis/googleapis/commit/1234567abcdefg)", "END_NESTED_COMMIT", ], - wrap_nested_commit(commit, messages), + wrap_googleapis_commit(commit, messages), ) def test_wrap_override_commit_success(self): @@ -153,3 +164,36 @@ def test_commit_link_success(self): "[googleapis/googleapis@1234567](https://github.com/googleapis/googleapis/commit/1234567abcdefg)", commit_link(commit), ) + + def test_format_repo_level_change_success(self): + config_change = ConfigChange( + change_to_libraries={ + ChangeType.REPO_LEVEL_CHANGE: [ + LibraryChange( + changed_param="gapic_generator_version", current_value="1.2.3" + ), + LibraryChange( + changed_param="libraries_bom_version", current_value="2.3.4" + ), + LibraryChange( + changed_param="protoc_version", current_value="3.4.5" + ), + ] + }, + baseline_config=gen_config, + current_config=gen_config, + ) + self.assertEqual( + [ + "BEGIN_NESTED_COMMIT", + "fix(deps): update the Java code generator (gapic-generator-java) to 1.2.3", + "END_NESTED_COMMIT", + "BEGIN_NESTED_COMMIT", + "chore: update the libraries_bom version to 2.3.4", + "END_NESTED_COMMIT", + "BEGIN_NESTED_COMMIT", + "chore: update repo-level parameter protoc_version to 3.4.5", + "END_NESTED_COMMIT", + ], + format_repo_level_change(config_change), + ) diff --git a/library_generation/utils/commit_message_formatter.py b/library_generation/utils/commit_message_formatter.py index 85bfbc8036..5b75db51a0 100644 --- a/library_generation/utils/commit_message_formatter.py +++ b/library_generation/utils/commit_message_formatter.py @@ -12,12 +12,21 @@ # See the License for the specific language governing permissions and # limitations under the License. import re -from typing import List -from typing import Dict from git import Commit +from library_generation.model.config_change import ConfigChange, ChangeType +from library_generation.model.generation_config import ( + GAPIC_GENERATOR_VERSION, + LIBRARIES_BOM_VERSION, +) -def format_commit_message(commits: Dict[Commit, str], is_monorepo: bool) -> List[str]: +PARAM_TO_COMMIT_MESSAGE = { + GAPIC_GENERATOR_VERSION: "fix(deps): update the Java code generator (gapic-generator-java) to", + LIBRARIES_BOM_VERSION: "chore: update the libraries_bom version to", +} + + +def format_commit_message(commits: dict[Commit, str], is_monorepo: bool) -> list[str]: """ Format commit messages. Add library_name to conventional commit messages if is_monorepo is True; otherwise no op. @@ -47,11 +56,29 @@ def format_commit_message(commits: Dict[Commit, str], is_monorepo: bool) -> List messages.append(formatted_message) else: messages.append(message_line) - all_commits.extend(wrap_nested_commit(commit, messages)) + all_commits.extend(wrap_googleapis_commit(commit, messages)) return all_commits -def wrap_nested_commit(commit: Commit, messages: List[str]) -> List[str]: +def format_repo_level_change(config_change: ConfigChange) -> list[str]: + """ + Format commit messages regarding repo-level changes. + + :param config_change: + :return: commit messages regarding repo-level changes. + """ + messages = [] + for repo_level_change in config_change.change_to_libraries.get( + ChangeType.REPO_LEVEL_CHANGE, [] + ): + message = f"chore: update repo-level parameter {repo_level_change.changed_param} to {repo_level_change.current_value}" + if repo_level_change.changed_param in PARAM_TO_COMMIT_MESSAGE: + message = f"{PARAM_TO_COMMIT_MESSAGE.get(repo_level_change.changed_param)} {repo_level_change.current_value}" + messages.extend(__wrap_nested_commit([message])) + return messages + + +def wrap_googleapis_commit(commit: Commit, messages: list[str]) -> list[str]: """ Wrap message between `BEGIN_NESTED_COMMIT` and `BEGIN_NESTED_COMMIT`. @@ -59,14 +86,11 @@ def wrap_nested_commit(commit: Commit, messages: List[str]) -> List[str]: :param messages: a (multi-line) commit message, one line per item. :return: wrapped messages. """ - result = ["BEGIN_NESTED_COMMIT"] - result.extend(messages) - result.append(f"Source Link: {commit_link(commit)}") - result.append("END_NESTED_COMMIT") - return result + messages.append(f"Source Link: {commit_link(commit)}") + return __wrap_nested_commit(messages) -def wrap_override_commit(messages: List[str]) -> List[str]: +def wrap_override_commit(messages: list[str]) -> list[str]: """ Wrap message between `BEGIN_COMMIT_OVERRIDE` and `END_COMMIT_OVERRIDE`. @@ -88,3 +112,16 @@ def commit_link(commit: Commit) -> str: """ short_sha = commit.hexsha[:7] return f"[googleapis/googleapis@{short_sha}](https://github.com/googleapis/googleapis/commit/{commit.hexsha})" + + +def __wrap_nested_commit(messages: list[str]) -> list[str]: + """ + Wrap message between `BEGIN_NESTED_COMMIT` and `BEGIN_NESTED_COMMIT`. + + :param messages: a (multi-line) commit message, one line per item. + :return: wrapped messages. + """ + result = ["BEGIN_NESTED_COMMIT"] + result.extend(messages) + result.append("END_NESTED_COMMIT") + return result