feat: add config change #2604

JoeWang1127 · 2024-03-26T14:07:30Z

In this PR:

Add config_change.py to get:
- Changed libraries, which will be passed to generate_repo.py to generate libraries selectedly.
- Qualified commits, which will be passed to generate_pr_description.py to generate PR description.
Refactor part of utility methods to proto_path_utils.py.
Refactor generation_config_comparator.py to move data class to config_change.py.
Refactor utilities.py and utilities.sh to utils/utilities.py and utils/utilities.sh.
Add unit tests.

Follow up of #2587

For the goal of series of PRs, please refer to improvement proposal.

diegomarquezp · 2024-03-28T15:00:10Z

library_generation/model/config_change.py

+    def get_changed_libraries(self) -> Optional[list[str]]:
+        """
+        Returns library name of changed libraries.
+        None if there is a repository level change.


None has a deep meaning related to all libraries being processed. I think it would help to explain what it means. For example "None represents that all libraries are affected", or return a constant ALL_LIBRARIES_CHANGED=None to make it self-explanatory.

diegomarquezp · 2024-03-28T15:03:19Z

library_generation/model/config_change.py

+        :param repo_url: the repository contains the commit history.
+        :return: QualifiedCommit objects.
+        """
+        tmp_dir = "/tmp/repo"


Is it possible to download googleapis in the output folder? This is just because we will convert our scripts to only use output and not /tmp in favor of running docker containers in parallel.

I changed to sh_util("get_output_folder")

diegomarquezp · 2024-03-28T15:06:17Z

library_generation/model/config_change.py

+        qualified_commits = self.get_qualified_commits()
+        library_names = []
+        for qualified_commit in qualified_commits:
+            library_names.extend(qualified_commit.libraries)


If I understand correctly, library_names could end up with repeated library names. Is this handled in any way?
If not, we can remove duplicates with something like return list(set(library_names))

Good catch.

I updated the method to return a list of unique library names in ascending order and added a unit test to verify it.

diegomarquezp · 2024-03-28T15:06:50Z

library_generation/model/config_change.py

+        shutil.rmtree(tmp_dir, ignore_errors=True)
+        return qualified_commits
+
+    def __get_library_name_from_qualified_commits(self) -> list[str]:


Suggested change

def __get_library_name_from_qualified_commits(self) -> list[str]:

def __get_library_names_from_qualified_commits(self) -> list[str]:

(This may imply more changes than just changing this line)

diegomarquezp · 2024-03-28T15:09:17Z

library_generation/model/config_change.py

+        """
+        Returns a qualified commit from the given Commit object; otherwise None.
+
+        :param proto_paths:


Would you mind expanding this comment to tell what are the expected contents of each key and value in proto_paths?

diegomarquezp · 2024-03-28T15:10:53Z

library_generation/test/model/config_change_unit_tests.py

+            baseline_config=ConfigChangeTest.__get_a_gen_config(),
+            latest_config=ConfigChangeTest.__get_a_gen_config(),
+        )
+        self.assertIsNone(config_change.get_changed_libraries())


Similar to one of the comments above, the test would be a bit more readable if we added something like self.assertEquals(ALL_LIBRARIES_AFFECTED, config_change.get_changed_libraries())

diegomarquezp · 2024-03-28T15:13:15Z

library_generation/test/model/config_change_unit_tests.py

+        self.assertEqual(3, len(qualified_commits))
+        self.assertEqual({"gke-backup"}, qualified_commits[0].libraries)
+        self.assertEqual(
+            "b8691edb3f1d3c1583aa9cd89240eb359eebe9c7",


nit: can we use variable names for these commit hashes? This is just in order to make the test more self-explanatory.

diegomarquezp · 2024-03-28T15:15:45Z

library_generation/utils/proto_path_utils.py

+
+def find_versioned_proto_path(file_path: str) -> str:
+    """
+    Returns a versioned proto_path from a given file_path; or file_path itself


Can we clarify that file_path should be a proto file?

blakeli0 · 2024-04-01T21:20:54Z

library_generation/model/config_change.py

+        shutil.rmtree(tmp_dir, ignore_errors=True)
+        os.mkdir(tmp_dir)
+        # we only need commit history, thus shadow clone is enough.
+        repo = Repo.clone_from(url=repo_url, to_path=tmp_dir, filter=["blob:none"])


I know this was done in previous PRs. Based on our recent discussions, is it possible to get the commit history remotely? Without cloning the repo? Creating new temp folders could have other implications like preventing us from running images in parallel.

I did some digging but I can't find one without git clone.

I don't think we need to focus on parallel execution as it's not the main goal of this PR. Also, we can achieve parallelism if we can randomize the temp folder so that no thread will clone the repo into the same folder.

I don't think it's a focus either but something we should keep in mind. Creating temp folders and writing files has other implications, e.g. it's usually slower than making a request, it complicates the scripts that we have to clean the temp folders and files up etc.
That being said, it's OK if there is no easy way at this moment. We can refactor how we pull the proto definitions in general in the future. e.g. I see that we are currently downloading proto definitions for every library, where we could download the whole googleapis repo as the first step of generate_repo.py. This actually brings another question that if googleapis_commitish is overridden on library level, our release notes might be generated incorrectly.
In short, this PR is good to go as it is, but we need to put the above tasks into consideration and track them somewhere.

I see that we are currently downloading proto definitions for every library,

The googleapis repo is downloaded only once unless there are googleapis commit in library level.

Since this use case is rarely used, how about removing it?

Based on discussion, we keep this piece of code as-is.

blakeli0 · 2024-04-02T16:11:12Z

library_generation/utils/utilities.py

@@ -65,7 +65,7 @@ def sh_util(statement: str, **kwargs) -> str:
        kwargs["stderr"] = subprocess.PIPE
    output = ""
    with subprocess.Popen(
-        ["bash", "-exc", f"source {script_dir}/utilities.sh && {statement}"],
+        ["bash", "-exc", f"source {script_dir}/../utilities.sh && {statement}"],


How about move utilities.sh as well?

blakeli0

LGTM. Please track the refactoring and potential issues discussed in this PR in our project plan.

sonarcloud · 2024-04-02T18:38:29Z

Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarcloud · 2024-04-02T18:41:40Z

Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

In this PR: - Add `config_change.py` to get: - Changed libraries, which will be passed to `generate_repo.py` to generate libraries selectedly. - Qualified commits, which will be passed to `generate_pr_description.py` to generate PR description. - Refactor part of utility methods to `proto_path_utils.py`. - Refactor `generation_config_comparator.py` to move data class to `config_change.py`. - Refactor `utilities.py` and `utilities.sh` to `utils/utilities.py` and `utils/utilities.sh`. - Add unit tests. Follow up of #2587 For the goal of series of PRs, please refer to [improvement proposal](https://docs.google.com/document/d/1JiCcG3X7lnxaJErKe0ES_JkyU7ECb40nf2Xez3gWvuo/edit?tab=t.g3vua2kd06gx#bookmark=id.72s3ukwwzevo).

In this PR: - Create `entry_point.py` to combine `generate_repo.py` and `generate_pr_description.py`. - Change Integration test. Follow up of #2604. For the goal of series of PRs, please refer to [improvement proposal](https://docs.google.com/document/d/1JiCcG3X7lnxaJErKe0ES_JkyU7ECb40nf2Xez3gWvuo/edit?tab=t.g3vua2kd06gx#bookmark=id.72s3ukwwzevo).

🤖 I have created a release *beep* *boop* --- <details><summary>2.39.0</summary> ## [2.39.0](v2.38.1...v2.39.0) (2024-04-18) ### Features * add `libraries_bom_version` to generation configuration ([#2639](#2639)) ([56c7ca5](56c7ca5)) * Add ChannelPoolSettings Getter for gRPC's ChannelProvider ([#2612](#2612)) ([d0c5191](d0c5191)) * add config change ([#2604](#2604)) ([8312706](8312706)) * add entry point ([#2616](#2616)) ([b19fa33](b19fa33)) * add generation config comparator ([#2587](#2587)) ([a94c2f0](a94c2f0)) * Add JavadocJar Task to build.gradle for self service libraries ([#2593](#2593)) ([993f5ac](993f5ac)) * Client/StubSettings' getEndpoint() returns the resolved endpoint ([#2440](#2440)) ([4942bc1](4942bc1)) * generate selected libraries ([#2598](#2598)) ([739ddbb](739ddbb)) * Validate the Universe Domain inside Java-Core ([#2592](#2592)) ([35d789f](35d789f)) ### Bug Fixes * add main to `generate_repo.py` ([#2607](#2607)) ([fedeb32](fedeb32)) * correct deep-remove and deep-preserve regexes ([#2572](#2572)) ([4c7fd88](4c7fd88)) * first attempt should use the min of RPC timeout and total timeout ([#2641](#2641)) ([0349232](0349232)) * remove duplicated calls to AutoValue builders ([#2636](#2636)) ([53a3727](53a3727)) * remove unnecessary slf4j and AbstractGoogleClientRequest native image configs ([0cb7d0e](0cb7d0e)) * remove unnecessary slf4j and AbstractGoogleClientRequest native image configs ([#2628](#2628)) ([0cb7d0e](0cb7d0e)) ### Dependencies * update arrow.version to v15.0.2 ([#2589](#2589)) ([777acf3](777acf3)) * update dependency com.google.cloud.opentelemetry:detector-resources-support to v0.28.0 ([#2649](#2649)) ([e4ed176](e4ed176)) * update dependency gitpython to v3.1.41 [security] ([#2625](#2625)) ([e41bd8f](e41bd8f)) * update dependency net.bytebuddy:byte-buddy to v1.14.13 ([#2646](#2646)) ([73ac5a4](73ac5a4)) * update dependency org.threeten:threeten-extra to v1.8.0 ([#2650](#2650)) ([226325a](226325a)) * update dependency org.threeten:threetenbp to v1.6.9 ([#2602](#2602)) ([371753e](371753e)) * update dependency org.threeten:threetenbp to v1.6.9 ([#2665](#2665)) ([8935bc8](8935bc8)) * update google api dependencies ([#2584](#2584)) ([cd20604](cd20604)) * update googleapis/java-cloud-bom digest to 7071341 ([#2608](#2608)) ([8d74140](8d74140)) * update netty dependencies to v4.1.109.final ([#2597](#2597)) ([8990693](8990693)) * update opentelemetry-java monorepo to v1.37.0 ([#2652](#2652)) ([f8fa2e9](f8fa2e9)) * update protobuf dependencies to v3.25.3 ([#2491](#2491)) ([b0e5041](b0e5041)) * update slf4j monorepo to v2.0.13 ([#2647](#2647)) ([f030e29](f030e29)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

JoeWang1127 added 7 commits March 26, 2024 13:22

feat: add a generation config change class

0a97d55

refact config comparator

233a105

fix unit tests

86d9111

add proto_path_utils.py

663bc06

refactor unit tests

f4bc0f1

refactor config comparator

501918b

fix unit tests

e1fc03d

product-auto-label bot added the size: l Pull request size is large. label Mar 26, 2024

JoeWang1127 added 9 commits March 26, 2024 14:08

format code

9598a1f

refactor utilities

9f8a95d

refactor proto_path_utils

59e02bb

add unit tests

4f58df7

add get_qualified_commits

8bed423

fix generate_pr_description.py

88da8d8

add unit tests

5fda1d7

add tests

14d419c

change test name

69715be

JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 26, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 26, 2024

JoeWang1127 marked this pull request as ready for review March 26, 2024 22:46

JoeWang1127 requested a review from a team as a code owner March 26, 2024 22:46

JoeWang1127 requested a review from diegomarquezp March 26, 2024 22:46

Merge branch 'main' into feat/add-config-change

ae5acb3

diegomarquezp reviewed Mar 28, 2024

View reviewed changes

JoeWang1127 added 6 commits March 28, 2024 15:44

use meaningful variable for commit sha

c032538

use variable rather than none

fc6fa0b

change helper method name

3bed10a

add method comment

50c1fa6

return unique and sored library names

0819266

add comment

4848d95

blakeli0 reviewed Apr 1, 2024

View reviewed changes

JoeWang1127 added 3 commits April 1, 2024 21:28

refactor get_proto_path_to_library_name to generation_config

f140c2b

move utilities.py to utils/utilities.py

ac8f0a9

refactor

9dd21a5

JoeWang1127 requested a review from blakeli0 April 2, 2024 14:29

blakeli0 reviewed Apr 2, 2024

View reviewed changes

blakeli0 approved these changes Apr 2, 2024

View reviewed changes

JoeWang1127 added 10 commits April 2, 2024 17:08

move utilities.sh to utils/utilities.sh

8e61a2b

refactor

067d06d

refactor

7d7ac09

refactor

37364d6

refactor

3e7bed8

use file path

c5d5540

format

f9bfed6

restore

4e18fff

change package

a784d06

explicitly declare shell scripts

cf58988

JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 2, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 2, 2024

JoeWang1127 enabled auto-merge (squash) April 2, 2024 18:53

JoeWang1127 merged commit 8312706 into main Apr 2, 2024
31 checks passed

JoeWang1127 deleted the feat/add-config-change branch April 2, 2024 19:13

release-please bot mentioned this pull request Apr 2, 2024

chore(main): release 2.39.0 #2594

Merged

JoeWang1127 mentioned this pull request Apr 3, 2024

feat: add entry point #2616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add config change #2604

feat: add config change #2604

JoeWang1127 commented Mar 26, 2024 •

edited

Loading

diegomarquezp Mar 28, 2024

JoeWang1127 Mar 28, 2024

diegomarquezp Mar 28, 2024

JoeWang1127 Mar 28, 2024

diegomarquezp Mar 28, 2024

JoeWang1127 Mar 28, 2024 •

edited

Loading

diegomarquezp Mar 28, 2024

JoeWang1127 Mar 28, 2024

diegomarquezp Mar 28, 2024

JoeWang1127 Mar 28, 2024

diegomarquezp Mar 28, 2024

JoeWang1127 Mar 28, 2024

diegomarquezp Mar 28, 2024

JoeWang1127 Mar 28, 2024

diegomarquezp Mar 28, 2024

JoeWang1127 Mar 28, 2024

blakeli0 Apr 1, 2024

JoeWang1127 Apr 2, 2024

blakeli0 Apr 2, 2024

JoeWang1127 Apr 2, 2024

JoeWang1127 Apr 2, 2024

blakeli0 Apr 2, 2024

JoeWang1127 Apr 2, 2024

blakeli0 left a comment

sonarcloud bot commented Apr 2, 2024

sonarcloud bot commented Apr 2, 2024

	def __get_library_name_from_qualified_commits(self) -> list[str]:
	def __get_library_names_from_qualified_commits(self) -> list[str]:

feat: add config change #2604

feat: add config change #2604

Conversation

JoeWang1127 commented Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeWang1127 Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 2, 2024

Quality Gate passed for 'gapic-generator-java-root'

sonarcloud bot commented Apr 2, 2024

Quality Gate passed for 'java_showcase_integration_tests'

JoeWang1127 commented Mar 26, 2024 •

edited

Loading

JoeWang1127 Mar 28, 2024 •

edited

Loading