-
Notifications
You must be signed in to change notification settings - Fork 243
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
Fix Cloning in CI for Forks #249
Conversation
.github/workflows/ci.yml
Outdated
@@ -59,7 +59,7 @@ jobs: | |||
apt-get update | |||
apt-get install -y git | |||
git config --global --add safe.directory '*' | |||
git clone "${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}" . | |||
git clone $(awk -v a=${{github.event.pull_request.head.repo.clone_url}} -v b=${{github.repositoryUrl}} 'BEGIN { if (a == "") { print b } else { print a } }') . |
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 you please add a comment?
Looking at the line, maybe 30-40 lines? Just to understand what is happening 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.
Yeah, it's really not nice and I don't really like it either.
The problem is that the repository referenced by the environment variables and contexts Github provides are always the one where the PR/push happens. In the case of forks, that's still our repo and not the forked one that gets cloned. Then, the branch providing the changes cannot be found there.
github.event.pull_request.head.repo.clone_url
is the URL of the repo referenced by the HEAD if the event that triggers the action is a PR. That's unset/empty if the event is not a PR, but a push to master (or another branch where we want to run the action). Thus, we check if we have that and if not, we simply clone from the "regular" repo Github gives us. I tried that with PRs from a fork and within our repo (where it worked), but I also want to test it with pushing to a branch in our repo.
I have no idea how to tell this in a nice way, but I guess it should also feature some links with information about these contexts, i.e., https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables and https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#github-context.
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.
Should now work for both scenarios: https://github.com/hyrise/sql-parser/actions/workflows/ci.yml
The first line is a PR within the base repository, the second one is a PR from a fork, and the third line is a push to a branch within the base repo (which was added to the branches where updates trigger action runs for this test).
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.
Nice work!
In #248, we noticed that manually retrieving the code in the gcc-6 action stage does not work for PRs from forks. This PR fixes this issue.
The problem is that in general, the repo referenced by GitHub's contexts and environment variables is the (target) repo where the PR is issued. For PRs from forks, this is still our base repo and not the fork. However, we can access a clone's URL from the HEAD information if the event triggering the action is a PR (update).
Still, we keep a version that checks out the main branch if the action is triggered by a branch update because we still want to run the CI workflow for updates on our master branch.
Some helpful documentation: