-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Improve migrations to support migrating milestones/labels/issues/comments/pullrequests #6290
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6290 +/- ##
=========================================
Coverage ? 41.13%
=========================================
Files ? 432
Lines ? 59504
Branches ? 0
=========================================
Hits ? 24475
Misses ? 31817
Partials ? 3212
Continue to review full report at Codecov.
|
cfa1233
to
a7c2fe0
Compare
c5bc0ed
to
4daa5c7
Compare
7ceb4de
to
c0878f8
Compare
I think this is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what happens if the user creates the repo to be a mirror? Will issues/prs/wiki etc still be synced or only on the initial creation of the repo?
import "github.com/go-xorm/xorm" | ||
|
||
// InsertIssue insert one issue to database | ||
func InsertIssue(issue *Issue, labelIDs []int64) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different to createIssue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not trigger notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then I'd prefer to have createIssue
call this function and trigger notifications to avoid having duplicated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to do that, but it's so different between theses two functions I would like let it duplicated currently. Maybe send a refactor PR after this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, sounds good.
models/repo.go
Outdated
@@ -877,6 +877,7 @@ type MigrateRepoOptions struct { | |||
IsPrivate bool | |||
IsMirror bool | |||
RemoteAddr string | |||
NoWiki bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please call this UsesWiki
or just Wiki
or something like that, then check if opts.Wiki
instead of if !opts.NoWiki
to avoid double negation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
if err := os.RemoveAll(wikiPath); err != nil { | ||
return repo, fmt.Errorf("Failed to remove %s: %v", wikiPath, err) | ||
} | ||
|
||
if err = git.Clone(wikiRemotePath, wikiPath, git.CloneRepoOptions{ | ||
Mirror: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this always a mirror?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the Mirror
means a full clone but not a mirror repository on Gitea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These codes just moved from above because I added opts.Wiki
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably ok and we certainly want at least part of what it's doing regarding copying the remote tracking branches. We should check what happens when we remove the upstream at the end of the migration and check that the refspec configuration is cleaned up though.
) | ||
|
||
// GiteaLocalUploader implements an Uploader to gitea sites | ||
type GiteaLocalUploader struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO since this is already on the local Gitea instance, the word Local
is double here.
The name should also make it more clear this is a migrator from github (is it?) and not just something "to move local git repos around".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kolaente there are uploaders and downloaders here. GiteaLocalUploader
will save data via gitea's local operations. Currently we have GithubV3Downloader
, in future, we could implement GitLab/Gitea/Gogs/... downloaders. Furthermore we could have GithubV3Uploader
so that we can migrate gitea's data to github.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the flow is GithubV3Uploader
-> GiteaLocalUploader
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current flow is GithubV3Downloader
- > GiteaLocalUploader
. Always from a downloader interface implementation to an uploader interface implementation.
} | ||
|
||
// CreateRepo creates a repository | ||
func (g *GiteaLocalUploader) CreateRepo(repo *base.Repository, includeWiki bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this (and a lot of other functions in this file) different from the implementation in models/migrate.go
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file will invoke models/migrate.go
when migrate git/wiki data.
modules/migrations/github.go
Outdated
labels = append(labels, convertGithubLabel(l)) | ||
} | ||
// FIXME: This API missing reactions, we may need another extra request to get reactions | ||
/*var reactions *Reactions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the commented code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
modules/migrations/migrate.go
Outdated
|
||
if opts.PullRequests { | ||
log.Trace("migrating pull requests and comments") | ||
prs, err := downloader.GetPullRequests(0, 1000000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this magic number do? I'd guess it's a limit of some kind, but what if there are > 1000000? Will it fail? Will it just not downlod anything > 1000000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved.
public/js/index.js
Outdated
@@ -979,6 +979,23 @@ function initRepository() { | |||
} | |||
} | |||
|
|||
var toggleMigrations = function(){ | |||
console.log("toggleMigrations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this console.log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
public/js/index.js
Outdated
@@ -979,6 +979,23 @@ function initRepository() { | |||
} | |||
} | |||
|
|||
var toggleMigrations = function(){ | |||
console.log("toggleMigrations") | |||
if (!$('#mirror').is(":checked") && $('#auth_username').val().length > 0 && ($('#clone_addr').val().startsWith("https://github.com") || $('#clone_addr').val().startsWith("http://github.com"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this only be triggered when a user loads the page and the clone url is already in the clone field? IMHO this should listen on input.
routers/repo/repo.go
Outdated
@@ -263,11 +290,12 @@ func MigratePost(ctx *context.Context, form auth.MigrateRepoForm) { | |||
// remoteAddr may contain credentials, so we sanitize it | |||
err = util.URLSanitizedError(err, remoteAddr) | |||
|
|||
if repo != nil { | |||
// TODO: remove | |||
/*if repo != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this commented code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@lunny One minor issue I noticed is that wrong github passwords gives just a big error 500 screen, that should not happen. |
@kolaente When you check |
@ashimokawa yes, I will fix that. |
@ashimokawa auth error handled. |
@lunny What about introducing a new database column for comments "original_author" or something where "github/ashimokawa" is stored for example? Then if it is confirmed later that ashimokawa signed up on the gitea instance where the repo was moved to, the admin could reassign the comments to the real local user ashimokawa. |
@ashimokawa I will not do that change since it's not possible to let all repository related users register in this Gitea instance and you cannot mapping them in fact. It will not result in difficult on maintain all the issues and pull requests even if the posters are not the original authors. |
@lunny |
@ashimokawa that could be next step for a private repository because I think there are many work to do. Currently I'll focus on public repositories. |
@jolheiser done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, my one concern is that with a large project migration API calls might hit rate limit quota, but I think adding in a GitHub token for auth to increase API rate limit can be done in another PR.
@techknowlogick that's already supported on this PR. you could put your token on username inputbox and then that's it. |
@lunny oh that's perfect! I missed that. Thanks for telling me :) |
The token-on-username thing is not documented anywhere, or did I miss it ? |
It is mentioned on the tip above the input boxes. |
Also, from gitea.com when selecting "This repo will be a mirror" the options to also sync issues/labels/releases etc. disappears. Is that not supported ? |
The migration target page now became a 500 error on https://gitea.com/QGIS/QGIS ... |
mirroring is not supported now. When it's migrating, it maybe failed on UI but it may still running on backend. That's what my new PR wants to resolve. See #6200 |
This PR will improve the migrations process. It will define an interface to migrate data from external git service and at first implement from github to gitea. Below is the TODO list:
Below will be ignored
The new migration UI looks like:
Next steps after this merged
GiteaDownloadV1
so that repositories could be transfer cross gitea instances.GitlabDownload
BitBucketDownload
GogsDownloadV1