Skip to content
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

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

wainersm
Copy link
Member

@wainersm wainersm commented Dec 8, 2023

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.

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>
@wainersm wainersm added the CI Issues related to CI workflows label Dec 8, 2023
@wainersm
Copy link
Member Author

wainersm commented Dec 8, 2023

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.
And tests in https://github.com/wainersm/cc-cloud-api-adaptor/actions/runs/7142242126 where the rebases happened.

Copy link
Member

@stevenhorsman stevenhorsman left a 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?

Comment on lines +22 to +25
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}"
Copy link
Member

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

Copy link
Contributor

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;;
Copy link
Member

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?

Copy link
Contributor

@ldoktor ldoktor left a 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.

@wainersm wainersm merged commit 75516c6 into confidential-containers:main Dec 12, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issues related to CI workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants