search

solvaholic / octodns-sync

GitHub Action to test and deploy DNS settings with OctoDNS
MIT License
28 stars 14 forks source link

solvaholic/octodns-sync checks refs/pull/NNN/merge rather than head #51

Closed solvaholic closed 3 years ago

solvaholic commented 3 years ago

Description

I think when this action is triggered on the pull_request event it's checking the config in refs/pull/NNN/merge rather than refs/pull/NNN/head.

Expected Behavior

When this action adds a comment to a pull request, I expect the commit SHA it names to match the last commit pushed to the pull request head ref.

Actual Behavior

The commit this action mentions in the PR comment may differ from the most recent one pushed to the PR head:

image

Possible Fix

Steps to Reproduce

  1. Tell solvaholic/octodns-sync to add a pull request comment
  2. Trigger solvaholic/octodns-sync on the pull_request event
  3. Notice the comment added to PR names refs/pull/NNN/merge rather than refs/pull/NNN/head

Context

The mismatch can be scary until you realize what it's checking.

In case it matters: The PR where I noticed this, its base is NOT the default branch in the repository.

Your Environment

solvaholic commented 3 years ago

Is this just an issue with the workflow definition?

https://github.com/solvaholic/octodns-sync/issues/41#issuecomment-780041826

The initial example here was in a private repository. I'll try it out in a public one so I can link it.

solvaholic commented 3 years ago

Confirmed using a workflow in scaling-succotash the github.ref and github.sha in the synchronize event payload are the pull request merge commit.

Here's how I named the job step:

      - name: Checkout ${{ github.repository }} at ${{ github.ref }} / ${{ github.sha }}

Here's how that looked in the workflow run log:

image

Here's the comment it added to the pull request:

image

solvaholic commented 3 years ago

I set up solvaholic/scaling-succotash to test this:

Telling the workflow to checkout refs/pull/NNN/head got it to check out the pull request head:

      - name: Checkout refs/pull/${{ github.event.number }}/head
        uses: actions/checkout@v2
        with:
          ref: refs/pull/${{ github.event.number }}/head

But the comment still named the merge commit SHA because comment.sh assumed the user checked out GITHUB_SHA:

https://github.com/solvaholic/octodns-sync/blob/e34e8fba3f49ca16b517bf8bcfca7a02863a429e/scripts/comment.sh#L18

I think that's easy to fix, at least, by setting $_sha to the output of git log -1 --format='%h'.

solvaholic commented 3 years ago

That did it: https://github.com/solvaholic/scaling-succotash/pull/15#issuecomment-836871827

So, this:

solvaholic/octodns-sync checks refs/pull/NNN/merge rather than head

Will happen when a) solvaholic/octodns-sync is used in a workflow triggered on a pull request synchronize and b) it runs in a checkout at GITHUB_REF or GITHUB_SHA. This happens because the synchronize event payload provides the pull request merge ref as its ref and sha.

In terms of which version of your code is being checked, this may be significant. refs/pull/NNN/merge should simply add the pull request changes to the pull request base branch. If the pull request base has changed in the meantime, however, then other files this Action uses may be different in /merge than they are in /head.

Consider, for example, if your pull request changes my.domain.yaml and in the meantime some other change pushed to main changed public.yaml, which uses my.domain.yaml. Would you want to run octodns-sync using the main version of public.yaml? or the pull request version?

In any case, refs/pull/NNN/merge should look exactly like what you'd get if you merge the pull request. And refs/pull/NNN/head may not.

To run solvaholic/octodns-sync on the pull request head ref, add this to the with: clause of the preceding actions/checkout step:

        with:
          ref: refs/pull/${{ github.event.number }}/head

Now that I understand more about why this is happening I'll close this issue and ignore the apparent discrepancy.

Ideas to address this issue in solvaholic/octodns-sync: