-
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: get PR description from googleapis commits #2531
Changes from 16 commits
eb93ac3
df8fe50
eb0da6b
9a1be4c
c1f556c
ccb1a21
057da91
da3fc4c
1e6ab5e
a1c786f
d128a2d
939c513
7079327
a08a866
b77aa1d
eecaba0
259c4ef
f09fb3b
5e1d85e
7e9be85
9f3bbca
8324776
cc9088e
334298b
aabd9c1
176871d
93e60de
d4d16f6
a279549
1946d64
be189c5
188a632
5d7c915
240038f
33df40b
cd7f639
1715714
6cf0045
b473438
2b507aa
2ce846a
c03d6dd
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 |
---|---|---|
@@ -0,0 +1,86 @@ | ||
#!/usr/bin/env python3 | ||
# Copyright 2024 Google LLC | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# 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 click | ||
|
||
from library_generation.model.generation_config import from_yaml | ||
from library_generation.utilities import get_commit_messages | ||
from library_generation.utilities import get_file_paths | ||
|
||
|
||
@click.group(invoke_without_command=False) | ||
@click.pass_context | ||
@click.version_option(message="%(version)s") | ||
def main(ctx): | ||
pass | ||
|
||
|
||
@main.command() | ||
@click.option( | ||
"--generation-config-yaml", | ||
required=True, | ||
type=str, | ||
help=""" | ||
Path to generation_config.yaml that contains the metadata about | ||
library generation. | ||
""", | ||
) | ||
@click.option( | ||
"--baseline-commit", | ||
required=True, | ||
type=str, | ||
help=""" | ||
The baseline commit from which the commit message is considered. | ||
This commit should be an ancestor of googleapis commit in configuration. | ||
""", | ||
) | ||
@click.option( | ||
"--repo-url", | ||
type=str, | ||
default="https://github.com/googleapis/googleapis.git", | ||
show_default=True, | ||
help=""" | ||
GitHub repository URL. | ||
""", | ||
) | ||
def generate( | ||
generation_config_yaml: str, | ||
repo_url: str, | ||
baseline_commit: str, | ||
) -> str: | ||
return generate_pr_descriptions( | ||
generation_config_yaml=generation_config_yaml, | ||
repo_url=repo_url, | ||
baseline_commit=baseline_commit, | ||
) | ||
|
||
|
||
def generate_pr_descriptions( | ||
generation_config_yaml: str, | ||
repo_url: str, | ||
baseline_commit: str, | ||
) -> str: | ||
config = from_yaml(generation_config_yaml) | ||
paths = get_file_paths(config) | ||
return get_commit_messages( | ||
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 we add another commit for the generator version update? This is currently done as 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. Generator updates will not use GitHub action as it only happens once in two weeks. I'll setup a meeting to discuss this issue. 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. Added in the PR description. |
||
repo_url=repo_url, | ||
latest_commit=config.googleapis_commitish, | ||
baseline_commit=baseline_commit, | ||
paths=paths, | ||
) | ||
|
||
|
||
if __name__ == "__main__": | ||
main() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,12 +17,14 @@ | |
import unittest | ||
from distutils.dir_util import copy_tree | ||
from distutils.file_util import copy_file | ||
from filecmp import cmp | ||
from filecmp import dircmp | ||
|
||
from git import Repo | ||
from pathlib import Path | ||
from typing import List | ||
from typing import Dict | ||
|
||
from library_generation.generate_pr_description import generate_pr_descriptions | ||
from library_generation.generate_repo import generate_from_yaml | ||
from library_generation.model.generation_config import from_yaml, GenerationConfig | ||
from library_generation.test.compare_poms import compare_xml | ||
|
@@ -49,6 +51,28 @@ | |
|
||
|
||
class IntegrationTest(unittest.TestCase): | ||
def test_get_commit_message_success(self): | ||
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 added this as an integration test because it required network connection for git clone and checkout of googleapis repository. |
||
repo_url = "https://github.com/googleapis/googleapis.git" | ||
baseline_commit = "a17d4caf184b050d50cacf2b0d579ce72c31ce74" | ||
description = generate_pr_descriptions( | ||
f"{config_dir}/google-cloud-java/{config_name}", | ||
JoeWang1127 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
repo_url=repo_url, | ||
baseline_commit=baseline_commit, | ||
) | ||
description_file = f"{config_dir}/pr-description.txt" | ||
if os.path.isfile(f"{description_file}"): | ||
os.remove(f"{description_file}") | ||
with open(f"{description_file}", "w+") as f: | ||
f.write(description) | ||
# There are five commits from googleapis commitish to baseline commit, | ||
# inclusively. Only two of the commits contain changes in proto_path | ||
# that are in configuration. Therefore, only two commit messages should | ||
# be included in the PR description. | ||
self.assertTrue( | ||
cmp(f"{config_dir}/pr-description-golden.txt", f"{description_file}") | ||
) | ||
os.remove(f"{description_file}") | ||
|
||
def test_generate_repo(self): | ||
shutil.rmtree(f"{golden_dir}", ignore_errors=True) | ||
os.makedirs(f"{golden_dir}", exist_ok=True) | ||
|
@@ -150,7 +174,7 @@ def __pull_repo_to(cls, default_dest: Path, repo: str, committish: str) -> str: | |
repo = Repo(dest) | ||
else: | ||
dest = default_dest | ||
repo_dest = f"{golden_dir}/{repo}" | ||
shutil.rmtree(dest, ignore_errors=True) | ||
repo_url = f"{repo_prefix}/{repo}" | ||
print(f"Cloning repository {repo_url}") | ||
repo = Repo.clone_from(repo_url, dest) | ||
|
@@ -169,6 +193,8 @@ def __get_library_names_from_config(cls, config: GenerationConfig) -> List[str]: | |
def __get_config_files(cls, path: str) -> List[tuple[str, str]]: | ||
config_files = [] | ||
for sub_dir in Path(path).resolve().iterdir(): | ||
if sub_dir.is_file(): | ||
continue | ||
repo = sub_dir.name | ||
if repo == "golden": | ||
continue | ||
|
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. All the commits description should be wrapped between So in this example, the golden files should at least include
And we can add any other metadata info before 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. Done. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
7a9a855287b5042410c93e5a510f40efd4ce6cb1 | ||
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 don't have the logic for telling exactly which library is affected, but maybe in the meantime we can format this commit hash to be an actual link to the commit in 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. Cool, that's a good idea. I'll create a follow up PR to format the commit message. 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 think this is a good idea, but we don't want to include them in the release notes, so please put it outside of 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. Done. The commit link is outside of |
||
feat: expose model_type in v1 processor, so that user can see the model_type after get or list processor version | ||
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 we include the service name in the PR description? Something like
Similar to what we are doing today for the title of the owlbot PR, e.g. googleapis/google-cloud-java#10422. 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 can do it. What are the benefit of doing so and what happens if a commit has multiple service names (generally one commit should only have changes in one service, but we don't have guarantee that). 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. The benefit is that release notes would be much clearer, see the latest release notes. In my experience, I never seen a commit affect multiple services, maybe a service and a common proto. I don't think we have to worry about it right now, but in case it happens, let's not fail the process and just pick the first service name. 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. Done. |
||
|
||
PiperOrigin-RevId: 603727585 | ||
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 think we can remove this in the PR description. 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. This is part of the commit message and it is included in Owlbot PR. 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. It is, but I don't think they provide much value and we don't want the release notes to include this message, can we try to parse the commit message and remove this part? Again, take a look at the existing release notes, we should try to make our PR description as close as possible. In addition, if we really want to keep the existing commit history, we can put it outside of the 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. How about I only keep the first line of each commit message? The first line is usually the commit summary. I've changed the golden file, PTAL. |
||
|
||
|
||
c7fd8bd652ac690ca84f485014f70b52eef7cb9e | ||
feat: add model_type in v1beta3 processor proto | ||
|
||
PiperOrigin-RevId: 603726122 |
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.
Is baseline-commit inclusive?
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.
No.
The baseline commit is included in the previous generation.
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 please add comments to make it clear?
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.
Changed the comments.