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

WIP: Move migration git clone to its own goroutine #5949

Closed

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Feb 3, 2019

The current code places the git clone in the POST goroutine, blocking that goroutine until it is finished. This PR asynchronises this, placing the clone within its own goroutine. Fix #3770. Fix #1249.

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

zeripath commented Feb 3, 2019

  • migration goroutine needs panic protection with a recover.
  • need to think about how migration failures are notified. I currently attach these in an action to the repo, but these should be deleted when the repo is... I think there must be a better way of informing errors etc. Turns out this is not possible.. Therefore if the migration fails I will keep the repository around
  • need to fix tests
  • rebase on head fix conflicts

@zeripath zeripath force-pushed the issue-3770-put-clone-in-goroutine branch from e20736d to e9dd53b Compare February 4, 2019 18:03
The current code places the git clone in the POST goroutine, blocking
that goroutine until it is finished. This PR asynchronises this,
placing the clone within its own goroutine. Fix go-gitea#3770
@zeripath zeripath force-pushed the issue-3770-put-clone-in-goroutine branch from e9dd53b to 938026f Compare February 4, 2019 18:05
@codecov-io
Copy link

codecov-io commented Feb 4, 2019

Codecov Report

Merging #5949 into master will increase coverage by 0.04%.
The diff coverage is 48.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5949      +/-   ##
=========================================
+ Coverage   38.86%   38.9%   +0.04%     
=========================================
  Files         345     346       +1     
  Lines       49510   49670     +160     
=========================================
+ Hits        19241   19324      +83     
- Misses      27486   27553      +67     
- Partials     2783    2793      +10
Impacted Files Coverage Δ
models/action.go 60.98% <ø> (ø) ⬆️
modules/templates/helper.go 48.95% <0%> (ø) ⬆️
models/repo.go 46.83% <28.57%> (-0.45%) ⬇️
models/repo_migrate.go 47.98% <47.98%> (ø)
routers/repo/repo.go 21.4% <53.84%> (+0.58%) ⬆️
routers/api/v1/repo/repo.go 56.79% <83.33%> (+0.15%) ⬆️
modules/util/sanitize.go 63.15% <0%> (+63.15%) ⬆️

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 11e3166...986060e. Read the comment docs.

@zeripath
Copy link
Contributor Author

zeripath commented Feb 5, 2019

Ok, this is now working. In terms of things that could be done:

  • At present I just create an empty repository and archive it as a placeholder for the migration. It may be possible to set something in the repository to show it is in migration?
  • I haven't checked what happens should you try to delete this repository whilst it is in migration. That may be horrible... It was horrible - this is now fixed.
  • It may actually make sense to try to recover as much as possible from a failed migration rather than just delete failed clones.

@zeripath
Copy link
Contributor Author

Closing this PR in preference to lunny's

@zeripath zeripath closed this Mar 15, 2019
@zeripath zeripath deleted the issue-3770-put-clone-in-goroutine branch April 22, 2019 20:22
@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/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
4 participants