-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: create pr after library generation #10503
Conversation
run: | | ||
title="chore: generate libraries at $(date)" | ||
git add java-* pom.xml gapic-libraries-bom/pom.xml versions.txt generation_config.yaml | ||
git commit -m "chore: ${title}" |
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.
"chore" appears twice.
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.
Updated the commit message.
- name: create or update the pull request | ||
shell: bash | ||
run: | | ||
title="chore: generate libraries at $(date)" |
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.
My instinct says "chore:" does not goes to Release Please pull request and the pull request description is not read by Release Please. Let's see.
(If this is really the case, we can fix it by "BEGIN_COMMIT_OVERRIDE".)
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.
I feel the same way. Can we make sure we understand the behavior before merging? e.g. If it is a chore
, is release please going to include the NESTE_COMMIT in the description? If it is a feat
, is the title going to be included? The behavior of release-please will affect how we generate the PR descriptions.
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.
According to @chingor13, release-please won't skip a commit if it's a chore, it just ignores chore commit messages.
Is this PR an example of the generated PR? |
Yes. |
The content is the same, thus no impact on build. |
- name: create or update the pull request | ||
shell: bash | ||
run: | | ||
title="chore: generate libraries at $(date)" |
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.
I feel the same way. Can we make sure we understand the behavior before merging? e.g. If it is a chore
, is release please going to include the NESTE_COMMIT in the description? If it is a feat
, is the title going to be included? The behavior of release-please will affect how we generate the PR descriptions.
git fetch -q --unshallow monorepo | ||
git push -f monorepo "${branch_name}" | ||
set -x | ||
gh pr create --title "${title}" --label "owlbot:run" --head "${branch_name}" --body "$(cat pr_description.txt)" |
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.
Why do we still need owlbot:run
here?
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.
The check is not removed yet.
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.
I think we should remove the label here and disable the check for this branch, otherwise owlbot postprocessing would still be run against this PR right?
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.
+1 to removing the required check for owlbot postprocessor and the label
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.
@@ -30,3 +72,40 @@ jobs: | |||
-e "REPO_BINDING_VOLUMES=${repo_volumes}" \ | |||
gcr.io/cloud-devrel-public-resources/java-library-generation:latest \ |
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.
Ideally we should not use latest
, but rather a versioned docker image, otherwise it is not hermetic. But since this is the first time we use the scripts and there might be bug fixes in this cycle, let's keep it for now. Once the scripts are stable, we should read the gapic-generator-java version from the build config file and use that version for the docker image.
For now, can we extract latest
as a parameter and share it between this step and the step below?
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.
Just FYI, we have a release job that releases gcr.io/cloud-devrel-public-resources/java-library-generation:${GGJ_VERSION}
, so if we are to remove it, we can use that tag (and configure renovate bot)
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.
Once the scripts are stable, we should read the gapic-generator-java version from the build config file and use that version for the docker image.
I think we should group the docker image tag with generator update so that renovate bot can update them together.
For now, can we extract latest as a parameter and share it between this step and the step below?
Sounds good.
generate-from-configuration: | ||
runs-on: ubuntu-22.04 | ||
env: | ||
branch_name: generate-libraries |
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 could be a follow up PR. Can we make this a parameter so that we can test the Github action in a separate branch? Otherwise it would always override the current PR if it is not merged.
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.
Otherwise it would always override the current PR if it is not merged
This is an intended behavior because we don't want a lot of open PRs in the repo.
This is the description of the workflow.
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.
In addition, can we change the branch name to include the branch name, something like generate-libraries-main
? As we would need to support LTS and multiple Java versions in the future.
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.
Otherwise it would always override the current PR if it is not merged
This is an intended behavior because we don't want a lot of open PRs in the repo.
This is the description of the workflow.
I agree with this behavior. What I'm suggesting is to make it more flexible so that we can test the Github action without overriding the PR.
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.
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.
LGTM. I think this PR should be a chore
as it does not really change any code, otherwise it would appear in the release notes.
A few follow up enhancements that I can think of:
- Make this Github action more generic so that it can be reused for other use cases. e.g. Extract the target branch, googleapis commitish to parameters, do not always update googleapis commitish etc.
- Encapsulate the PR creation logic in the docker image. So that it can be tested independently and more hermetic.
- Create a separate Github action or reuse this Github action to automatically update renovate bot PRs for gapic-generator-java version update.
generate-from-configuration: | ||
runs-on: ubuntu-22.04 | ||
env: | ||
branch_name: generate-libraries |
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.
In addition, can we change the branch name to include the branch name, something like generate-libraries-main
? As we would need to support LTS and multiple Java versions in the future.
git fetch -q --unshallow monorepo | ||
git push -f monorepo "${branch_name}" | ||
set -x | ||
gh pr create --title "${title}" --head "${branch_name}" --body "$(cat pr_description.txt)" |
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.
I guess this creates a PR against main branch by default? Can we make the target branch a parameter as well?
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.
- name: create or update the pull request | ||
shell: bash | ||
run: | | ||
title="chore: generate libraries at $(date)" |
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.
The title generation could be moved to sdk-platform-java and/or inside the docker images.
env: | ||
branch_name: generate-libraries | ||
library_generation_image_tag: latest | ||
repo_volumes: "-v repo-google-cloud-java:/workspace/google-cloud-java" |
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.
Anything that references google-cloud-java
should be extracted to a parameter, so that we can reuse this Github action in handwritten library repos.
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.
I'll leave this one as-is and we'll refactor it once we prove this is working on google-cloud-java.
@@ -167,6 +167,14 @@ libraries: | |||
GAPICs: | |||
- proto_path: google/appengine/v1 | |||
|
|||
- api_shortname: apphub |
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.
Which script do you use to populate this yaml?
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.
I manually added these libraries, the parameters are copied from new library generation PR.
In this PR:
generation_config.yaml
.generation_config.yaml
.generate-libraries
.generation_config.yaml
.generation_config.yaml
to include new libraries and new versions.Context: this is the launch plan of hermetic build project.