Skip to content

Show status check for merged PRs #13975

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

Merged
merged 15 commits into from
Dec 18, 2020
Merged

Show status check for merged PRs #13975

merged 15 commits into from
Dec 18, 2020

Conversation

CirnoT
Copy link
Contributor

@CirnoT CirnoT commented Dec 13, 2020

This PR allows Gitea to show status check for merged PRs:

  • Show status icon on PRs list for PRs whose head was removed by loading data from base repo's pull ref
  • Show list of checks on PR page for merged pulls

chrome_2020-12-13_20-12-41

chrome_2020-12-13_20-13-00

@CirnoT CirnoT marked this pull request as draft December 13, 2020 19:16
@CirnoT
Copy link
Contributor Author

CirnoT commented Dec 13, 2020

Show list of checks on PR page for merged pulls

This is pretty straightforward and I don't see any potential issues with it; basically copies what non-merged PRs do

Show status icon on PRs list for PRs whose head was removed by loading data from base repo's pull ref

This is little more complicated because it changes how status is fetched for PR, maybe @zeripath could chime on on potential performance issues with this

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 13, 2020
@codecov-io
Copy link

codecov-io commented Dec 13, 2020

Codecov Report

Merging #13975 (8ccc9d3) into master (66379ba) will decrease coverage by 0.00%.
The diff coverage is 58.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13975      +/-   ##
==========================================
- Coverage   42.20%   42.19%   -0.01%     
==========================================
  Files         710      710              
  Lines       77273    77280       +7     
==========================================
- Hits        32612    32610       -2     
- Misses      39288    39296       +8     
- Partials     5373     5374       +1     
Impacted Files Coverage Δ
routers/repo/pull.go 31.78% <33.33%> (+0.01%) ⬆️
services/pull/pull.go 41.37% <87.50%> (+0.20%) ⬆️
modules/indexer/stats/queue.go 64.70% <0.00%> (-11.77%) ⬇️
modules/indexer/stats/db.go 43.47% <0.00%> (-8.70%) ⬇️
models/unit.go 46.57% <0.00%> (-2.74%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.54%) ⬇️
models/error.go 38.49% <0.00%> (-0.49%) ⬇️
modules/git/repo_compare.go 69.36% <0.00%> (+5.40%) ⬆️

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 66379ba...8ccc9d3. Read the comment docs.

@CirnoT CirnoT marked this pull request as ready for review December 13, 2020 22:00
@a1012112796 a1012112796 added the topic/ui Change the appearance of the Gitea UI label Dec 14, 2020
@a1012112796 a1012112796 added this to the 1.14.0 milestone Dec 14, 2020
@CirnoT
Copy link
Contributor Author

CirnoT commented Dec 14, 2020

Not sure this qualifies as kind/ui since it doesn't really touch on frontend

@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 Dec 14, 2020
@zeripath
Copy link
Contributor

Not sure this qualifies as kind/ui since it doesn't really touch on frontend

I think essentially this about showing stuff on the UI -> Therefore kind/ui!

@zeripath zeripath added the topic/pr Issues related to pull requests label Dec 14, 2020
@lunny
Copy link
Member

lunny commented Dec 16, 2020

The status information may be deleted when sometime?

@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 Dec 16, 2020
@CirnoT
Copy link
Contributor Author

CirnoT commented Dec 16, 2020

The status information may be deleted when sometime?

We don't remove statuses from database (and we shouldn't, if that's what you ask for - it's a valid piece of information just like comments made by users), so unless someone were to remove it manually (in which case it will not display anything as if commit never had any statuses), no.

@lunny
Copy link
Member

lunny commented Dec 16, 2020

The ancient commit statuses are unuseful, especially for a big repository which has over ten thousands of commits. But they will reduce the performance of commit statuses table. So I think we should allow to delete merged or closed PR's commits statuses which haven't been merged to the git tree.

And of course, the PR is work even if the commit status deleted I think.

@CirnoT
Copy link
Contributor Author

CirnoT commented Dec 16, 2020

Yes the PR will work even if you were to allow removal of old statuses, it will just act as if there was no status and won't show anything (just like right now).

@lunny
Copy link
Member

lunny commented Dec 18, 2020

File conflicts

@6543
Copy link
Member

6543 commented Dec 18, 2020

🚀

@6543 6543 merged commit efa9a8a into go-gitea:master Dec 18, 2020
@CirnoT CirnoT deleted the merged-status branch December 18, 2020 14:34
@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
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. topic/pr Issues related to pull requests topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants