-
Notifications
You must be signed in to change notification settings - Fork 52
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
chore: add a composite action for library generation #3173
Conversation
.github/scripts/action.yaml
Outdated
@@ -0,0 +1,51 @@ | |||
name: Copy library generation script | |||
description: Copy library generation shell script to the calling 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.
description: Copy library generation shell script to the calling workflow | |
description: Copies library generation shell script to the calling workflow |
Just a quick nit: In general, descriptions should not be imperative statements but rather assertions of what the file/object/etc does.
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 updated the name and description.
.github/scripts/action.yaml
Outdated
run: | | ||
if [[ "${GITHUB_REPOSITORY}" != "${REPO_FULL_NAME}" ]]; then | ||
echo "This PR comes from a fork. Skip library generation." | ||
exit 0 |
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 it possible that this exit 0
just means for the runner that the step finished successfully?
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.
Do you think we should fail this step?
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 any way of stopping the workflow without throwing an error would work fine. I added this comment because I think this exit 0
will be considered as a successful step and the next step would then start. Maybe using GITHUB_OUTPUT
along with if
in the following steps may do the job (SO)?
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 added a separate job to determine whether to run the library generation.
If the pull request comes from a fork, the generation will be skipped.
The following error occurs if I removed the local install.
|
.github/scripts/action.yaml
Outdated
description: the tag of hermetic build image | ||
required: false | ||
token: | ||
description: PAT |
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 is PAT
?
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.
Personal access token.
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 use the full name instead of abbreviations since it's not common? See style guide.
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 to full name.
# run hermetic code generation docker image. | ||
docker run \ | ||
--rm \ | ||
-u "$(id -u):$(id -g)" \ | ||
-v "$(pwd):${workspace_name}" \ | ||
-v "$HOME"/.m2:/home/.m2 \ | ||
-e GENERATOR_VERSION="${generator_version}" \ | ||
-v "${m2_folder}":/home/.m2 \ |
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.
So .m2 is still needed?
fetch-depth: 0 | ||
token: ${{ secrets.CLOUD_JAVA_BOT_TOKEN }} | ||
- name: Generate changed libraries | ||
- name: Determine whether the pull request comes from a fork |
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 keep this step to prevent this workflow runs in a fork.
The workflow will fail if it's a fork and it can prevent us from setting this workflow as required, e.g., renovate PR will be blocked by the failure.
Quality Gate passed for 'gapic-generator-java-root'Issues Measures |
Quality Gate passed for 'java_showcase_integration_tests'Issues Measures |
echo "This PR comes from a fork. Skip library generation." | ||
echo "SHOULD_RUN=false" >> $GITHUB_ENV | ||
else | ||
echo "SHOULD_RUN=true" >> $GITHUB_ENV |
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 there a way to exit the workflow early so we don't have to pass this env to all the subsequent steps?
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.
Looks like another way of skipping steps is creating a separate job and make the generation job depends on that, just like what we've done in unit/integration tests.
I'll keep it as-is now.
In this PR: - Add a composite action to execute library generation shell script. - Separate repo specific logic, e.g., installing modules, building images, etc., in the generation shell script. An example workflow in google-cloud-java: googleapis/google-cloud-java#11117
In this PR:
An example workflow in google-cloud-java: googleapis/google-cloud-java#11117