Skip to content

Do not always overwrite the GITHUB_REF for PRs - #217

Merged
rneatherway merged 2 commits into
mainfrom
rneatherway/optional-merge
Sep 29, 2020
Merged

Do not always overwrite the GITHUB_REF for PRs#217
rneatherway merged 2 commits into
mainfrom
rneatherway/optional-merge

Conversation

@rneatherway

Copy link
Copy Markdown
Contributor

As we move towards analysing the merge commit for pull requests by
default, we should stop sending /refs/pull/n/head rather than
refs/pull/n/mergeunless the checked-out SHA has actually changed.
Here we assume that any change (compared to GITHUB_SHA) indicates that
git checkout HEAD^2 has been run earlier. This may sometimes be
incorrect (e.g. git checkout mybranch), but in that case the ref
would be wrong either way.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

As we move towards analysing the merge commit for pull requests by
default, we should stop sending `/refs/pull/n/head` rather than
`refs/pull/n/merge` _unless_ the checked-out SHA has actually changed.
Here we assume that any change (compared to GITHUB_SHA) indicates that
`git checkout HEAD^2` has been run earlier. This may sometimes be
incorrect (e.g. `git checkout mybranch`), but in that case the ref
would be wrong either way.
@rneatherway
rneatherwayforce-pushed the rneatherway/optional-merge branch from 74587c5 to 7795860CompareSeptember 17, 2020 12:11
@robertbrignullrobertbrignull self-assigned this Sep 18, 2020

@robertbrignullrobertbrignull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chrisgavinchrisgavin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this with the backend changes required to map merge commit annotations to the head commit and it worked great!

@rneatherway

Copy link
Copy Markdown
ContributorAuthor

Thanks!

@rneatherway
rneatherway merged commit bb6fa8e into mainSep 29, 2020
@rneatherway
rneatherway deleted the rneatherway/optional-merge branch September 29, 2020 12:17
@github-actionsgithub-actionsBot mentioned this pull request Oct 5, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@rneatherway@robertbrignull@chrisgavin