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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support workflow_dispatch Event Context #45

Closed
jhoffmcd opened this issue Jun 24, 2021 · 8 comments 路 Fixed by #51
Closed

Support workflow_dispatch Event Context #45

jhoffmcd opened this issue Jun 24, 2021 · 8 comments 路 Fixed by #51

Comments

@jhoffmcd
Copy link
Contributor

馃殌 Feature Proposal

Support the workflow_dispatch as an acceptable event context to run this action.

Motivation

It looks like this only supports the pull_request event context here. In my case, my CI runs on a different provider which I can't change out right now. I would like to initiate a workflow_dispatch via API when my CI has passed so that the auto-merge action can then take effect.

workflow_dispatch

Example

# actions file
name: Dependabot Auto Merge
on: workflow_dispatch

jobs:
  automerge:
    runs-on: ubuntu-latest
    steps:
      - uses: fastify/github-action-merge-dependabot@v2.1.1
        with:
          github-token: ${{ secrets.TOKEN }}
# script
curl -X POST \
  -H "Accept: application/vnd.github.v3+json" \
  -H "Authorization: token ${PAT}" \
  https://api.github.com/repos/{ownder}/{repo}/actions/workflows/{workflow_id}/dispatches \
  -d '{"ref":"ref"}'
@zekth
Copy link
Member

zekth commented Jun 25, 2021

Meanwhile this issue is legit i'm wondering how you can wait for all your CI runs if it's not in the same workflow file. Do you have an example?
For this we need to add a check for the ref branch and find if there is a PR associated to it in prior. Would you want to work on this feature and write a PR for it?

@simoneb
Copy link
Collaborator

simoneb commented Jul 2, 2021

The feature request is legitimate, we need to pay special attention to the attack surface. If you look into how this action works, which is also described here, we're working around limitations in permissions of the GH token by delegating the merge to an external GH app. Hence, we need to be extra careful which PRs that application is capable of merging.

With that being said, I don't see this feature request necessarily impacting the attach surface. The action would have to be changed to accept a PR number, which could come from anywhere, including workflow_dispatch trigger inputs. If that is not provided, the current behavior is preserved.

The syntax would then look something like:

name: automerge

on: 
  workflow_dispatch:
    inputs:
      pr:
        required: true

jobs:
  automerge:
    runs-on: ubuntu-latest
    steps:
      - uses: fastify/github-action-merge-dependabot@v2.1.1
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}
          pr: ${{ github.event.inputs.pr }}

@jhoffmcd
Copy link
Contributor Author

jhoffmcd commented Jul 3, 2021

I will try to make time to take a look at making this change. Our CI provider is CircleCI, I believe I can derive the PR number from a job triggered by a pull request.

@jhoffmcd
Copy link
Contributor Author

jhoffmcd commented Jul 4, 2021

I am testing this a bit, and in addition to the PR number, we would also need to derive these details:

  • user (currently pulled from pr.user.login)
  • package name (currently pulled from pr.head.ref)

I can easily get the ref in the workflow event, it's in github.context.payload.ref. Seems like the user (the initiator of the PR that triggers everything) should also be sent as input from CI though. Does this seem correct?

name: automerge

on: 
  workflow_dispatch:
    inputs:
      pr:
        required: true
      user:
        required: true

jobs:
  automerge:
    runs-on: ubuntu-latest
    steps:
      - uses: fastify/github-action-merge-dependabot@v2.1.1
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}
          pr: ${{ github.event.inputs.pr }}
          user: ${{ github.event.inputs.user }}

@zekth
Copy link
Member

zekth commented Jul 4, 2021

Wouldn't it be better that the user resolution will done using the PR number?
Isn't the github.actor populated with the good user?

@jhoffmcd
Copy link
Contributor Author

jhoffmcd commented Jul 4, 2021

The user value is used in the code to determine isDependabotPR. I think you could also derive this from the ref based on a pattern match to what we know dependabot branch names look like. So another option is to not use user at all.

For my tests, when I am doing a workflow_dispatch via API, it looks like actor is going to be the owner of the generated personal access token, so in my case, jhoffmcd.

@simoneb
Copy link
Collaborator

simoneb commented Jul 4, 2021

While thinking about and working on this, please always consider that any user provided input is a possible attack vector, hence we want to minimize it. As I described earlier, I don't think that anything other than the PR number is necessary

@jhoffmcd
Copy link
Contributor Author

jhoffmcd commented Jul 5, 2021

After taking a look at what the code is checking for, I thought it would be easier just to fetch the pull request data from the supplied input. That way we still only need the PR number provided by the CI/script that initiates the manual workflow request. All the other functionality should remain the same.

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 a pull request may close this issue.

3 participants