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

Avoid bad database state after failed migration #7040

Merged
merged 2 commits into from May 25, 2019
Merged

Avoid bad database state after failed migration #7040

merged 2 commits into from May 25, 2019

Conversation

rfwatson
Copy link
Contributor

Ensure g.repo is not nil after a failed migration, this allows the cleanup here to be triggered as expected.

Fixes #7039

@rfwatson
Copy link
Contributor Author

Is there any docs for setting up integration tests locally? I'll add a test case if I can get them running

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

make test-sqlite will run the integration tests on sqlite which is probably all you need to run unless you're very keen.

I think CONTRIBUTING.md has some information about how to run drone to perform a pull-request check.

drone exec --local --build-event pull_request

@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 May 25, 2019
@zeripath zeripath added this to the 1.9.0 milestone May 25, 2019
@rfwatson
Copy link
Contributor Author

Thanks.

Out of interest, is there a reason we don’t rollback a database transaction here, instead of cleaning up manually?

@zeripath
Copy link
Contributor

It's not in a transaction unfortunately. Keeping it in a transaction would cause the entire database to be locked whilst you're migrating potentially large repositories.

@codecov-io
Copy link

codecov-io commented May 25, 2019

Codecov Report

Merging #7040 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7040      +/-   ##
==========================================
- Coverage   41.45%   41.44%   -0.01%     
==========================================
  Files         442      442              
  Lines       59597    59597              
==========================================
- Hits        24703    24701       -2     
- Misses      31665    31667       +2     
  Partials     3229     3229
Impacted Files Coverage Δ
modules/migrations/gitea.go 8.3% <100%> (ø) ⬆️
models/repo_list.go 72.08% <0%> (-1.02%) ⬇️

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 355ab0c...e7240e9. Read the comment docs.

@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 May 25, 2019
@zeripath zeripath merged commit daaae2a into go-gitea:master May 25, 2019
@lunny
Copy link
Member

lunny commented May 26, 2019

@rfwatson good catch.

jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
@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/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed migration leaves database in a bad state
6 participants