-
Notifications
You must be signed in to change notification settings - Fork 61
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: generate pr description from configuration comparison result #2841
Conversation
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.
Thanks for the PR. Just a few nits + 1 comment.
@@ -0,0 +1,18 @@ | |||
BEGIN_COMMIT_OVERRIDE | |||
BEGIN_NESTED_COMMIT | |||
Update repo-level parameter gapic_generator_version to 1.2.3 |
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 we change this to something like fix(deps): Update the Java code generator (gapic-generator-java) to 2.39.0
to keep it consistent with the existing Owlbot behavior? See googleapis/java-bigtable#2191
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.
We need a mapping to do this, from parameter name to description.
What's the benefits of doing this? I think we don't have to generate the same message as long as the meaning of this message is 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.
What's the benefits of doing this? I think we don't have to generate the same message as long as the meaning of this message is clear.
I agree. But I think the previous message makes more sense for handwritten library owners. Because
- It includes a conventional commit prefix.
- They don't need to know something like
repo-level parameter
, it is an implementation detail of hermetic build scripts. On high level, they just need to know the generator version is updated and the GAPIC layer is regenerated.
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.
Sure, this does make sense.
I need to create a mapping from parameter to commit message and I'll only add generator version and libraries_bom version for now. Other parameter will fallback to default message.
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.
Update repo-level parameter libraries_bom_version to 2.3.4 | ||
END_NESTED_COMMIT | ||
BEGIN_NESTED_COMMIT | ||
Update repo-level parameter protoc_version to 3.4.5 |
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.
We should remove protoc_version from our test proto now.
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.
This is just for testing a optional parameter change can appear in the message.
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.
Yes, but using protoc_version might also confuse other people. Can you use googleapis_commitish instead? Or use something random to indicate that the current logic can be used for any repo level changes?
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 use googleapis_commitish instead?
googleapis commit will not appear here because the commits are formatted to separate messages.
I'll remove this message.
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
library_generation/test/resources/goldens/pr_description-golden.txt
Outdated
Show resolved
Hide resolved
|
|
Downstream check failed in java-spanner-jdbc, but I don't think this related to this PR.
|
…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](googleapis/googleapis@3b6f144) (exclusive) and [googleapis/googleapis@0cea717](googleapis/googleapis@0cea717) (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](googleapis/googleapis@0cea717) END_NESTED_COMMIT END_COMMIT_OVERRIDE ```
In this PR:
An example of pr description with repo-level change: