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

Dependencies and tests taken from develop branch #2253

Open
egede opened this issue Jan 9, 2024 · 4 comments
Open

Dependencies and tests taken from develop branch #2253

egede opened this issue Jan 9, 2024 · 4 comments

Comments

@egede
Copy link
Member

egede commented Jan 9, 2024

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.
@samadpls
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

please do

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

No branches or pull requests

3 participants