Check for open PRs on `push` GHA trigger

[jarojasm95]

Hello 👋

I've been successfully using this action for a couple weeks now (it is great 👏 ) - I am opening this issue to suggest an improvement:

When the action is run as part of a push it would be great if the action could lookup the open PR that the commit belongs to and post the PR comment there.

I think this could simplify the usage of this action by obviating the need for:

• pull_request trigger
• actions/upload-artifact@v3
• workflow_run trigger

(I am not suggesting that the above functionality is removed, only suggesting that we improve the push event handling for now, which could make this action easier to use imo)

What do you think? If this sounds like an acceptable improvement I would also be willing to work on it 🙂

[ewjoachim]

Hm, I could be wrong, but either you plan for only people who have write access to the repo to contribute, in which case you don't need the upload-artifact action and the workflow_run, or you plan for external people to contribute, and they won't be able to trigger the push action, would they ?

The only setup where this would help if I understand correctly would be if you only have write-access contributors, and have only set up your CI for push whatever the branch is and the CI would be the same otherwise whether we push to main or feature_branch_x so you'd like to plug the action as-is and let it determine if we should run in "default branch" mode or "PR" mode ? In this case, the problem might be solved by changing :

on:
  push:

to

on:
  pull_request:
  push:
    branches:
      - "main"

So if I understand correctly everything, you'd like to avoid doing the change above, and instead for the action to support PR mode on push. Right ? (I'm really only asking if I understood your problem correctly)

[jarojasm95]

So if I understand correctly everything, you'd like to avoid doing the change above, and instead for the action to support PR mode on push. Right ?

This is correct 👍

I would like to avoid adding pull_request trigger to my existing workflow primarily because that workflow takes a significant amount of time to complete, and having both triggers means that the workflow runs twice for new commits added to an open pull request

[jarojasm95]

Maybe also helpful context:

The repository where I use this is a private repo where all contributors have write access

[ewjoachim]

having both triggers means that the workflow runs twice for new commits added to an open pull request

If you look closely at the example I put above, you'll notice that when adding pull_request, I'm also restricting push to only happen on the main branch. So the CI only runs once when you push: either it's in a PR and it runs as part of the PR trigger, or you just merged a PR to main, which triggers the push. Of course, the idea is to put all your protected branches there.

[jarojasm95]

having both triggers means that the workflow runs twice for new commits added to an open pull request

If you look closely at the example I put above, you'll notice that when adding pull_request, I'm also restricting push to only happen on the main branch. So the CI only runs once when you push: either it's in a PR and it runs as part of the PR trigger, or you just merged a PR to main, which triggers the push. Of course, the idea is to put all your protected branches there.

Yep, I caught that, but then that means there's no CI run until a PR is opened.. which might not be ideal

Sorry if my use case seems too specific, I guess I am looking for a way to use this action to "build my own workflow" instead of adapting to the workflow suggested by this action.

[ewjoachim]

I was about to try and start working on that but...

If you push to a branch, the pull request may well not exist yet by the time the action finishes, so we can't post a comment to a non-existant PR ?

There's always the possibility of posting if there's a PR, and ignoring otherwise. Would that make a good UX ?

[jarojasm95]

I was about to try and start working on that but...

If you push to a branch, the pull request may well not exist yet by the time the action finishes, so we can't post a comment to a non-existant PR ?

There's always the possibility of posting if there's a PR, and ignoring otherwise. Would that make a good UX ?

My "workaround" for that right now is the following:

name: CI

on:
  push:
    branches:
      - "**"

jobs:
  # run all the lengthy and expensive tests and checks
  ########
  reports:
    name: reports
    runs-on: [self-hosted]
    permissions:
      pull-requests: write
      contents: write
      checks: write
    if: always()
    needs: init
    steps:
      - name: coverage data
        uses: py-cov-action/python-coverage-comment-action@v3
        with:
          GITHUB_TOKEN: ${{ github.token }}
      ###########
      # upload coverage file to cloud storage

then on PR:

name: PR

on:
  pull_request:
    branches-ignore:
      - dependabot/**

jobs:
  reports:
    name: reports
    runs-on: [self-hosted]
    needs: [init]
    permissions:
      checks: write
      contents: read
      pull-requests: write
    steps:
      # download coverage assuming ci job is done
      ####
      - name: coverage data
        uses: py-cov-action/python-coverage-comment-action@v3
        with:
          GITHUB_TOKEN: ${{ github.token }}
          ANNOTATE_MISSING_LINES: true

[ewjoachim]

Ok ! If I were to handle this ticket, what would be your proposed workflow in case the action runs on push, not on the default branch, and there's no PR yet ?

And what would happen if we subsequently openned a PR ?

From the conversation, my current understanding is that:

• You'd like the action to work when only configured on push
• You'd like a comment to still be posted if the PR is openned after the push workflow has ended

As far as I can tell, those 2 statements are incompatible.

[jarojasm95]

My proposed workflow:

• if the action is run on push (non default branch)
  • if a PR is open, post the PR comment and annotations, etc
  • if no PR is open, don't do anything (same as what is already on main I think)
• if the actions is run on pull_request
  • post the PR comment and annotations, etc (same as what is already on main I think)

[kieferro]

My proposed workflow

The problem with this is the following scenario: You push to the feature branch (the action does nothing) and then open a PR without pushing again. In order for the action to react to this and to post a comment, it needs to be used with a pull_request trigger.
So you can't do without a pull_request trigger because otherwise you won't cover all of the important events.

[ewjoachim]

Summarizing:

I could be wrong but I think @jarojasm95 acknowledged that if no PR is open at push time, then there's nothing we can (nor should) do to have a comment posted once the PR is openned, unless the action is also configured to run on pull_request.

@jarojasm95 mentioned the ability to not having to configure the pull_request trigger as a benefit, but it's still necessary to have it unless you're ok with PRs not having the comment (at least until they are re-pushed after the PR was opened).

Though I do understand that you may want your own CI steps to run on the first push and not wait until a PR is opened to start running, which leads to the workaround @jarojasm95 posted where the CI runs on push, the action on pull_request, and we need to somehow pass the coverage results between those steps.

The problem with that workaround is what happens if the PR is opened right away before the CI finished running. Which is exactly where their proposal makes sense: if the action could post the comment on a push event, then it would work on all cases except when on the initial push where the PR doesn't exist. If the user wants this (pretty important) case covered, they'd need to put a similar workaround where they would store the coverage data, and add a "pull resquest created" workflow just for this case. But at least the problem of this workaround highlighted above would be solved.

I think we can support it without it being too complicated, but it will be such a mess to use (due to having to setup the workaround) that I'm not even sure we'd want to advertise this in the doc, or at least more than a couple lines pointing to this ticket.

[kieferro]

Ah okay, I think I understand it now. So in the non-default-branch push event we would store the comment as an artifact and in the PR event we would check if such an artifact already exists and use that if it does. Do I understand that correctly?

[ewjoachim]

So in the non-default-branch push event we would store the comment as an artifact and in the PR event we would check if such an artifact already exists and use that if it does.

Ah, I was thinking about something much more direct: when running in push event, but not on the default branch, if there's a PR open on that branch, we'd run as if we were in pull_request event. But you're right that it would make sense to push the comment as an artifact in that situation. Though I don't think it's worth it to have this action publish the comment from the artifact in PR mode: this can be done pretty easily with a dedicated workflow using gh that we can document here.

[ewjoachim]

@jarojasm95 Would you mind testing the PR linked above and tell us if it works as expected ? When it runs on a push event that's not on the default branch, ils tries to find the associated PR. If found, it acts as if it had been launched on the PR itself. If not found, it posts the PR comment as an artifact and leaves.