ganga
ganga copied to clipboard
Dependencies and tests taken from develop branch
With the recent changes in the testing framework to allow tests to run for branches provided by external users after approval, the setup.py
file (defining dependencies) and the ci_push_testing.yml
file (defining how the tests are done) are taken from the branch at the time it was created, and not from the updated code. This works fine when fixing bugs, but when defining new features it is painful as they often involve new dependencies or changes in the testing framework. Two solutions to this could be investigated.
- Make it such that for PRs owned by a registered user, it uses the files from the updated branch (so if I am submitting a PR, it behaves as in the past).
- Somehow make the approval step (which is either implicit or explicit) update the files. As it is already reading the
ci_push_testing.yml
this sounds tricky.
My suggestion is to modify the CI configuration file to use the latest commit from the pull request branch. This approach aligns well with the workflow and ensures that CI runs on the most updated files. However, updating files during the approval stage can be complex and risky. Can I take over this issue if @alexanderrichards isn't working on it?
The problem is we explicitly check out the pull_request_target
. This is so that we can use the repo secrets in forked PRs. By the time is has done this and start the action it's then too late to update the action. We can't simply run from the latest PR branch as GitHub will not allow secret sharing for forked branches. This is why we do it this way in the first place.
Tests defined in GangaTest should be updatable fine, the problem really is in updating the ci_push_testing.yml
file itself as it's already run.
The best approach is probably the first thing that @ulrik suggests. I think we can essentially run things as they are for external forks and possibly drop back to running the old ci_push_testing.yml
for internal forks. This may mean we need two .yml files or maybe put both in one. I'm not sure which would work best without trying things out.
Thanks for clarifying, @alexanderrichards. Let's use the current configuration for external forks and try a separate one for internal forks. I'm open to submitting a pull request.
please do