Initial review

[minrk]

This is great!

Some notes from looking at the action:

• Use force-push so repeated action triggers don't fail #17 the branch is pushed to test-this-pr/$NUMBER. If a PR is force-updated and then the test comment is issued again, I think this push will fail unless it's done with --force
• Use the merge ref instead of adding the forked repo as a remote #18 I believe there is already a merge ref available so you can avoid fetching both full repos and doing the merge yourself: git fetch origin pull/$NUMBER/merge:test-this-pr/$NUMBER will fetch the existing merge ref and create the local branch test-this-pr/$NUMBER.
• Provide a link to GitHub Actions instead of branch #19 The comment at the end provides a link to the tree view of the new branch. Since this is for testing, it might make sense to link directly to the Actions page for that branch, e.g. https://github.com/binderhub-test-org/pr-test/actions?query=branch%3Amore-tests
• Should the test-this-pr branch be deleted automatically on any particular event? E.g. merge/close of the parent PR?

I still don't know if it will work best to 'just' test the branch, or open local-branch PRs. WDYT so far after trying it out?

My initial thoughts:

• pro PR: you get all the nice checks UI, easier view of success/failure
• pro just branch: two PRs for the same change might get confusing. Which one do we merge? What happens when they update? If they are in-sync, merging either one should close both, but it's not easy to see if/when they are out of sync.
• pro just branch: It seems like the checks/test conditions to avoid too many redundant tests might be easier for this case? I'm not sure.

[sgibson91]

Thank you for this! Great stuff to work on here.

Should the test-this-pr branch be deleted automatically on any particular event? E.g. merge/close of the parent PR?

This to me sounds like an optional feature so that the project maintainers can use their judgment. Although looking at the API docs, I can't easily find anything that'll close a PR, only merge one. So that might throw a spanner in the works this feature, and would be a point in favour for the "just branch" approach.

Edit: I think I meant this this other way around, but I'm also not sure I easily see an API endpoint to delete a branch either, but surely that's possible?

• I believe there is already a merge ref available so you can avoid fetching both full repos and doing the merge yourself: git fetch origin pull/$NUMBER/merge:test-this-pr/$NUMBER will fetch the existing merge ref and create the local branch test-this-pr/$NUMBER.

This is good to know as at the minute, I'm stuck on git auth to clone the repo 😄 So this would solve the "needing the fork as a remote" problem. I know some actions require checkout of the repo before hand, so if we could do that and somehow pass the checkout to the Docker container, that'd be helpful. And I think the built-in GITHUB_TOKEN would have enough permissions to push.

Otherwise, I think we'd need to specifically create a PAT and store it as a repository secret, which feels like it circumvents the whole point of this action 😄

[minrk]

not sure I easily see an API endpoint to delete a branch

Weirdly it seems you have to use the git API to delete branches. So maybe if you have push access already, deleting via git push will be simplest: git push origin :test-this-pr/number

[sgibson91]

Should the test-this-pr branch be deleted automatically on any particular event?

On reflection I'm thinking if we do implement this, it wouldn't be in this repo built into this action. I think it's a whole other action that has probably already been implemented somewhere else.

For example in mybinder.org-deploy, we could have another job that looks something like the below.

jobs:
  ...
  delete-test-branch:
    # Only runs on pushes to test-this-pr/** branches
    if: (github.event_name == 'push') && (contains(github.ref, 'test-this-pr'))
    # Requires the deploy to staging job to succeed before this job runs
    needs: staging-deploy
    runs-on: ubuntu-latest
    steps:
      - run: |
          # Some code to delete the branch

Or there are some options that, e.g., sweep and delete merged and stale branches https://github.com/marketplace/actions/git-sweep-merged-and-stale