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.

Sign up for GitHub

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

Jump to bottom

models: include LFS when calculating repo size #11060

Merged
merged 3 commits into from
May 31, 2020
Merged

models: include LFS when calculating repo size #11060

merged 3 commits into from
May 31, 2020

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Apr 13, 2020

Fixes #9369

@lafriks
Copy link
Member

lafriks commented Apr 13, 2020

LFS size should probably be saved in other field imho, also it should include migration to add that to existing repos

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 13, 2020
@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 Apr 14, 2020
@lunny lunny added the type/enhancement An improvement of existing functionality label Apr 14, 2020
@lunny lunny added this to the 1.12.0 milestone Apr 14, 2020
@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 14, 2020

LFS size should probably be saved in other field imho, also it should include migration to add that to existing repos

Is there any use case for that?

In my case, when I want to limit user for specific size, then I expect repo.Size will return the whole consumed storage of repo.

6543
6543 approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@6543 6543 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split lfs size in a seperate var has it's up and downs.

so as long as we do not explicit need to split this up (because of a feature e.g. ...) we can leafe it by adding lfs to normal repo size

@lafriks do you have a feature/function already in mind?

@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 Apr 14, 2020
@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 14, 2020

The test failed looks weird.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11060   +/-   ##
=======================================
  Coverage   43.46%   43.47%           
=======================================
  Files         597      597           
  Lines       84706    84738   +32     
=======================================
+ Hits        36821    36840   +19     
- Misses      43344    43351    +7     
- Partials     4541     4547    +6
Impacted Files Coverage Δ
models/repo.go 51.53% <57.14%> (+0.02%) ⬆️
modules/indexer/stats/db.go 50.00% <0.00%> (-9.38%) ⬇️
modules/git/utils.go 65.67% <0.00%> (-4.48%) ⬇️
routers/user/setting/applications.go 41.17% <0.00%> (-4.44%) ⬇️
routers/api/v1/user/app.go 67.61% <0.00%> (-1.76%) ⬇️
modules/log/event.go 64.61% <0.00%> (-1.03%) ⬇️
modules/git/repo.go 47.28% <0.00%> (-0.84%) ⬇️
models/issue_comment.go 46.68% <0.00%> (ø)
models/issue.go 51.48% <0.00%> (+0.07%) ⬆️
models/token.go 74.60% <0.00%> (+0.83%) ⬆️
... and 4 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 980ef24...5e7f063. Read the comment docs.

@cuonglm cuonglm requested a review from lunny April 14, 2020 17:04
@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 14, 2020

@6543 your approval is black, is it problem?

@lafriks
Copy link
Member

lafriks commented Apr 14, 2020

We should split them so that we could allow different limits for git repository size and lfs

@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 14, 2020

We should split them so that we could allow different limits for git repository size and lfs

Currently, LFS is transparent with user, so I don't think it's worth to split them yet. We can easily do it in the future, if necessary.

@6543
Copy link
Member

6543 commented Apr 14, 2020

@6543 your approval is black, is it problem?

no it just means I cant merge

sapk
sapk approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should may be add tests for repo.Size but those are not currently existing. Otherwise LGTM.

@sapk
Copy link
Member

sapk commented Apr 14, 2020 •
edited
Loading

This PR will close one item of #7833. For including or not lfs object in .Size or in a specific field, I think it should be included (as .Size is the global size) and if needed two specific fields could be added for git and lfs specific size info.

@guillep2k
Copy link
Member

I also like the idea of having separate values for the size. LFS objects might be shared among repositories and reside in different places (e.g. cloud storage), which in turn can result in separate policies, so it makes sense to count them separately (and we can display them in the UI as well).

@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 15, 2020

We should may be add tests for repo.Size but those are not currently existing. Otherwise LGTM.

I did add a test in doLFS. Can this come in?

@sapk
Copy link
Member

sapk commented Apr 15, 2020

@cuonglm Yes that is good enough. It is a more broad problem (and previously existing). Currently the only solution to force the updateSize and the size fields to be calculated is to update object in the repo.

@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 19, 2020

@sapk @lunny Can this come in?

@lunny
Copy link
Member

lunny commented Apr 19, 2020

@cuonglm I think we could split the size in future PRs.

@lafriks lafriks modified the milestones: 1.12.0, 1.13.0 May 16, 2020
@cuonglm
Copy link
Contributor Author

cuonglm commented May 20, 2020

@sapk @lunny How is this change going?

There're too many changes in recent master, which makes doLfs function gone.

@techknowlogick
Copy link
Member

ping LG-TM

@techknowlogick techknowlogick merged commit d98df0e into go-gitea:master May 31, 2020
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
@cuonglm @jolheiser @techknowlogick
models: include LFS when calculating repo size (go-gitea#11060)
99ee0eb 
Fixes go-gitea#9369

Co-authored-by: John Olheiser <john.olheiser@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@peterbabic
Copy link

Does this change affect #3658 ?

lunny
lunny reviewed Oct 17, 2020
View reviewed changes
models/repo.go
@@ -806,6 +806,14 @@ func (repo *Repository) updateSize(e Engine) error {
return fmt.Errorf("updateSize: %v", err)
}

objs, err := repo.GetLFSMetaObjects(-1, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A select sum sql is better than this change.

@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.
Reviewers

@lunny lunny lunny approved these changes

@6543 6543 6543 approved these changes

@sapk sapk sapk approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

Repository size in repository management does not account for git LFS data