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

Various Merge Base fixes #10786

Merged
merged 10 commits into from
Mar 31, 2020
Merged

Various Merge Base fixes #10786

merged 10 commits into from
Mar 31, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 20, 2020

@zeripath zeripath added this to the 1.12.0 milestone Mar 20, 2020
@codecov-io
Copy link

codecov-io commented Mar 20, 2020

Codecov Report

Merging #10786 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #10786    +/-   ##
========================================
  Coverage   43.45%   43.46%            
========================================
  Files         592      593     +1     
  Lines       82905    83005   +100     
========================================
+ Hits        36027    36074    +47     
- Misses      42402    42451    +49     
- Partials     4476     4480     +4     
Impacted Files Coverage Δ
models/migrations/migrations.go 4.16% <ø> (ø)
models/migrations/v128.go 0.00% <0.00%> (ø)
models/migrations/v134.go 0.00% <0.00%> (ø)
routers/repo/pull.go 28.01% <0.00%> (-0.82%) ⬇️
models/unit.go 41.97% <0.00%> (-2.47%) ⬇️
models/pull.go 42.85% <0.00%> (+0.55%) ⬆️
services/pull/pull.go 35.88% <0.00%> (+0.88%) ⬆️
services/pull/patch.go 64.51% <0.00%> (+2.58%) ⬆️
services/pull/temp_repo.go 34.18% <0.00%> (+5.12%) ⬆️
... and 1 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 8cffae6...fa48a13. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 20, 2020
lunny
lunny previously requested changes Mar 21, 2020
models/migrations/v132.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Mar 21, 2020

It's better to add some tests.

@zeripath
Copy link
Contributor Author

Tests? Where can we put them? I'd love it if we had tests for migrations but we don't have infrastructure for them. I'm happy to begin doing that but it's going to be a huge PR.

@lunny
Copy link
Member

lunny commented Mar 21, 2020

We can add a function on modules/git named CalculateMergebase And add some tests on that package.

@zeripath
Copy link
Contributor Author

This function should only be used in the migration otherwise we should simply be storing the actual MergeBase used for PRs. There's no reason to have to recalculate it.

Ideally this fixed migration would only run on PRs that were merged before v128 was run.

@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 Mar 21, 2020
@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 25, 2020
@zeripath
Copy link
Contributor Author

@lunny needs your review

@lafriks
Copy link
Member

lafriks commented Mar 26, 2020

Please resolve conflicts

@zeripath
Copy link
Contributor Author

conflicts resolved.

@lafriks
Copy link
Member

lafriks commented Mar 27, 2020

not anymore :P

@zeripath
Copy link
Contributor Author

eurgh we need a better way of doing migrations!

@zeripath
Copy link
Contributor Author

conflicts resolved again ... 😛

@lafriks
Copy link
Member

lafriks commented Mar 27, 2020

@lunny need your approval

@lunny lunny dismissed their stale review March 31, 2020 10:09

Just don't block this change.

@lafriks
Copy link
Member

lafriks commented Mar 31, 2020

Just tested migration v134.go on copy of my production server and it does fix broken PR's that I had

@lafriks lafriks merged commit 2c25e75 into go-gitea:master Mar 31, 2020
@lafriks
Copy link
Member

lafriks commented Mar 31, 2020

Please send backport

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.

9 participants