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

feat(gerrit): use commit message footers to store source branch name #29802

Merged
merged 19 commits into from
Jul 2, 2024

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Jun 21, 2024

Changes

This changes the approach used to store and retrieve the source branch information in Gerrit. Instead of relying on hashtags, we now store the source branch information in a commit message footer named Renovate-Branch.

I believe the new approach is a lot friendlier for the end user. Less hacky, more intuitive, and better looking. Kind of aligns with the Gerrit way of doing things (like how they store and retrieve the change id using the Change-Id footer).

Here is an example of how it was:

https://review.gerrithub.io/c/felipecrs/gerrit-jenkins/+/1196601/1//COMMIT_MSG

image

And here is an example on how it is now:

https://review.gerrithub.io/c/felipecrs/gerrit-jenkins/+/1196702

image

Context

I had proposed this in #18961 (comment), but we concluded to do this post-merge of the Gerrit support given for how long that PR was being dragged on already.

So, I finally got around to doing this.

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 felipecrs changed the title feat(gerrit): use commit message footers instead of hashtags feat(gerrit): use commit msg footers instead of hashtags to store source branch Jun 21, 2024
@felipecrs felipecrs changed the title feat(gerrit): use commit msg footers instead of hashtags to store source branch feat(gerrit): use commit message footers to store source branch information Jun 21, 2024
@felipecrs felipecrs changed the title feat(gerrit): use commit message footers to store source branch information feat(gerrit): use commit message footers to store source branch name Jun 21, 2024
@felipecrs
Copy link
Contributor Author

@NiasSt90, @viceice, @valodzka, @kerby82, found your names from previous Gerrit-related PRs. I am just pinging you in case you'd like to opine about/review this PR.

@viceice viceice self-requested a review June 22, 2024 09:09
@NiasSt90
Copy link
Contributor

NiasSt90 commented Jun 24, 2024

I don't like the idea of doing this as a breaking change.

We can/should search for the old hashtag too. (syntax untested)

filters.push(`(hashtag:sourceBranch-${searchConfig.branchName} OR footer:.....)`);

@felipecrs
Copy link
Contributor Author

felipecrs commented Jun 24, 2024

@NiasSt90, yeah, keeping backwards compatibility is doable. But it would be considerably more difficult to do:

  • Search for existing issues (your comment)
  • Extract source branch from hashtags and from the commit message footer
  • Add some kind of migration function to migrate existing changes from hashtags to commit message footer
  • Add tests for all variants of the points above

I'm just not sure if it's worth it. As I mentioned, Gerrit support is still marked as experimental.

@NiasSt90
Copy link
Contributor

@NiasSt90, yeah, keeping backwards compatibility is doable. But it would be considerably more difficult to do:

  • Search for existing issues (your comment)
  • Extract source branch from hashtags and from the commit message footer

use the existing extractSourceBranch code and append your new search to the end...if first search found nothing.

  • Add some kind of migration function to migrate existing changes from hashtags to commit message footer

migration?
You search/extract from both (extract from hashtags first) sources and write only to the new (commit-message) destination....nothing more needed or?

  • Add tests for all variants of the points above

combine the old and the new tests....instead of change the old ones.

I'm just not sure if it's worth it. As I mentioned, Gerrit support is still marked as experimental.

because it's easy doable....

@felipecrs
Copy link
Contributor Author

migration?
You search/extract from both (extract from hashtags first) sources and write only to the new (commit-message) destination....nothing more needed or?

Does Renovate always recreate changes from scratch and only retain their source branch and commit ids? I thought Renovate would sometimes just rebase changes, in which case I would expect to write code to migrate such change from hashtag to footer.

Also, hashtags won't be gone from Gerrit once a new patchset is published with the footer. We would need to write code for it.

@felipecrs
Copy link
Contributor Author

Or maybe you mean we should search for both kind of changes and let it be. Someday users would end up merging or closing all changes and then there won't be more changes with hashtags.

We don't necessarily need to do housekeeping of removing the hashtags.

Is that what you meant?

@NiasSt90
Copy link
Contributor

NiasSt90 commented Jun 24, 2024

Or maybe you mean we should search for both kind of changes and let it be. Someday users would end up merging or closing all changes and then there won't be more changes with hashtags.

We don't necessarily need to do housekeeping of removing the hashtags.

Is that what you meant?

Yes, we search and extract from both sources (hashtags and commit-msg) but we only write the sourceBranch to the new location inside the commit message.

This way we don't lost/forget old changes (even closed ones) and accidentally reopen (or better recreate) them but all new created changes have the new location for their sourceBranch.

@felipecrs
Copy link
Contributor Author

Alright. I can work on this. @rarkins no objections I suppose?

@felipecrs
Copy link
Contributor Author

felipecrs commented Jun 24, 2024

Done.

Here is an example of a change that was "migrated". Note that the hashtag was not removed.

https://review.gerrithub.io/c/felipecrs/gerrit-jenkins/+/1196601

image

@felipecrs
Copy link
Contributor Author

Note that I updated the documentation to mention that support for hashtags will be removed once support for Gerrit is graduated from experimental. I don't like technical debts, and I think that's a fair compromise.

@felipecrs felipecrs requested a review from rarkins June 24, 2024 18:45
lib/modules/platform/gerrit/readme.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/readme.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

The docs look good to me.

lib/modules/platform/gerrit/scm.spec.ts Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/types.ts Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/utils.ts Outdated Show resolved Hide resolved
@felipecrs felipecrs requested a review from viceice June 28, 2024 11:28
@rarkins
Copy link
Collaborator

rarkins commented Jun 29, 2024

@NiasSt90 can you review/approve?

@NiasSt90
Copy link
Contributor

@NiasSt90 can you review/approve?

Yes, but not until Monday

viceice
viceice previously approved these changes Jul 1, 2024
@felipecrs
Copy link
Contributor Author

@NiasSt90 just a friendly reminder. :)

@felipecrs
Copy link
Contributor Author

FWIIW I have been self-hosting this PR since its creation, no issues.

@rarkins rarkins enabled auto-merge July 2, 2024 14:52
@rarkins
Copy link
Collaborator

rarkins commented Jul 2, 2024

Needs 7 conversations resolved

@felipecrs
Copy link
Contributor Author

@rarkins, they are all resolved to me:

image

@rarkins rarkins added this pull request to the merge queue Jul 2, 2024
Merged via the queue into renovatebot:main with commit 74aa3d7 Jul 2, 2024
38 checks passed
@felipecrs felipecrs deleted the use-footers-gerrit branch July 2, 2024 15:06
@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 37.422.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to 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.

None yet

7 participants