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

fix(github)!: change automerge strategy priority order to allow platform signed commits #32016

Merged

Conversation

pballandras
Copy link
Contributor

@pballandras pballandras commented Oct 18, 2024

Changes

This PR introduces a change in the way Renovate selects an automerge strategy/method for GitHub repositories. Previously it would test which merge strategy the repo allows and choose the first one available in this order in an if statement:

  1. Rebase
  2. Squash
  3. Merge

This PR changes this to:

  1. Squash
  2. Merge
  3. Rebase

Context

Discussion:

Why is the order important

The reason why the order is important lies in a limitation GitHub has when it comes to verifying/signing commits when merging PRs. More specifically:

When using the Rebase and Merge option on a pull request, it's important to note that the commits in the head branch are added to the base branch without commit signature verification.

https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification#signature-verification-for-rebase-and-merge

Since Renovate uses this method if there's no other configuration to explicitly change it in the repository, by default, it means the PRs Renovate merges won't be signed. Hence, this PR.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

Although I don't believe there are changes needed to the doc, if you feel like additions could be made somewhere I didn't think of, I'll gladly contribute.

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

To test if the reordering would work I disabled some merge strategies to force Renovate into other branches of the if statement. With no surprises, the current order is respected and the interesting part is when Renovate squashes or uses merge commits (the merge strategy), the resulting commits are signed. Which is what is we want.

@CLAassistant
Copy link

CLAassistant commented Oct 18, 2024

CLA assistant check
All committers have signed the CLA.

@pballandras pballandras changed the title Change merge strategy priority order to allow GitHub to sign commits fix: change automerge strategy priority order to allow GitHub to sign commits Oct 18, 2024
@rarkins
Copy link
Collaborator

rarkins commented Oct 19, 2024

@pballandras thanks for this PR, but please create a Discussion first so we can discuss. I'll mark this PR as draft in the meantime. Please be sure to describe exactly which use case/scenario you find not be working today.

@rarkins rarkins marked this pull request as draft October 19, 2024 05:54
@pballandras
Copy link
Contributor Author

I see you use semantic release and in the discussion you said you wanted this to be in a major release. I believe you can add it in the commit footer during merge, but do you need anything from me at this point? Thanks!

@pballandras pballandras marked this pull request as ready for review October 23, 2024 15:32
@viceice viceice added the breaking Breaking change, requires major version bump label Oct 23, 2024
@viceice viceice changed the title fix: change automerge strategy priority order to allow GitHub to sign commits fix(github)?: change automerge strategy priority order to allow platform signed commits Oct 23, 2024
@viceice viceice changed the title fix(github)?: change automerge strategy priority order to allow platform signed commits fix(github)!: change automerge strategy priority order to allow platform signed commits Oct 23, 2024
@rarkins rarkins changed the base branch from main to v39 October 25, 2024 16:36
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Remove unused param

@rarkins
Copy link
Collaborator

rarkins commented Oct 31, 2024

@pballandras can you accept the change today? We want to release v39 soon.

@rarkins rarkins merged commit fbd3680 into renovatebot:v39 Nov 1, 2024
36 checks passed
@pballandras pballandras deleted the fix/change-automerge-method-order-for-github branch November 1, 2024 13:06
rarkins pushed a commit that referenced this pull request Nov 4, 2024
Changes the priority order so that squash merges are preferred over rebase merge and merge commits. Doing so allows GitHub to sign the resulting commit on the base branch.

BREAKING CHANGE: Renovate will now prefer squash merges over others in GitHub, if they are allowed.
rarkins pushed a commit that referenced this pull request Nov 4, 2024
Changes the priority order so that squash merges are preferred over rebase merge and merge commits. Doing so allows GitHub to sign the resulting commit on the base branch.

BREAKING CHANGE: Renovate will now prefer squash merges over others in GitHub, if they are allowed.
rarkins pushed a commit that referenced this pull request Nov 4, 2024
Changes the priority order so that squash merges are preferred over rebase merge and merge commits. Doing so allows GitHub to sign the resulting commit on the base branch.

BREAKING CHANGE: Renovate will now prefer squash merges over others in GitHub, if they are allowed.
rarkins pushed a commit that referenced this pull request Nov 4, 2024
Changes the priority order so that squash merges are preferred over rebase merge and merge commits. Doing so allows GitHub to sign the resulting commit on the base branch.

BREAKING CHANGE: Renovate will now prefer squash merges over others in GitHub, if they are allowed.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking Breaking change, requires major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants