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(gerrit): improve commit message vs pr title workaround #32115

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Oct 24, 2024

Changes

This improves the workaround for the Gerrit change title mismatch issue.

The previous approach had big downsides:

  1. It would create new patchsets every time the commit message title did not match with the prTitle

    • Creating new patchsets means triggerring unecessary CI jobs, which depending on the project, might be expensive
    • Increases the list of iterations of a change. Some Gerrit deployments will limit the amount of iterations a change can have and if the limit is reached, the change is doomed and needs to be recreated, which requires manual intervention.

    Image:

    description
  2. If Renovate for whatever reason fails before updating the commit message to match the PR title, it would mean that the change would be forgot by Renovate forever, and Renovate would attempt to create a new one next time. Also, if this happens and someone closes the change (to ignore future updates), Renovate will also never realize that the change was closed and will keep trying to create a new one.

    • Although it seems rare, it has happened several times with my Renovate deployment. I spent several hours to figure out this was the root cause.

The new approach will always ensure the commit is created or recreated with the proper title. In this case, none of the downsides above will happen. Also, it cleans the code a little, removing gimmicks no longer necessary.

I understand this fix touches a bit of code outside of the Gerrit platform, but I believe it is better enough to at least justify it being merged (when compared to the current approach). If a better approach is found in the future, we can always refactor it.

With this fix, I also believe that the user doesn't even need to be aware of how this is handled. Previously, maybe the user would notice the second patchset and thus the documentation would explain this behavior. Now, the user will never notice this behavior to begin with. Obviously, I'm open to suggestions.

Context

See above.

Documentation (please check one with an [x])

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

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

@felipecrs
Copy link
Contributor Author

@NiasSt90 you may want to take a look at this.

@felipecrs felipecrs changed the title fix: improve gerrit change title mismatch handling fix(gerrit): improve commit message vs pr title workaround Oct 24, 2024
@NiasSt90
Copy link
Contributor

@NiasSt90 you may want to take a look at this.

the changes are looking okay for me (not tried, perhaps next Tuesday i can try it).
I think it's good to get rid of these unnecessary patches. The new workaround is better then my original one.

As long as renovate needs the distinction between prTitle and the first line of the commit message, the only option is such extra handling within the Gerrit platform.

@rarkins
Copy link
Collaborator

rarkins commented Oct 25, 2024

As long as renovate needs the distinction between prTitle and the first line of the commit message, the only option is such extra handling within the Gerrit platform.

This is a requirement we can probably loosen. Technically you can probably loosen it today, although we say that editing prTitle is deprecated. Would it be something to look at before, or part of, or after this PR?

@felipecrs
Copy link
Contributor Author

@rarkins I believe such change can (and I'd prefer it to) be done after this PR.

So long the prTitle is important, this PR is still relevant. It retains the current requirements but improves how.

To be honest, it doesn't seem anything simple to change such requirement (at least to me).

@felipecrs
Copy link
Contributor Author

I have been hosting Renovate with this PR for three weeks by now. My self-hosted Renovate manages hundreds of changes daily. I can confidently say everything is working as expected.

@rarkins rarkins enabled auto-merge November 12, 2024 05:08
@rarkins rarkins added this pull request to the merge queue Nov 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024
@rarkins rarkins added this pull request to the merge queue Nov 12, 2024
Merged via the queue into renovatebot:main with commit f7e4668 Nov 12, 2024
39 checks passed
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 39.10.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants