-
Notifications
You must be signed in to change notification settings - Fork 95
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
workflows: rebase code on pull_request_target #1622
Conversation
The e2e_on_pull workflow run on pull_request_target event that uses the the merge branch. Unlike the pull_request event, the merge branch is not rebased on top of the target branch (usually 'main'). In simple terms, the e2e_on_pull workflow does not automatically rebase the code so that changes merged on 'main' after the pull request creation are disregarded. This added a renamed (to ci-helper.sh) and modified version of https://github.com/kata-containers/kata-containers/blob/main/tests/git-helper.sh that's called after 'actions/checkout' step to rebase the code atop of 'main'. Because the touched workflows may be called on different workflows and events (e.g. release and dispatch_workflow), the rebase step is guarded by `if: github.event_name == 'pull_request_target'` to only run on pull_request_target triggered workflows. Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Tested in https://github.com/wainersm/cc-cloud-api-adaptor/actions/runs/7142483680 which is was triggered by dispatch_workflow, so the rebase step is skipped. |
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 looks generally good to me and matches the logic in kata-containers. I don't know whether we should add Fabiano and Choi's sign off as they wrote the original code in git-helper.sh that has been copied?
if [ -n "${GITHUB_WORKSPACE:-}" ] && [ "$(uname -m)" != "x86_64" ]; then | ||
echo "Rebase failed, cleaning up a repository for self-hosted runners and exiting" | ||
cd "${GITHUB_WORKSPACE}"/.. | ||
sudo rm -rf "${GITHUB_WORKSPACE}" |
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'm not sure we have this scenario anywhere at the moment? I'm not sure it is particularly harmful to leave in though
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.
Perhaps it would be safer to use cd "${GITHUB_WORKSPACE}"; sudo rm -rf .
, what do you think?
action="${1:-}" | ||
|
||
case "${action}" in | ||
rebase-atop-of-the-latest-target-branch) rebase_atop_of_the_latest_target_branch;; |
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.
nit: I think this case pattern should be indented for readability?
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 good to me and seems to be safe. I'd still prefer not going outside the workspace to delete it, but it's not a must have.
The e2e_on_pull workflow run on pull_request_target event that uses the the merge branch. Unlike the pull_request event, the merge branch is not rebased on top of the target branch (usually 'main'). In simple terms, the e2e_on_pull workflow does not automatically rebase the code so that changes merged on 'main' after the pull request creation are disregarded.
This added a renamed (to ci-helper.sh) and modified version of https://github.com/kata-containers/kata-containers/blob/main/tests/git-helper.sh that's called after 'actions/checkout' step to rebase the code atop of 'main'.
Because the touched workflows may be called on different workflows and events (e.g. release and dispatch_workflow), the rebase step is guarded by
if: github.event_name == 'pull_request_target'
to only run on pull_request_target triggered workflows.