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

Action fails with unhelpful error message when PR branch is out of date with target branch #19

Closed
owenpearson opened this issue Jun 10, 2021 · 4 comments · Fixed by #26

Comments

@owenpearson
Copy link
Member

See this run for example behaviour.

@owenpearson owenpearson self-assigned this Jun 10, 2021
@QuintinWillison
Copy link
Contributor

We saw this again today and I think we've identified where the problem lies, and that is outside of the code in this repository.

It's surprising that the default behaviour of GitHub's Checkout Action is that it creates a merge commit by default, rather than just running against HEAD of the branch underlying the pull request. Others have faced this issue too - see actions/checkout#504

We see "Error: Conflict merging main into ":

Screenshot 2021-07-05 at 10 25 05

It looks like the workaround is documented in the checkout action's readme here (also mentioned in the issue I linked to above). Unfortunately that means we will need to update every workflow that we have, or at least definitely all of the ones that use our upload action, to explicitly checkout head.sha.

Remediation in respect of this repository specifically should be to update the readme to mention this, in order to save confusion in future.

@QuintinWillison
Copy link
Contributor

QuintinWillison commented Sep 15, 2021

We have been conforming the branch protection rule for the main branch in most of our open source repositories, specifically enabling "Require status checks to pass before merging" and its sub-option "Require branches to be up to date before merging".

Screenshot 2021-09-15 at 21 25 09

As far as I can tell, that sub-option negates the need for our checkout action invocations to create a merge commit to run checks against, because we'll always be told "This branch is out-of-date with the base branch" with an "Update branch" button in the GitHub UI to add a merge commit from main, triggering checks to run again - thus HEAD of the pull request's underlying branch will always be adequate.

This brings me to the conclusion that we probably need to standardise the replacement of our existing checkout action invocations that currently look like this:

- uses: actions/checkout@v2

With this longer form:

- uses: actions/checkout@v2
  with:
    ref: ${{ github.event.pull_request.head.sha }}

This is very disappointing as it would have been really nice had we been able to use the checkout action without needing to specify any explict config (i.e. prefer convention over configuration). But, fundamentally, we're seeing too many occasions where we have multiple pull requests all targeting the main branch and some or all of them end up failing checks due to workflow jobs that use this SDK Upload Action... because of this issue.

@owenpearson
Copy link
Member Author

Just had another proper look in to this and I'm now thinking it's to do with this auto_merge option for creating a deployment. From the docs:

Attempts to automatically merge the default branch into the requested ref, if it's behind the default branch.
Default: true

I'll make a 1.1 release tomorrow with this set to false.

@QuintinWillison
Copy link
Contributor

Ooh, exciting. Thanks for looking, @owenpearson. Fingers crossed! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants