-
Notifications
You must be signed in to change notification settings - Fork 89
chore: update hermetic_library_generation.yaml to only run on PRs that modify generation_config.yaml #2625
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
Conversation
…t modify generation_config.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.
The underlying script already does this check. We need the workflow to run when the last commit has affected generation_config.yaml. Otherwise we would have a loop (generation_config -> source code + generation_config -> source code + generation_config)
java-storage/.github/scripts/hermetic_library_generation.sh
Lines 72 to 76 in a038b69
| change_of_last_commit="$(git diff-tree --no-commit-id --name-only HEAD~1..HEAD -r)" | |
| if [[ ! ("${change_of_last_commit}" == *"${generation_config}"*) ]]; then | |
| echo "The last commit doesn't contain any changes to the generation_config.yaml, skipping the whole generation process." || true | |
| exit 0 | |
| fi |
We may expand this check to allow other files being modified, such as the OwlBot yaml, or even some more sophisticated logic.
cc: @JoeWang1127
|
I acknowledge that the script detects this, but github will also fail if the action is present on a branch that does not have a token. My preference would be that the workflow for the generation script not even attempt to access the token until a "write step", but the current script is all or nothing. Filtering on input change seems like an okay compromise. RE:
Could this be a check in the script |
|
@BenWhitehead, we have disabled the check on PRs that come from fork (i.e. repos that don't have the token).
For all other PRs, the token should be accessible since it's a repo-level secret. Regarding the screenshot you shared, would you mind sharing the link to it to see which PR it comes from? I suspect it's either a workflow run from before we set the token or from a fork. |
|
Spoke offline to get consensus. |

No description provided.