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

Migrate reviews when migrating repository from github #9463

Merged
merged 17 commits into from
Jan 23, 2020

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 22, 2019

This PR will migrate review and review comments when migrating repository from github.
TODO:

  • Need tests.
  • Replace OriginalAuthorID
  • Don't think migrator is the author if it's a migrated review or comment.
  • Don't think reviewers as the same person on reviewers summary.
  • Create action comment for reviews
  • Migrate review comment reactions

A pull request migrated from go-gitea/test_repo#4 screenshot as below:

图片

@lunny lunny added the type/enhancement An improvement of existing functionality label Dec 22, 2019
@codecov-io
Copy link

codecov-io commented Dec 22, 2019

Codecov Report

Merging #9463 into master will decrease coverage by 0.13%.
The diff coverage is 29.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9463      +/-   ##
==========================================
- Coverage   42.35%   42.22%   -0.14%     
==========================================
  Files         608      609       +1     
  Lines       79411    79692     +281     
==========================================
+ Hits        33633    33648      +15     
- Misses      41617    41899     +282     
+ Partials     4161     4145      -16
Impacted Files Coverage Δ
models/migrations/migrations.go 1.3% <ø> (ø) ⬆️
modules/migrations/base/downloader.go 26.66% <ø> (ø) ⬆️
models/review.go 41.26% <0%> (-5.38%) ⬇️
modules/migrations/git.go 53.33% <0%> (-3.81%) ⬇️
models/migrate.go 0% <0%> (ø) ⬆️
models/pull.go 41.99% <0%> (-0.84%) ⬇️
models/migrations/v125.go 0% <0%> (ø)
models/external_login_user.go 20.72% <0%> (-0.58%) ⬇️
modules/migrations/gitea.go 6.16% <1.02%> (-1.65%) ⬇️
modules/migrations/migrate.go 20.95% <19.04%> (-3.26%) ⬇️
... and 14 more

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 bfdfa9a...f37545b. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 22, 2019
@6543
Copy link
Member

6543 commented Jan 2, 2020

migration has conflicts

@lunny lunny force-pushed the lunny/migrate_review branch 2 times, most recently from 6501af7 to 754962a Compare January 4, 2020 02:57
@lunny lunny added this to the 1.12.0 milestone Jan 4, 2020
@lunny lunny changed the title WIP: Migrate reviews when migrating repository from github Migrate reviews when migrating repository from github Jan 4, 2020
@lunny
Copy link
Member Author

lunny commented Jan 4, 2020

It's ready for review.

@@ -0,0 +1,24 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2019 The Gitea Authors. All rights reserved.
// Copyright 2020 The Gitea Authors. All rights reserved.

models/review.go Outdated
}
sess.Close()

for _, review := range reviews {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this out of xorm session?

Copy link
Member

Choose a reason for hiding this comment

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

  • why is line 408 not behind line 404

@lunny
Copy link
Member Author

lunny commented Jan 4, 2020

@6543 All done.

@lunny lunny added the pr/wip This PR is not ready for review label Jan 4, 2020
@6543
Copy link
Member

6543 commented Jan 4, 2020

current state if I test i get this error:
Migrate repository from https://github.com/6543/remaster failed: pull request does not exist [id: 128, issue_id: 0, head_repo_id: 0, base_repo_id: 0, head_branch: , base_branch: ]

@6543
Copy link
Member

6543 commented Jan 4, 2020

@lunny
Copy link
Member Author

lunny commented Jan 5, 2020

@6543 Thanks! I have fixed that.

@guillep2k
Copy link
Member

It's ready for review.

Maybe remove the wip label? 😁

models/migrate.go Outdated Show resolved Hide resolved
modules/migrations/gitea.go Show resolved Hide resolved
for _, comment := range review.Comments {
headCommitID, err := g.gitRepo.GetRefCommitID(pr.GetGitRefName())
if err != nil {
return fmt.Errorf("GetRefCommitID[%s]: %v", pr.GetGitRefName(), err)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe if the ref can't be found we should add the comment anyway (e.g. as a normal comment)?

Copy link
Member Author

@lunny lunny Jan 10, 2020

Choose a reason for hiding this comment

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

It should be correct since we have migrated all pullrequest patches.

modules/migrations/migrate.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member Author

lunny commented Jan 8, 2020

It's ready for review.

Maybe remove the wip label? 😁

Currently github stored position related the patch head but gitea stored line of treepath. I will resolve that.

@guillep2k
Copy link
Member

Fixes #9443

@6543
Copy link
Member

6543 commented Jan 9, 2020

pleace resolve confilct

@lunny lunny removed the pr/wip This PR is not ready for review label Jan 10, 2020
@lunny
Copy link
Member Author

lunny commented Jan 10, 2020

@6543 @guillep2k It's ready to review again.

@lunny
Copy link
Member Author

lunny commented Jan 10, 2020

depends on #9688

@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 10, 2020
@lunny
Copy link
Member Author

lunny commented Jan 23, 2020

Please review again.

@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 Jan 23, 2020
@lafriks lafriks merged commit f6067a8 into go-gitea:master Jan 23, 2020
@lunny lunny deleted the lunny/migrate_review branch January 24, 2020 01:14
This was referenced Jan 28, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants