hls4ml icon indicating copy to clipboard operation
hls4ml copied to clipboard

Create branch on PR

Open jmduarte opened this issue 2 years ago • 2 comments

Description

Currently, our GitLab PyTests do not run on PRs created from forks due to the configuration of GitLab CI (i.e. the commit has to exist in our repo). The purpose of this PR is to push the head of a PR branch (especially those made from forks) to a branch in this repo.

This PR adds two GitHub Action workflows

  • When a new PR is opened, a branch is created called pr/#, where # is the PR number
  • When a PR is updated, the head of the PR branch is pushed to the (hopefully existing) branch pr/#

Type of change

For a new feature or function, please create an issue first to discuss it with us before submitting a pull request.

  • [x] Other (Specify): CI update

Tests

Tested on https://github.com/jmduarte/hls4ml

Checklist

  • [x] I have read the guidelines for contributing.
  • [x] I have commented my code, particularly in hard-to-understand areas.
  • [x] I have made corresponding changes to the documentation.
  • [x] My changes generate no new warnings.
  • [x] I have added tests that prove my fix is effective or that my feature works.B

jmduarte avatar Aug 11 '22 01:08 jmduarte

This looks very useful to me. I have a few questions:

  • Do you understand why the test is failing?
  • What about the transient time, after we merge this, before all the /pr/* branches exist, will this cause errors? If so, is the solution to manually add the /pr/* branches to all currently open branches or to add a check?
  • Do we want to limit this to only be done if the PR is from a different fork? It doesn't seem necessary to do it if it's not from a fork, but maybe there are no problems doing it.
  • Do we want to delete the branches automatically when we accept or close a PR so that we don't wind up with lots of branches?

jmitrevs avatar Aug 25 '22 22:08 jmitrevs

I like this, but I think we may still need some manual action involved just as a protection. I've seen examples where a specific comment like "run tests" by an admin in the PR chat triggers the workflow, or there are these.

thesps avatar Aug 26 '22 08:08 thesps

After some extensive testing I think the current configuration does the right thing:

  • If you raise or update a PR from the same repo, the action runs but it does nothing
  • If you raise a PR from a fork of the repo, the action runs and it creates a pr/# branch
  • If you update a PR from a fork of the repo, the action runs and it updates the pr/# branch

Tests in my fork: https://github.com/jmduarte/hls4ml/pulls

This needs to be merged into main to take effect on new/open PRs. This is because we're using on: pull_request_target instead of on: pull_request, which means the workflow runs from the base repo instead of the head repo to ensure we have permission to push to our own (base) repo.

The one caveat is if a PR from a fork updates the GitHub Actions workflows, then this action will fail because GITHUB_TOKEN won't have the right permissions to update workflows. I think this is actually sensible for security reasons. If this happens, we can just push the branch ourselves.

jmduarte avatar Nov 02 '22 05:11 jmduarte

@thesps @jmitrevs please review again

jmduarte avatar Nov 02 '22 05:11 jmduarte

We still need some manual component to the action, just a click button or anything to trigger the making of the branch. That’s just to have some oversight to prevent arbitrary code being run on the CI setup.

thesps avatar Nov 03 '22 11:11 thesps

@thesps I updated it so the action will only be triggered if an admin (or actually, anyone with triage access to the repo) adds the label please test to the PR.

Does that work for you?

You can see it in action here: https://github.com/jmduarte/hls4ml/pull/17

Note this won't work in our repo until this is merged.

jmduarte avatar Nov 09 '22 03:11 jmduarte