create-pull-request icon indicating copy to clipboard operation
create-pull-request copied to clipboard

Action run via cron leads to some unexpected behaviour

Open jrfnl opened this issue 3 years ago • 1 comments
trafficstars

Description

First of all: thank you for this action!

I'm using the action in a workflow which runs every night via a cron job and checks whether a new version of an externally hosted file, of which the repo contains a copy, has changed and if so, creates a PR to update the copy in the repo.

So far, so good. Works like a charm.

Having said that, I'm seeing some unexpected behaviour in very specific circumstances. Nothing blocking or anything, just little annoyances, which might be fixable, which is why I'm writing these up.

I suspect some of these are closely related and fixing one (2), may also fix one of the others (1).

I also suspect that some of the things I'm seeing may be tied into the GH system itself. Even so, you are probably better positioned to evaluate that.

Note: for the issues I'm describing, no commits were made to the repo between when the generated PR was first created and "now", which make this a different issue compared to #1062.

1. Review requests

In the workflow, we have the reviewers key set.

Expected behaviour

  • Reviewers get automatically notified with a "review request" when the PR gets first created and that's it.

Actual behaviour

  • Reviewers get a "review request" every single time the cron job runs while the PR is open, even when the PR has not been changed.
  • Reviewers get notified, even when they have already approved the PR. Think: two reviewers, one has approved the PR when it first got opened, the other hasn't yet. Both get a new "review request" notification email every night.

2. PR gets "updated" even when there is no change

In the body of the PR message, a date is used, which is intended to be the date when the change to the external file was first detected, i.e. the date when the auto-generated pull request first gets opened.

The actual commit and the commit message remain the same between workflow runs.

Expected behaviour

Based on this part of the documentation:

If a pull request already exists and there are no further changes (i.e. no diff with the current pull request branch) then the action exits silently.

... I'd expect that the action would determine that there are no code changes compared to when the PR was first opened and then bow out,

Actual behaviour

  • Even though the commit is unchanged, it gets (force) pushed again.
  • The PR description is changed every night, updating the date in the description, which now makes the date incorrect for our purposes.

3. Multiple PRs for same branch, different base

Aside from the cron job, the workflow is also run on PRs when the PR triggering the workflow contains a change to the workflow file (to verify that the workflow still runs correctly after the change) and if the PR contains a change to the local file being monitored (to prevent malicious PRs being merged as this is a security related file).

When run via the cron job, a PR gets opened against the develop branch. When the workflow run is triggered for a PR, the new PR gets opened against the branch which triggered the workflow.

Expected behaviour

When a branch already exists with an associated PR against develop, a new branch would be created (with a random number suffix or something) for the auto-generated PR against the workflow-triggering PR-branch.

The second branch would be based on the PR-branch.

Actual behaviour

The original auto-generated PR branch get rebased to the workflow-triggering PR-branch and a new PR gets opened against the workflow-triggering PR-branch (so now there are two PRs for the same auto-generated branch, but with different base branches and the original PR now also contains the commits from the workflow-triggering PR-branch due to the rebase).

Then on the nightly cron job, that same auto-generated branch gets rebased again on develop and both PRs get updated, with the second PR no longer being based on the workflow-triggering PR-branch.

👉🏻 This is very much an edge-case and I should probably solve this by using a different branch name for PRs created via PR triggered workflow runs.

Steps to reproduce

The actual workflow can be seen here: https://github.com/WordPress/Requests/blob/develop/.github/workflows/update-cacert.yml

Essential parts of it as related to the above described behaviour:

on:
  schedule:
    - cron: '20 4 * * *'
  pull_request:
    paths:
      - '.github/workflows/update-cacert.yml'

jobs:
  check:
    runs-on: ubuntu-latest
    steps:
      - name: Determine branches to use
        id: branches
        env:
          HEAD_REF: ${{ github.head_ref }}
        run: |
          if [[ "${{ github.event_name }}" == 'schedule' ]]; then
            echo "::set-output name=BASE::develop"
          else # = PR
            echo "::set-output name=BASE::$HEAD_REF"
          fi

      - name: Checkout code
        uses: actions/checkout@v2

      # Do the actual check which _may_ or _may not_ result in a change to the repo.

      - name: "Get date"
        id: get-date
        run: echo "::set-output name=DATE::$(/bin/date -u "+%F")"

      - name: Create pull request
        uses: peter-evans/create-pull-request@v3
        with:
          base: ${{ steps.branches.outputs.BASE }}
          branch: "feature/auto-update-cacert"
          delete-branch: true
          commit-message: ":lock_with_ink_pen: Update certificate bundle"
          title: ":lock_with_ink_pen: Update certificate bundle"
          body: |
            Updated certificate bundle as of ${{ steps.get-date.outputs.DATE }}.

            Source: https://curl.se/docs/caextract.html

            This PR is auto-generated by [create-pull-request](https://github.com/peter-evans/create-pull-request) using the `.github/workflows/update-cacert.yml` workflow.
          labels: |
            Type: enhancement
          reviewers: jrfnl

jrfnl avatar Mar 23 '22 14:03 jrfnl

Hi @jrfnl

Thank you for describing these in detail.

For the behaviour outlined in point 1, I might be able to do something about that. Basically, I'm calling GitHub's API to update the reviewers regardless of whether they have changed or not. I assumed that GitHub's API would be intelligent enough not to keep issuing notifications for users that are already a reviewer. Clearly that's not the case. Looks like I might need to fetch the current value and determine if it needs an update or not. This probably applies to a number of other pull request fields. I will take a look at this when I can make time.

For point 2, there is actually two separate things going on here. The update of a branch and the update of the PR on GitHub are separate.

Even though the commit is unchanged, it gets (force) pushed again.

This will happen when there have been changes on the base of the pull request. The action effectively rebases the changes onto the new commits on the base. So even though the commit itself is unchanged, the branch is now rebased. This is fundamental to how the action works and can't be changed. The comment I made in issue https://github.com/peter-evans/create-pull-request/issues/1062#issuecomment-1044548704 (that you linked) has a little more detail about that, and some workarounds.

The PR description is changed every night, updating the date in the description, which now makes the date incorrect for our purposes.

This is expected behaviour, and is basically the same reasoning I expressed here https://github.com/peter-evans/create-pull-request/issues/1062#issuecomment-1044548704, that "create" and "update" should not lead to a different result. If the action runs, it will always try and reach the desired state. When it runs, you are telling the action that you now want the body to be in a different state, so it updates it. I can't change this behaviour so I recommend working around it. For example, you could create a comment in each PR when it's created with the date information.

      - name: Create Pull Request
        id: cpr
        uses: peter-evans/create-pull-request@v4

      - name: Create comment
        if: steps.cpr.outputs.pull-request-operation == 'created'
        uses: peter-evans/create-or-update-comment@v2
        with:
          issue-number: ${{ steps.cpr.outputs.pull-request-number }}
          body: |
            Updated certificate bundle as of ${{ steps.get-date.outputs.DATE }}.

As for the behaviour described in point 3...

This is very much an edge-case and I should probably solve this by using a different branch name for PRs created via PR triggered workflow runs.

Yes, this is exactly the way to handle this situation. 👍

peter-evans avatar Mar 24 '22 03:03 peter-evans