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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Question] How does check-dist handle dependabot PRs? #96

Closed
qoomon opened this issue Apr 9, 2024 · 3 comments
Closed

[Question] How does check-dist handle dependabot PRs? #96

qoomon opened this issue Apr 9, 2024 · 3 comments
Assignees

Comments

@qoomon
Copy link

qoomon commented Apr 9, 2024

Hi there,

I'm struggling to understand how the check-dist workflow handles dependabot pull requests in a different way as the comment is mentioning =>

# This will fail the workflow if the PR wasn't created by Dependabot.

Could you explain where the dependabot magic is happening?

@ncalteen
Copy link
Collaborator

Hmmm...I think you're correct. This comment doesn't make sense in the context of the script being run. There's nothing to indicate the PR author.

That does raise the question of how Dependabot PRs should be handled in general. Currently, the check-dist workflow will fail if a Dependabot update results in the transpiled JavaScript needing to be rebuilt. Doing some digging around other repos in this org, it seems that many of them either have the same workflow or call a reusable workflow that does the same thing. In either case, it looks like Dependabot PRs are still not really being handled, so developers would need to check out the Dependabot PR branch, run npm run all or npm run bundle, commit and push the rebuilt dist/ directory.

I'm sure there's a cleaner way to handle this. My initial thought is to update this workflow to check if the PR did originate from Dependabot and, if so, rebuild the action code. I'll dig into this a bit and let you know how it goes!

@ncalteen ncalteen self-assigned this Apr 12, 2024
@ncalteen
Copy link
Collaborator

I was able to hack together an update that would make this work, however it does require additional configuration that I feel may be out of scope for this repo. I'll add it in here though so it can be found by anyone who might need it :)

First, I would highly recommend not triggering this workflow on push events to main, as that could result in writing directly to your default branch. Similarly, the check-dist workflow will need additional permissions to be able to write to the repository.

name: Check Transpiled JavaScript

on:
  pull_request:

permissions:
  contents: write

Second, it is also important to check if the PR is originating from a fork. Otherwise, there are a bunch of potential security risks in letting a workflow run with write access to your repository contents. Though Dependabot doesn't work through fork PRs, it's a good idea to add as a safety check either way :) For example, you can set this as an environment variable using the following conditional statement.

env:
  # github.repository - This repository
  # github.event.pull_request.head.repo.full_name - The fork repository
  IS_FORK: ${{ github.repository != github.event.pull_request.head.repo.full_name }}

Combining these and a couple extra steps in the shell script, you can optionally commit the rebuilt dist/ directory on Dependabot PRs:

- name: Compare Directories
  id: diff
  env:
    # Check if this is a PR event (in case this workflow is being triggered on other events)
    IS_PR: ${{ github.event_name == 'pull_request' }}
    # Check if this is a fork PR
    IS_FORK: ${{ github.repository != github.event.pull_request.head.repo.full_name }}
  run: |
    echo "${{ github.event.pull_request.head.repo.full_name }}"
    echo "${{ github.repository }}"

    if [ "$(git diff --ignore-space-at-eol --text dist/ | wc -l)" -gt "0" ]; then
      echo "Detected uncommitted changes! See status below:"
      git diff --ignore-space-at-eol --text dist/

      # Fail the workflow on fork PRs where dist/ does not match
      if [ "$IS_FORK" == true]; then
        echo "Fork PR...Failing workflow due to uncommitted changes."
        exit 1
      fi

      # Fail the workflow run on non-PR events
      if [ "$IS_PR" == false ]; then
        echo "Non-PR Event...Failing workflow due to uncommitted changes."
        exit 1
      fi

      # Fail the workflow run on non-Dependabot PRs
      if [ "${{ github.actor }}" != "dependabot[bot]" ]; then
        echo "Non-Dependabot PR...Failing workflow due to uncommitted changes."
        exit 1
      fi

      # Commit and push dist/ on Dependabot PRs and pass the workflow run
      if [ "${{ github.actor }}" == "dependabot[bot]" ]; then
        echo "Dependabot Update...Committing changes to PR branch."
        git config --global user.email "<>"
        git config --global user.name "${{ github.actor }}"
        git add dist/
        git commit -m "Rebuild dist/ directory"
        git push origin $GITHUB_HEAD_REF
        exit 0
      fi

      # Catch-all
      exit 1
    fi

The above should work, however there is one big issue that will occur. Using the built-in GITHUB_TOKEN created for this workflow run will not trigger any additional events. For example, this repo's CI workflow runs on any updates to a PR branch. However, when this step runs and updates Dependabot's branch, the CI workflow will not be triggered. This is by design to prevent recursive workflow calls.

If you did want to re-trigger CI, you would need to use a personal access token or GitHub App token so that the event does not originate from the workflow token.

I hope this info helps! I know it was probably a long-winded non-answer to your question, but I hope the extra context helps. To answer directly, those comments are incorrect and I will remove them shortly ;)

@ncalteen
Copy link
Collaborator

ncalteen commented May 2, 2024

Heyo! I'm going to go ahead and close this issue out for now. If you have any questions please feel free to reopen!

@ncalteen ncalteen closed this as completed May 2, 2024
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

No branches or pull requests

2 participants