[go: up one dir, main page]

GitHub Importer: comments containing suggestions are not imported if the conversation was resolved

Hi there! 👋

Summary

I noticed a specific type of comments are missing on the imported Merge Requests: comments containing suggestions. Interesting fact: it's not all of them. It seems to only impact the resolved conversations. More details below.

Steps to reproduce

About 24 hours ago, I imported a github.com repository on gitlab.com. I used the Github integration[1] which was straightforward. I didn't have to choose anything other than the repo I wanted to import.

[1] https://docs.gitlab.com/ee/user/project/import/github.html#use-the-github-integration

What is the current bug behavior?

Here's an example:

Screen_Shot_2021-11-26_at_11.52.12

In this Github PR, we can see 3 comments. The one in the middle contains a suggestion.

Once imported on Gitlab:

Screen_Shot_2021-11-26_at_11.53.07

only 2 of the comments got imported. I expanded all threads on Gitlab and Ctrl+F'ing the middle comment doesn't yield anything.

Interestingly, I found an example where a suggestion was correctly imported:

Screen_Shot_2021-11-26_at_11.59.30

Screen_Shot_2021-11-26_at_12.01.04

As you can see, this one wasn't resolved. I guess it's the difference that explains the behavior. I found a few more examples validating this pattern. I'm happy to provide more information, if needed

What is the expected correct behavior?

All comments, including resolved ones should be imported.

Output of checks

This bug happens on GitLab.com

Possible fixes

I don't know what the possibles fixes are. That said, I saw that part of the GitHub importer has had its implementation changed for the past 2 months:

How does that sound to you all, @kassio, @georgekoltsov, @jmarquez2, @dstull?

Analysis

The error on the mentioned import was DiffNote::NoteDiffFileCreationError, after checking on Sentry and noticing that this error is not happening exclusively during the import a solution proposed on !76376 (merged) is to fallback to LegacyDiffNote if this exception happens.

Solution implemented

Since I couldn't find the root cause of the problem, I implemented a fallback to avoid missing merge request comments. Since !76376 (merged) if a comment with a suggestion fails to be created with the formatted suggestion, the importer will fallback to import the comment without formatting the suggestion.

Edited by Kassio Borges