Skip to content

Create branch on PR #636

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

Merged
merged 13 commits into from
Nov 17, 2022
Merged

Create branch on PR #636

merged 13 commits into from
Nov 17, 2022

Conversation

jmduarte
Copy link
Member

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.

  • Other (Specify): CI update

Tests

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

Checklist

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

@jmduarte jmduarte requested review from vloncar and thesps August 11, 2022 01:22
@jmitrevs
Copy link
Contributor

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?

@thesps
Copy link
Contributor

thesps commented Aug 26, 2022

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.

@jmduarte
Copy link
Member Author

jmduarte commented Nov 2, 2022

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 jmduarte self-assigned this Nov 2, 2022
@jmduarte
Copy link
Member Author

jmduarte commented Nov 2, 2022

@thesps @jmitrevs please review again

@thesps
Copy link
Contributor

thesps commented Nov 3, 2022

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.

@jmduarte
Copy link
Member Author

jmduarte commented Nov 9, 2022

@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: jmduarte#17

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

Copy link
Contributor

@thesps thesps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this way with the label 👍 GH said the branch needed to pull from main before merging, so I hit the button to do that

@jmduarte jmduarte merged commit 16ad946 into fastmachinelearning:main Nov 17, 2022
calad0i pushed a commit to calad0i/hls4ml that referenced this pull request Jul 1, 2023
* create branch on pr

* try to update

* try workflow_dispatch

* try again

* try again

* try again

* update

* try again

* try again

* update

* origin

* trigger on please test label

Co-authored-by: Sioni Summers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants