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

Fix Cloning in CI for Forks #249

Merged
merged 24 commits into from
Sep 12, 2024
Merged

Fix Cloning in CI for Forks #249

merged 24 commits into from
Sep 12, 2024

Conversation

dey4ss
Copy link
Member

@dey4ss dey4ss commented Sep 10, 2024

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:

@dey4ss dey4ss changed the title [WIP] Fix Cloning in CI for forks Fix Cloning in CI for Forks Sep 10, 2024
@@ -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 } }') .
Copy link
Contributor

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.

Copy link
Member Author

@dey4ss dey4ss Sep 11, 2024

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.

Copy link
Member Author

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

image

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).

Copy link
Contributor

@Bouncner Bouncner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@dey4ss dey4ss merged commit 9a67d24 into hyrise:master Sep 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants