feat: parse `BUILD.bzel` to determine whether a commit that only changed `BUILD.bazel` is a qualified commit #2937

JoeWang1127 · 2024-06-22T21:35:15Z

In this PR:

For a commit that only changed BUILD.bzel, parse the BUILD.bazel to determine whether this is a qualified commit. The commit is a qualified commit if the two Gapic_Inputs objects parsed from the commit and its parent are different.

diegomarquezp

Thank you. Just a few small nits.

diegomarquezp · 2024-06-24T20:14:02Z

library_generation/model/config_change.py

@@ -156,3 +170,30 @@ def __create_qualified_commit(
        if len(libraries) == 0:
            return None
        return QualifiedCommit(commit=commit, libraries=libraries)
+
+    @staticmethod
+    def __qualified_build_change(commit: Commit, build_file_path: str) -> bool:


readability nit: __is_qualified_build_change may help to quickly understand the check in line 159.

diegomarquezp · 2024-06-24T20:14:05Z

library_generation/model/config_change.py

+    def __qualified_build_change(commit: Commit, build_file_path: str) -> bool:
+        """
+        Checks if the given commit containing a BUILD.bazel change is a
+        qualified commit.


Can you add another sentence explaining that the function parses the gapic_inputs from both versions to tell if there is a relevant change?

diegomarquezp · 2024-06-24T20:24:32Z

library_generation/model/config_change.py

            versioned_proto_path = find_versioned_proto_path(file)
            if versioned_proto_path in proto_paths:
+                # determine a commit that only contains `BUILD.bazel`


I think the if statement this comment refers to will skip the commit if the only affected file in the commit is the BUILD file and doesn't contain relevant changes. Can you please explain that in the comment?

blakeli0

Is there going to be another PR to actually creates the PR description or it should be already taken care of by existing logics?

blakeli0 · 2024-06-24T22:34:33Z

library_generation/model/config_change.py

+                # generation, e.g., transport, rest_numeric_enum.
+                if (
+                    file.endswith("BUILD.bazel")
+                    and file_change_num == 1


What if a commit contains changes to multiple BUILD.bazel files? It is a common use case for multi-module library.

Good catch, I need to make some changes when a commit contains multiple BUILD.bazel.

Done.

I changed the logic to qualify commits that adding BUILD.bazel because it's unlikely that a commit added a BUILD.bazel without proto change.

Commits that changed BUILD.bazel will be determined by parsing BUILD.bazel.

blakeli0 · 2024-06-24T22:50:31Z

library_generation/model/config_change.py

+        """
+        versioned_proto_path = find_versioned_proto_path(build_file_path)
+        build = str((commit.tree / build_file_path).data_stream.read())
+        parent_commit = commit.parents[0]


I know we are getting it from commit.parents, but conceptually I guess this is the last/previous commit?

Maybe.

However, there might be multiple commits changed a BUILD.bazel between baseline and current commit.

there might be multiple commits changed a BUILD.bazel between baseline and current commit

Can you elaborate on this with an example?

Can you elaborate on this with an example?

I don't have a concrete example but now I understand your original question.

Yes, the parent commit contains the last commit touching this file.

JoeWang1127 · 2024-06-24T22:55:30Z

Is there going to be another PR to actually creates the PR description or it should be already taken care of by existing logics?

The existing logic should take care of creating pr description as long as the commit is marked as a qualified commit.

code change after approval

blakeli0 · 2024-06-25T20:17:29Z

library_generation/test/model/config_change_unit_tests.py

@@ -208,6 +208,29 @@ def test_get_qualified_commits_success(self):
            qualified_commits[2].commit.hexsha,
        )

+    def test_get_qualified_commits_build_only_commit_returns_a_commit(self):


It's not clear to me that what this test is testing just based on the name, I guess this is for the basic scenario that a commit contains rest_numeric_enums changes in BUILD.bazel?

I changed the test name to test_get_qualified_commits_rest_numeric_enum_change_returns_a_qualified_commit. WDYT?

blakeli0 · 2024-06-25T20:18:49Z

library_generation/test/model/config_change_unit_tests.py

        self.assertTrue(len(config_change.get_qualified_commits()) == 0)

+    def test_get_qualified_commits_new_client_commit_returns_a_commit(self):


Same question here, the test name is not clear. Also since we are using real commits for the tests, it makes the problem worse that it's not easy for developers to see the test data, but that's a different problem to solve.

I changed the test name to test_get_qualified_commits_add_build_file_returns_a_qualified_commit. WDYT?

blakeli0 · 2024-06-25T20:21:46Z

library_generation/model/config_change.py

            versioned_proto_path = find_versioned_proto_path(file)
            if versioned_proto_path in proto_paths:
+                if (
+                    file.endswith("BUILD.bazel")


Why do we use endswith instead of ==? Any special cases we need to handle?

The file is a relative path starting from google, e.g., google/cloud/aiplatform/v1/BUILD.bazel.

Makes sense. A nit improvement, rename file to file_path, I thought it is a file_name that is only BUILD.bazel.

blakeli0 · 2024-06-25T21:36:48Z

library_generation/model/config_change.py

+        """
+        versioned_proto_path = find_versioned_proto_path(build_file_path)
+        build = str((commit.tree / build_file_path).data_stream.read())
+        parent_commit = commit.parents[0]


there might be multiple commits changed a BUILD.bazel between baseline and current commit

Can you elaborate on this with an example?

blakeli0 · 2024-06-26T01:59:36Z

library_generation/model/config_change.py

            versioned_proto_path = find_versioned_proto_path(file)
            if versioned_proto_path in proto_paths:
+                if (
+                    file.endswith("BUILD.bazel")


Makes sense. A nit improvement, rename file to file_path, I thought it is a file_name that is only BUILD.bazel.

sonarqubecloud · 2024-06-26T12:24:55Z

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

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud · 2024-06-26T12:27:56Z

Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

🤖 I have created a release *beep* *boop* --- <details><summary>2.43.0</summary> ## [2.43.0](v2.42.0...v2.43.0) (2024-07-25) ### Features * add `transport` option to `generation_config.yaml` ([#3052](#3052)) ([3b1a915](3b1a915)) * get released version from versions.txt to render `README.md` ([#3007](#3007)) ([99bb2b3](99bb2b3)) * Introduce java.time to Gax-Java ([#1872](#1872)) ([308aeaf](308aeaf)) * Mark `getDefaultEndpoint()` with @ObsoleteApi ([#2347](#2347)) ([e46648f](e46648f)) * parse `BUILD.bzel` to determine whether a commit that only changed `BUILD.bazel` is a qualified commit ([#2937](#2937)) ([502f801](502f801)) ### Bug Fixes * Fix: ([d996c2d](d996c2d)) * `BaseApiTracer` to noop on attemptFailed via overloaded method call ([#3016](#3016)) ([2fc938a](2fc938a)) * Generator to skip generation for empty services. ([#3051](#3051)) ([ff2c485](ff2c485)) * restore hermetic build image publication ([#2952](#2952)) ([97a6d67](97a6d67)) ### Dependencies * update dependency com.fasterxml.jackson:jackson-bom to v2.17.2 ([#3028](#3028)) ([d16f9d1](d16f9d1)) * update dependency com.google.cloud.opentelemetry:detector-resources-support to v0.30.0 ([#2975](#2975)) ([b3ec93f](b3ec93f)) * update dependency com.google.cloud.opentelemetry:detector-resources-support to v0.31.0 ([#3044](#3044)) ([6bd07dc](6bd07dc)) * update dependency com.google.errorprone:error_prone_annotations to v2.29.2 ([#3058](#3058)) ([8ea0868](8ea0868)) * update dependency com.google.errorprone:error_prone_annotations to v2.29.2 ([#3059](#3059)) ([81b23dc](81b23dc)) * update dependency com.google.guava:guava to v33.2.1-jre ([#3027](#3027)) ([12ee456](12ee456)) * update dependency commons-codec:commons-codec to v1.17.1 ([#3049](#3049)) ([58d94b7](58d94b7)) * update dependency dev.cel:cel to v0.6.0 ([#3050](#3050)) ([bc332d9](bc332d9)) * update dependency net.bytebuddy:byte-buddy to v1.14.18 ([#3029](#3029)) ([8799cf6](8799cf6)) * update dependency org.apache.commons:commons-lang3 to v3.15.0 ([#3060](#3060)) ([2538334](2538334)) * update dependency org.checkerframework:checker-qual to v3.45.0 ([#2988](#2988)) ([4edd216](4edd216)) * update google api dependencies ([#2951](#2951)) ([c16f6c9](c16f6c9)) * update google auth library dependencies to v1.24.0 ([#3039](#3039)) ([98b5bd7](98b5bd7)) * update googleapis/java-cloud-bom digest to 47c5dbc ([#2974](#2974)) ([57623f0](57623f0)) * update grpc dependencies to v1.65.1 ([#3061](#3061)) ([27497e2](27497e2)) * update junit5 monorepo to v5.10.3 ([#2963](#2963)) ([bc55fe1](bc55fe1)) * update netty dependencies to v4.1.112.final ([#3057](#3057)) ([5af127b](5af127b)) * update opentelemetry-java monorepo to v1.40.0 ([#3035](#3035)) ([5c31c42](5c31c42)) * Use Gapic-Showcase v0.35.1 ([#3018](#3018)) ([43773f0](43773f0)) ### Documentation * add support option to 'new issue' choices ([#3055](#3055)) ([6a2a17d](6a2a17d)) </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 2 commits June 22, 2024 17:33

feat: parse BUILD.bazel to find qualified commit

a49b574

add unit test

378ae55

product-auto-label bot added the size: m Pull request size is medium. label Jun 22, 2024

JoeWang1127 added 7 commits June 22, 2024 17:35

Merge branch 'main' into feat/parse-build-in-commit

eb6674d

add comments

22208fa

early return for a new client commit

c2907bc

Merge branch 'main' into feat/parse-build-in-commit

db56ad8

keep the new client commit

23e66d5

only consider commit that only changed BUILD.bazel

2a5cf36

Merge branch 'main' into feat/parse-build-in-commit

1052244

JoeWang1127 changed the title ~~feat: parse BUILD.bzel to find qualified commit~~ feat: parse BUILD.bzel to determine whether a commit that only changed BUILD.bzel is a qualified commit Jun 24, 2024

JoeWang1127 changed the title ~~feat: parse BUILD.bzel to determine whether a commit that only changed BUILD.bzel is a qualified commit~~ feat: parse BUILD.bzel to determine whether a commit that only changed BUILD.bazel is a qualified commit Jun 24, 2024

JoeWang1127 marked this pull request as ready for review June 24, 2024 19:49

JoeWang1127 requested a review from a team as a code owner June 24, 2024 19:49

JoeWang1127 requested review from blakeli0 and diegomarquezp June 24, 2024 19:50

diegomarquezp previously approved these changes Jun 24, 2024

View reviewed changes

JoeWang1127 added 2 commits June 24, 2024 17:09

change comments

c7ab267

Merge branch 'main' into feat/parse-build-in-commit

c98066d

blakeli0 reviewed Jun 24, 2024

View reviewed changes

JoeWang1127 added 2 commits June 24, 2024 20:37

qualify a commit that adding BUILD.bazel

06176fa

Merge branch 'main' into feat/parse-build-in-commit

8228434

JoeWang1127 requested review from diegomarquezp and blakeli0 June 25, 2024 00:52

JoeWang1127 added 2 commits June 24, 2024 21:02

Merge branch 'main' into feat/parse-build-in-commit

d4128c4

Merge branch 'main' into feat/parse-build-in-commit

1e73898

blakeli0 reviewed Jun 25, 2024

View reviewed changes

change test name

00e96e4

Merge branch 'main' into feat/parse-build-in-commit

0ebbf03

JoeWang1127 requested a review from blakeli0 June 25, 2024 22:00

blakeli0 approved these changes Jun 26, 2024

View reviewed changes

refactor

fd7318f

diegomarquezp approved these changes Jun 26, 2024

View reviewed changes

JoeWang1127 merged commit 502f801 into main Jun 26, 2024
34 checks passed

JoeWang1127 deleted the feat/parse-build-in-commit branch June 26, 2024 13:51

release-please bot mentioned this pull request Jun 26, 2024

chore(main): release 2.43.0 #2953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: parse `BUILD.bzel` to determine whether a commit that only changed `BUILD.bazel` is a qualified commit #2937

feat: parse `BUILD.bzel` to determine whether a commit that only changed `BUILD.bazel` is a qualified commit #2937

JoeWang1127 commented Jun 22, 2024 •

edited

Loading

diegomarquezp left a comment

diegomarquezp Jun 24, 2024

JoeWang1127 Jun 24, 2024

diegomarquezp Jun 24, 2024

JoeWang1127 Jun 24, 2024

diegomarquezp Jun 24, 2024

JoeWang1127 Jun 24, 2024

blakeli0 left a comment

blakeli0 Jun 24, 2024

JoeWang1127 Jun 24, 2024

JoeWang1127 Jun 25, 2024

blakeli0 Jun 24, 2024

JoeWang1127 Jun 25, 2024

blakeli0 Jun 25, 2024

JoeWang1127 Jun 25, 2024

JoeWang1127 commented Jun 24, 2024

blakeli0 Jun 25, 2024

JoeWang1127 Jun 25, 2024

blakeli0 Jun 25, 2024

JoeWang1127 Jun 25, 2024

blakeli0 Jun 25, 2024

JoeWang1127 Jun 25, 2024

blakeli0 Jun 26, 2024

blakeli0 Jun 25, 2024

blakeli0 Jun 26, 2024

sonarqubecloud bot commented Jun 26, 2024

sonarqubecloud bot commented Jun 26, 2024

		self.assertTrue(len(config_change.get_qualified_commits()) == 0)

		def test_get_qualified_commits_new_client_commit_returns_a_commit(self):

feat: parse BUILD.bzel to determine whether a commit that only changed BUILD.bazel is a qualified commit #2937

feat: parse BUILD.bzel to determine whether a commit that only changed BUILD.bazel is a qualified commit #2937

Conversation

JoeWang1127 commented Jun 22, 2024 • edited Loading

diegomarquezp left a 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

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

JoeWang1127 commented Jun 24, 2024

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

sonarqubecloud bot commented Jun 26, 2024

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

sonarqubecloud bot commented Jun 26, 2024

Quality Gate passed for 'java_showcase_integration_tests'

feat: parse `BUILD.bzel` to determine whether a commit that only changed `BUILD.bazel` is a qualified commit #2937

feat: parse `BUILD.bzel` to determine whether a commit that only changed `BUILD.bazel` is a qualified commit #2937

JoeWang1127 commented Jun 22, 2024 •

edited

Loading