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

Checking out from forks in the system tests #580

Closed
MakisH opened this issue Nov 4, 2024 · 2 comments · Fixed by #638
Closed

Checking out from forks in the system tests #580

MakisH opened this issue Nov 4, 2024 · 2 comments · Fixed by #638

Comments

@MakisH
Copy link
Member

MakisH commented Nov 4, 2024

We currently require that the commits/tags we are testing are all in the original, specified repositories. This is clearly restricting, and we could extend the system tests infrastructure to cover that as well.

@fsimonis
Copy link
Member

You may be able to use github pull ref to fetch the necessary commits before calling checkout.

git fetch upstream pull/2216/head

@MakisH
Copy link
Member Author

MakisH commented Mar 25, 2025

GitHub pull ref

@fsimonis I did not know that one could get the pull ref from the repository itself, thanks!

I see that we can also get the number of a PR in workflow: ${{ github.event.number }}.

Current workflow:

  • Get the PRECICE_REF (a commit hash), from either:
    • A manual workflow execution input list
    • A job that gets the latest components
    • A direct user input
  • Pass it to the arguments list of the systemtest.py
  • Pass it as an environment variable to the Dockerfile
  • In the Dockerfile, git clone https://github.com/precice/precice.git precice and then git checkout ${PRECICE_REF}

What I am thinking to implement on top:

  • For the workflow that is triggered by a PR (only, so we can assume it)
  • Add a workflow variable PRECICE_PR, that takes its value from ${{ github.event.number }}
  • The systemtest.py doesn't even store this variable, so does not need any modifications
  • In the Dockerfile, check if the variable is defined, and then do a git fetch origin pull/${PRECICE_PR}/head.

I think this should work.

Security implications

One reason I did not want to rush this before was because we should first understand some security implications.

However, now that runs on GitHub can be triggered only by people with permissions (by adding a label or triggering a workflow explicitly), I think we can accept running code from forks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants