ganga-devs / ganga

Ganga is an easy-to-use frontend for job definition and management
GNU General Public License v3.0
100 stars 159 forks source link

Dependencies and tests taken from develop branch #2253

Open egede opened 10 months ago

egede commented 10 months ago

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.

samadpls commented 9 months ago

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?

alexanderrichards commented 9 months ago

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.

samadpls commented 9 months ago

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.

alexanderrichards commented 9 months ago

please do