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: Support for Platform "Gerrit" #18961

Merged
merged 117 commits into from Dec 29, 2023
Merged

Conversation

NiasSt90
Copy link
Contributor

@NiasSt90 NiasSt90 commented Nov 17, 2022

Changes

Allow to use Renovate on the Gerrit-Platform.
The Pull-Request-Branch Model of Renovate is translated to Gerrit pre-Submit Model (push to refs/for/).

Context

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 tick 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

- all major features are complete and seems to be working fine

see lib/modules/platform/gerrit/index.md for documentation
@NiasSt90 NiasSt90 changed the title Support for Platform "Gerrit" feat: Support for Platform "Gerrit" Nov 17, 2022
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.

Let's start with some simple changes. The docs need more work after merging my suggestions.

Some sections need more explanations.

docs/usage/configuration-options.md Outdated Show resolved Hide resolved
lib/config/options/index.ts Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
NiasSt90 and others added 8 commits November 22, 2022 07:59
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

the git changes definetly needs to be in seperate PR to be merged before

lib/util/git/index.ts Outdated Show resolved Hide resolved
lib/util/http/gerrit.ts Outdated Show resolved Hide resolved
lib/config/presets/local/index.ts Outdated Show resolved Hide resolved
lib/config/presets/local/index.ts Outdated Show resolved Hide resolved
lib/config/options/index.ts Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/index.md Outdated Show resolved Hide resolved
@Shegox
Copy link
Contributor

Shegox commented Nov 23, 2022

I tested this today on our internal Gerrit. First: really awesome stuff and very much looking forward to seeing it in action.
Pretty much worked out-of-the-box in my tests.

Take the below please as small user feedback, not sure if that is in anyway feasible to easily implement or even needed :)

On open question we had, and might just be a documentation thing, is how to enable rebasing.
Currently it still shows the checkbox, and as far as I know you can't tick/edit the comment on Gerrit?
image

It might be worth to add some documentation if that works and maybe even adjust the PR-content to mention how to do it to have the information self-contained.

Again many thanks for your work on this, looking forward to seeing Gerrit-support.


Edit: I found a way to get it to "rebase/retry" the change.
Prefixing the change name (which is the first line of the commit message) with rebase! will force a rebase:

image

NiasSt90 and others added 3 commits November 24, 2022 09:13
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@Shegox
Copy link
Contributor

Shegox commented Nov 24, 2022

@NiasSt90 I found that the azure devops platform uses this regex to "massage" the markdown and remove what is not needed. Maybe we can do something similar for removing the rebase checkbox and replace it with "Prefix the commit message with rebase! to force a rebase/retry." statement.

export function massageMarkdown(input: string): string {
// Remove any HTML we use
return smartTruncate(input, 4000)
.replace(
'you tick the rebase/retry checkbox',
'rename PR to start with "rebase!"'
)
.replace(regEx(`\n---\n\n.*?<!-- rebase-check -->.*?\n`), '')
.replace(regEx(/<!--renovate-(?:debug|config-hash):.*?-->/g), '');
}

@NiasSt90
Copy link
Contributor Author

@NiasSt90 I found that the azure devops platform uses this regex to "massage" the markdown and remove what is not needed. Maybe we can do something similar for removing the rebase checkbox and replace it with "Prefix the commit message with rebase! to force a rebase/retry." statement.

export function massageMarkdown(input: string): string {
// Remove any HTML we use
return smartTruncate(input, 4000)
.replace(
'you tick the rebase/retry checkbox',
'rename PR to start with "rebase!"'
)
.replace(regEx(`\n---\n\n.*?<!-- rebase-check -->.*?\n`), '')
.replace(regEx(/<!--renovate-(?:debug|config-hash):.*?-->/g), '');
}

I've seen this.
But it feels more like a hack than a good solution. But as a last resort this could be used.

To your other "rebase.." question/answer:

if you can edit the commit-msg from the Gerrit-ui why you don't click on the rebase button to enforce a rebase?

How your "rebaseWhen" configuration looks like during your Gerrit-Tests?

@Shegox
Copy link
Contributor

Shegox commented Nov 24, 2022

To your other "rebase.." question/answer:
if you can edit the commit-msg from the Gerrit-ui why you don't click on the rebase button to enforce a rebase?
How your "rebaseWhen" configuration looks like during your Gerrit-Tests?

For pure "git native" rebase the UI thing is sufficient. What I normally use the retry/rebase checkbox for is to actually retry a change. E.g. to explicitly tell Renovate, please recreate this change with the latest information now. This might be different from just a rebase.

This might be necessary if Renovate failed during the creation with an error like below (that explicit message is from GitHub, but the same can happen on Gerrit as well):
image

Anyway I think the current setup with the commit message fully satisfies this and it is probably just about documenting it.

@NiasSt90
Copy link
Contributor Author

To your other "rebase.." question/answer:
if you can edit the commit-msg from the Gerrit-ui why you don't click on the rebase button to enforce a rebase?
How your "rebaseWhen" configuration looks like during your Gerrit-Tests?

For pure "git native" rebase the UI thing is sufficient. What I normally use the retry/rebase checkbox for is to actually retry a change. E.g. to explicitly tell Renovate, please recreate this change with the latest information now. This might be different from just a rebase.

This might be necessary if Renovate failed during the creation with an error like below (that explicit message is from GitHub, but the same can happen on Gerrit as well): image

okay, rebuilding the complete change from current baseBranch is the same what is actually done with rebaseWhen=auto and Gerrit.getRepoForceRebase=true.
Therefore i'm wondering why a manual rebase/rebuild is needed.

See here for some details about the current default behavior, the comment was gone to the wrong conversation.

Anyway I think the current setup with the commit message fully satisfies this and it is probably just about documenting it.

okay, the commit-msg (prTitle) was checked independent of the rebaseWhen configuration.

# Conflicts:
#	lib/config/presets/local/__snapshots__/index.spec.ts.snap
#	lib/config/presets/local/index.spec.ts
#	lib/config/presets/local/index.ts
@rvarun11
Copy link
Contributor

rvarun11 commented Jan 5, 2023

Any update on this getting merged anytime soon?

@viceice
Copy link
Member

viceice commented Jan 5, 2023

@viceice viceice marked this pull request as draft January 5, 2023 13:11
@NiasSt90
Copy link
Contributor Author

For testing purpose i've pushed the current status (based on #19065) to this branch.

Improvements:

  • no special configuration needed anymore (neither gitNoVerify nor platformCommit)
  • better markdown handling (some replacements specific to Gerrit: PR -> Change-Request, ...)

@NiasSt90
Copy link
Contributor Author

I've updated this branch to the latest version of #19065.

We are coming closer to a final version ;)

@sselberg
Copy link

Although we patched it to support Gerrit 2.14 (which we will consider to upstream if we can make it in a clean way).

2.14 has been EOL for > 4 years, I hope that there aren't that many installations of 2.14 anymore.

If you are having trouble upgrading I know that Luca and company over at GerritForge have helped a lot of Gerrit users upgrade to latest.

rarkins
rarkins previously approved these changes Dec 18, 2023
@rarkins
Copy link
Collaborator

rarkins commented Dec 18, 2023

@viceice please approve when ready

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.

I'm happy with the docs.

@rarkins
Copy link
Collaborator

rarkins commented Dec 18, 2023

@NiasSt90 should be no need to merge from main unless there's a conflict now

lib/modules/platform/gerrit/index.ts Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/utils.spec.ts Outdated Show resolved Hide resolved
lib/modules/platform/gerrit/utils.ts Show resolved Hide resolved
lib/util/http/gerrit.ts Outdated Show resolved Hide resolved
lib/util/http/gerrit.ts Outdated Show resolved Hide resolved
auto-merge was automatically disabled December 19, 2023 08:38

Head branch was pushed to by a user without write access

@valodzka
Copy link
Contributor

I'm not sure how many gerrit users beyond me have tested this Pr, but I'm sure this Pr can be go to production.

To add +1 datapoint, I tested it with my company gerrit setup (last 3.9.1 version) and can confirm that PR works well.

@rarkins rarkins added this pull request to the merge queue Dec 29, 2023
Merged via the queue into renovatebot:main with commit b2422d8 Dec 29, 2023
38 checks passed
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 37.112.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@elupus
Copy link

elupus commented Jan 2, 2024

What happened to the docs? https://docs.renovatebot.com/modules/platform/gerrit/ is empty?

@valodzka
Copy link
Contributor

valodzka commented Jan 2, 2024

What happened to the docs? https://docs.renovatebot.com/modules/platform/gerrit/ is empty?

I am not familiar with renovate docs structure, but my guess file index.md should be called readme.md.

@MindTooth
Copy link
Contributor

I've created a pull request for the fix: #26479

@viceice viceice added the platform:gerrit Gerrit Platform label Jan 5, 2024
zT-1337 pushed a commit to scm-manager/renovate that referenced this pull request Jan 22, 2024
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Co-authored-by: Rhys Arkins <rhys@arkins.net>
zT-1337 pushed a commit to scm-manager/renovate that referenced this pull request Jan 24, 2024
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Co-authored-by: Rhys Arkins <rhys@arkins.net>
zT-1337 pushed a commit to scm-manager/renovate that referenced this pull request Jan 24, 2024
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Co-authored-by: Rhys Arkins <rhys@arkins.net>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
platform:gerrit Gerrit Platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Gerrit