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

Properly migrate automatic merge GitLab comments #27873

Merged
merged 22 commits into from
Feb 22, 2024

Conversation

invliD
Copy link
Contributor

@invliD invliD commented Nov 2, 2023

GitLab generates "system notes" whenever an event happens within the platform. Unlike Gitea, those events are stored and retrieved as text comments with no semantic details. The only way to tell whether a comment was generated in this manner is the system flag on the note type.

This PR adds detection for two specific kinds of events: Scheduling and un-scheduling of automatic merges on a PR. When detected, they are downloaded using Gitea's type for these events, and eventually uploaded into Gitea in the expected format, i.e. with no text content in the comment.

This PR also updates the template used to render comments to add support for migrated comments of these two types.

ref: https://gitlab.com/gitlab-org/gitlab/-/blob/11bd6dc826e0bea2832324a1d7356949a9398884/app/services/system_notes/merge_requests_service.rb#L6-L17

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 2, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 2, 2023
@lunny lunny added the type/enhancement An improvement of existing functionality label Nov 2, 2023
@lunny lunny added this to the 1.22.0 milestone Nov 2, 2023
@invliD invliD requested a review from silverwind November 3, 2023 19:58
@silverwind
Copy link
Member

Maybe add a test?

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2023
@@ -488,30 +489,8 @@ func (g *GitlabDownloader) GetComments(commentable base.Commentable) ([]*base.Co
return nil, false, fmt.Errorf("error while listing comments: %v %w", g.repoID, err)
}
for _, comment := range comments {
// Flatten comment threads
if !comment.IndividualNote {
Copy link
Member

Choose a reason for hiding this comment

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

This line now will be ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both branches here were exactly the same, except only the first note was converted if IndividualNote is true. That isn't really necessary, because IndividualNote means that this is not a discussion (=thread) but just a single comment that cannot be responded to. There will always be exactly one note in such a "discussion", so there is no reason to explicitly ignore any further notes.

@invliD invliD requested a review from lunny November 11, 2023 05:22
@invliD
Copy link
Contributor Author

invliD commented Nov 23, 2023

@lunny @silverwind Is there anything else you would like me to change?

@lunny
Copy link
Member

lunny commented Nov 23, 2023

@lunny @silverwind Is there anything else you would like me to change?

I will review it again.

@invliD
Copy link
Contributor Author

invliD commented Jan 15, 2024

@lunny Any chance you could look at this again? I have a lot more changes coming to make GitLab imports better.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 16, 2024
@invliD
Copy link
Contributor Author

invliD commented Feb 18, 2024

@silverwind Could you please take another look as well?

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 18, 2024
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 21, 2024
@silverwind
Copy link
Member

silverwind commented Feb 21, 2024

Sorry for back and forth, I noticed the CSS .ui .migrate a only later which makes muted-links unnecessary.

@wxiaoguang wxiaoguang removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 22, 2024
invliD and others added 2 commits February 22, 2024 07:39
@wxiaoguang wxiaoguang added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 22, 2024
@lunny lunny enabled auto-merge (squash) February 22, 2024 06:54
@lunny lunny merged commit a70c00b into go-gitea:main Feb 22, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 22, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 23, 2024
* giteaofficial/main:
  Start to migrate from `util.OptionalBool` to `optional.Option[bool]` (go-gitea#29329)
  Add slow SQL query warning (go-gitea#27545)
  Unify organizations header (go-gitea#29248)
  Frontport changelogs of minor releases (go-gitea#29337)
  Support SAML authentication (go-gitea#25165)
  Upgrade to fabric 6 (go-gitea#29334)
  Don't show third-party JS errors in production builds (go-gitea#29303)
  Remove bountysource (go-gitea#29330)
  Remove unnecessary "Str2html" modifier from templates (go-gitea#29319)
  Ignore the linux anchor point to avoid linux migrate failure (go-gitea#29295)
  Remove jQuery from the repo commit functions (go-gitea#29230)
  Remove unnecessary "Safe" modifier from templates (go-gitea#29318)
  Remove jQuery from the image pasting functionality (go-gitea#29324)
  Improve the `issue_comment` workflow trigger event (go-gitea#29277)
  Properly migrate automatic merge GitLab comments (go-gitea#27873)
  Refactor cmd setup and remove deadcode (go-gitea#29313)
  small cache when get user id on interation (go-gitea#29296)
  Discard unread data of `git cat-file` (go-gitea#29297)
  Don't install playwright twice (go-gitea#29302)

# Conflicts:
#	templates/home.tmpl
@invliD invliD deleted the gitlab-schedule-merge branch February 23, 2024 06:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants