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

add Index to comment for migrations and mirroring #18806

Merged
merged 11 commits into from
Mar 6, 2022

Conversation

singuliere
Copy link
Contributor

@singuliere singuliere commented Feb 18, 2022

See original merge request from Loïc Dachary @ forgefriends


Comments have an id (see Gitea[0], GitLab[1] or GitHub[2], etc.), and the
comment migration format must represent it during migrations so that
it can be used during mirroring or incremental migrations.

[0] https://try.gitea.io/api/swagger#/issue/issueGetComment
[1] https://docs.gitlab.com/ee/api/discussions.html#get-single-issue-discussion-item
[2] https://docs.github.com/en/rest/reference/issues#get-an-issue-comment

@singuliere singuliere added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/repo-migration Migrate repos from other platforms to Gitea, or from Gitea to them labels Feb 18, 2022
@singuliere singuliere added this to the 1.17.0 milestone Feb 18, 2022
@lunny
Copy link
Member

lunny commented Feb 19, 2022

What's the definition of comment index?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 19, 2022
@singuliere
Copy link
Contributor Author

singuliere commented Feb 19, 2022

What's the definition of comment index?

It is the unique identifier of the comment. It could be named ID, Number or Index. The name Index was chosen because it is also chosen in another pull request to unify the naming of unique identifiers in the context of migrations.

@singuliere
Copy link
Contributor Author

The CI failure is unrelated to this PR, could someone please restart it?

+ make lint-backend

golangci-lint run --timeout 10m

level=error msg="[linters context] typechecking error: pattern ./...: open /drone/src/node_modules/esbuild-android-arm64: no such file or directory"

make: *** [Makefile:782: golangci-lint] Error 7

Comments have an id (see Gitea[0], GitLab[1], GitHub[2], etc.), and the
comment migration format must represent it during migrations so that
it can be used during mirroring or incremental migrations.

[0] https://try.gitea.io/api/swagger#/issue/issueGetComment
[1] https://docs.gitlab.com/ee/api/discussions.html#get-single-issue-discussion-item
[2] https://docs.github.com/en/rest/reference/issues#get-an-issue-comment

Signed-off-by: Loïc Dachary <loic@dachary.org>
@singuliere
Copy link
Contributor Author

It would be too complicated to add the comment identifier in the database. This pull request was simplified to only add the information in the migration files. It can later be used to set the information in the ForeignReference table when this pull request is merged.

@lunny
Copy link
Member

lunny commented Feb 19, 2022

The Index is per repository or per issue? Since there is no id or index from Github, what should be stored?

@singuliere
Copy link
Contributor Author

The Index is per repository or per issue?

It depends. The index for Gitea is absolute even though the API endpoint is relative to the repository. The API endpoint for GitHub is also relative to the repository but the documentation does not specify if the id field is relative to the repository or not. However, since the API endpoint is not relatve to the issue, I think the id is not per issue.

Since there is no id or index from Github, what should be stored?

There is an idfor comments from GitHub. And for GitLab too and every other forge supported by Gitea at the moment.

Maybe I misunderstood your question?

@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 Feb 26, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@b24e8d3). Click here to learn what that means.
The diff coverage is 13.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18806   +/-   ##
=======================================
  Coverage        ?   46.56%           
=======================================
  Files           ?      854           
  Lines           ?   122889           
  Branches        ?        0           
=======================================
  Hits            ?    57218           
  Misses          ?    58749           
  Partials        ?     6922           
Impacted Files Coverage Δ
modules/migration/comment.go 100.00% <ø> (ø)
services/migrations/codebase.go 0.78% <0.00%> (ø)
services/migrations/gitlab.go 5.21% <0.00%> (ø)
services/migrations/gogs.go 2.02% <0.00%> (ø)
services/migrations/onedev.go 0.82% <0.00%> (ø)
services/migrations/github.go 66.30% <50.00%> (ø)
services/migrations/gitea_downloader.go 67.11% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b24e8d3...a53e163. Read the comment docs.

@Gusted
Copy link
Contributor

Gusted commented Mar 4, 2022

In simple words this PR provide a value for the GetForeignIndex() for the comment structs?

Answered by dachary in matrix:

Yes, that's exactly the purpose of this PR. Without an Index in the migration files, there is no way to match a comment migrated into Gitea with the comment it comes from in the original forge.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 4, 2022
@singuliere
Copy link
Contributor Author

make l-g-t-m work

@singuliere singuliere removed the request for review from KN4CK3R March 6, 2022 10:16
@singuliere
Copy link
Contributor Author

make l-g-t-m work

@6543 6543 merged commit cc64328 into go-gitea:main Mar 6, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 7, 2022
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Add Index to comment for migrations and mirroring (go-gitea#18806)
  Support ignore all santize for external renderer (go-gitea#18984)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
Comments have an id (see Gitea[0], GitLab[1], GitHub[2], etc.), and the
comment migration format must represent it during migrations so that
it can be used during mirroring or incremental migrations.

[0] https://try.gitea.io/api/swagger#/issue/issueGetComment
[1] https://docs.gitlab.com/ee/api/discussions.html#get-single-issue-discussion-item
[2] https://docs.github.com/en/rest/reference/issues#get-an-issue-comment

Signed-off-by: Loïc Dachary <loic@dachary.org>
Co-authored-by: Loïc Dachary <loic@dachary.org>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
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. topic/repo-migration Migrate repos from other platforms to Gitea, or from Gitea to them type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants