-
Notifications
You must be signed in to change notification settings - Fork 53
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: parse BUILD.bzel
to determine whether a commit that only changed BUILD.bazel
is a qualified commit
#2937
Changes from all commits
a49b574
378ae55
eb6674d
22208fa
c2907bc
db56ad8
23e66d5
2a5cf36
1052244
c7ab267
c98066d
06176fa
8228434
d4128c4
1e73898
00e96e4
0ebbf03
fd7318f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,16 @@ | |
from enum import Enum | ||
from typing import Optional | ||
from git import Commit, Repo | ||
|
||
from library_generation.model.gapic_inputs import parse_build_str | ||
from library_generation.model.generation_config import GenerationConfig | ||
from library_generation.model.library_config import LibraryConfig | ||
from library_generation.utils.utilities import sh_util | ||
from library_generation.utils.proto_path_utils import find_versioned_proto_path | ||
|
||
INSERTIONS = "insertions" | ||
LINES = "lines" | ||
|
||
|
||
class ChangeType(Enum): | ||
GOOGLEAPIS_COMMIT = 1 | ||
|
@@ -77,6 +82,7 @@ def get_changed_libraries(self) -> Optional[list[str]]: | |
Returns a unique, sorted list of library name of changed libraries. | ||
None if there is a repository level change, which means all libraries | ||
in the current_config will be generated. | ||
|
||
:return: library names of change libraries. | ||
""" | ||
if ChangeType.REPO_LEVEL_CHANGE in self.change_to_libraries: | ||
|
@@ -143,11 +149,23 @@ def __create_qualified_commit( | |
:return: qualified commits. | ||
""" | ||
libraries = set() | ||
for file in commit.stats.files.keys(): | ||
if file.endswith("BUILD.bazel"): | ||
continue | ||
versioned_proto_path = find_versioned_proto_path(file) | ||
for file_path, changes in commit.stats.files.items(): | ||
versioned_proto_path = find_versioned_proto_path(file_path) | ||
if versioned_proto_path in proto_paths: | ||
if ( | ||
file_path.endswith("BUILD.bazel") | ||
# Qualify a commit if the commit only added BUILD.bazel | ||
# because it's very unlikely that a commit added BUILD.bazel | ||
# without adding proto files. Therefore, the commit is | ||
# qualified duo to the proto change eventually. | ||
and (not ConfigChange.__is_added(changes)) | ||
and ( | ||
not ConfigChange.__is_qualified_build_change( | ||
commit=commit, build_file_path=file_path | ||
) | ||
) | ||
): | ||
continue | ||
# Even though a commit usually only changes one | ||
# library, we don't want to miss generating a | ||
# library because the commit may change multiple | ||
|
@@ -156,3 +174,30 @@ def __create_qualified_commit( | |
if len(libraries) == 0: | ||
return None | ||
return QualifiedCommit(commit=commit, libraries=libraries) | ||
|
||
@staticmethod | ||
def __is_added(changes: dict[str, int]) -> bool: | ||
return changes[INSERTIONS] == changes[LINES] | ||
|
||
@staticmethod | ||
def __is_qualified_build_change(commit: Commit, build_file_path: str) -> bool: | ||
""" | ||
Checks if the given commit containing a BUILD.bazel change is a | ||
qualified commit. | ||
|
||
The commit is a qualified commit if the | ||
:class:`library_generation.model.gapic_inputs.GapicInputs` objects | ||
parsed from the commit and its parent are different, since there are | ||
changes in fields that used in library generation. | ||
|
||
:param commit: a GitHub commit object. | ||
:param build_file_path: the path of the BUILD.bazel | ||
:return: True if the commit is a qualified commit; False otherwise. | ||
""" | ||
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 was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know we are getting it from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe. However, there might be multiple commits changed a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on this with an example? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
parent_build = str((parent_commit.tree / build_file_path).data_stream.read()) | ||
inputs = parse_build_str(build, versioned_proto_path) | ||
parent_inputs = parse_build_str(parent_build, versioned_proto_path) | ||
return inputs != parent_inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add another sentence explaining that the function parses the
gapic_inputs
from both versions to tell if there is a relevant change?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.