Add repo-sync-releases admin command #3254

strk · 2017-12-23T00:18:56Z

Will help recovering corrupted database, see #3247

lafriks · 2017-12-23T00:26:00Z

cmd/admin.go

+		Usage:  "Synchronize repository releases with tags",
+		Action: runRepoSyncReleasesWithTags,
+		Flags: []cli.Flag{
+			cli.StringFlag{


These flags seems wrong

flags fixed, please lift blocker ?

lafriks · 2017-12-23T06:57:07Z

Shorter name for command would probably be better, ex. sync-tags

strk · 2017-12-23T09:00:38Z

Please see 57cab54a I've changed the name to "repo-sync" and fixed tags. Only when I run the command locally I don't see a return from models.SyncReleasesWithTags for the second of two repositories I have locally:

2017/12/23 09:57:23 [I] XORM Log Mode: File(Info) 2017/12/23 09:57:23 [T] Found 2 repos 2017/12/23 09:57:23 [T] Synchronizing repo error/test with path /home/strk/gitea-repositories/error/test.git 2017/12/23 09:57:23 [T] currentgNumReleases is 0, running SyncReleasesWithTags 2017/12/23 09:57:23 [W] SyncReleasesWithTags: GetTags: exit status 128 - fatal: Not a git repository (or any of the parent directories): .git 2017/12/23 09:57:23 [T] Repo error/test releases synchronized to tags: from 0 to 0 2017/12/23 09:57:23 [T] Synchronizing repo strk/test with path /home/strk/gitea-repositories/strk/test.git 2017/12/23 09:57:23 [T] currentgNumReleases is 0, running SyncReleasesWithTags

At the end of each loop there should be a "Repo xxx/xxx releases syncrhonized to tags..." message, but you see only one (for the repository with an error, btw) Also, I'm afraid the default paging would limit the scan to just the first page (of a non-infinite default length)

ethantkoenig · 2017-12-25T15:03:36Z

cmd/admin.go

+		}
+
+		oldnum := repo.NumReleases
+		log.Trace(" currentgNumReleases is %d, running SyncReleasesWithTags", oldnum)


ethantkoenig · 2017-12-25T15:03:39Z

cmd/admin.go

+		return fmt.Errorf("models.SetEngine: %v", err)
+	}
+
+	repos, count, err := models.SearchRepositoryByName(&models.SearchRepoOptions{})


2 things:

Should we use paging instead of dragging all repositories into memory at once?

We need set Private to true

See models/repo_indexer.go for an example.

ethantkoenig · 2017-12-25T15:06:45Z

cmd/admin.go

+	repos, count, err := models.SearchRepositoryByName(&models.SearchRepoOptions{})
+	if err != nil {
+		log.Fatal(4, "SearchRepositoryByName: %v", err)
+		return nil


If we return nil, is the return status 0? I think that if we aren't able to even the load the repositories, then the exit status should be non-zero (e.g. if someone is running this command as part of a shell script). Never mind, the return nil is unreachable due to the log.Fatal.

strk · 2017-12-30T10:34:05Z

@ethantkoenig thanks for the review, I addressed all issues with fe1ab765

Maybe the return code should be the number of failed synchronization, but before going there, I'd really like someone else to try the command on their own installation, because I find it behaving in unexpected ways. For example I cloned a managed repository locally, created and pushed a tag, then run the command but the command did not report creating one additional tag...

codecov-io · 2017-12-30T10:41:05Z

Codecov Report

Merging #3254 into master will decrease coverage by 0.05%.
The diff coverage is 3.57%.

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
- Coverage   34.86%   34.81%   -0.06%     
==========================================
  Files         277      277              
  Lines       40210    40263      +53     
==========================================
- Hits        14020    14018       -2     
- Misses      24130    24184      +54     
- Partials     2060     2061       +1

Impacted Files	Coverage Δ
models/repo_list.go	`67.18% <ø> (ø)`	⬆️
cmd/admin.go	`0% <0%> (ø)`	⬆️
models/migrations/v39.go	`0% <0%> (ø)`	⬆️
models/issue_indexer.go	`67.81% <100%> (ø)`	⬆️
models/repo_indexer.go	`50.97% <100%> (-0.98%)`	⬇️
models/repo.go	`43.2% <0%> (-0.19%)`	⬇️
modules/process/manager.go	`81.15% <0%> (+4.34%)`	⬆️

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 ae9cc8f...e8772e3. Read the comment docs.

strk · 2017-12-30T10:46:57Z

Well it turns out SyncReleasesWithTags does not do what I though (and what whoever wrote the migration though) ?

That is, it does not read git repository TAGS and turn them into RELEASES into the database.

This (#3247) is a very nasty migration bug, please give it higher priority

lafriks · 2017-12-31T01:14:34Z

cmd/admin.go

+	for page := 1; ; page++ {
+		repos, count, err := models.SearchRepositoryByName(&models.SearchRepoOptions{
+			Page:     page,
+			PageSize: 10,


Bigger page size would be better I think (50 for ex.)

I was also wondering why 10, only took that value from the example code I was reading: PopulateRepoIndexer here:

gitea/models/repo_indexer.go

Line 82 in f5155b9

repos, _, err := SearchRepositoryByName(&SearchRepoOptions{

But on the other hand, why 50 ? The question is: how much RAM does a single repository information take ? What are we loading, beside the single record, from the DB ? Why not 255 ?

Should we be using a constant for this, to avoid having the same discussion in multiple places ?
For example 20 is used in migration 39:

gitea/models/migrations/v39.go

Line 34 in f5155b9

pageSize := 20

With 59672bf6 I've introduced a new RepositoryListDefaultPageSize constant, set to 64 and used from within the new admin command (to be used by other places too, later)

strk · 2017-12-31T14:10:24Z

So the command finally works, it was just the debug output being wrong, and now it's fixed. Can we merge now ?

Will help recovering corrupted database, see go-gitea#3247

…scan private repos, fix typo

Use it from the new admin command

lafriks · 2017-12-31T14:31:48Z

CI still fails for docs comment

lafriks requested changes Dec 23, 2017

View reviewed changes

strk force-pushed the admin-repo-sync-release branch from a584ae4 to d182fd3 Compare December 23, 2017 00:32

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 23, 2017

strk changed the title ~~Add repo-sync-releases-with-tags admin command~~ Add repo-sync admin command Dec 25, 2017

strk force-pushed the admin-repo-sync-release branch 2 times, most recently from 00e2e78 to f422ada Compare December 25, 2017 09:46

ethantkoenig suggested changes Dec 25, 2017

View reviewed changes

ethantkoenig approved these changes Dec 30, 2017

View reviewed changes

lafriks reviewed Dec 31, 2017

View reviewed changes

strk changed the title ~~Add repo-sync admin command~~ Add repo-sync-releases admin command Dec 31, 2017

tboerger 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 31, 2017

lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 31, 2017

lafriks added this to the 1.4.0 milestone Dec 31, 2017

lafriks approved these changes Dec 31, 2017

View reviewed changes

strk added 5 commits December 31, 2017 15:23

Add repo-sync-releases admin command

ddc51c1

Will help recovering corrupted database, see go-gitea#3247

Load repos in chunks of 10, exit with error if unable to get a list, …

1d44aeb

…scan private repos, fix typo

Fix debug output about num releases

a16360b

Introduce RepositoryListDefaultPageSize constant, set to 64

c425fb9

Use it from the new admin command

Use RepositoryListDefaultPageSize in more places

74b4deb

strk force-pushed the admin-repo-sync-release branch from 054245c to bb04944 Compare December 31, 2017 14:24

tboerger 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 31, 2017

Document RepositoryListDefaultPageSize

e8772e3

strk force-pushed the admin-repo-sync-release branch from bb04944 to e8772e3 Compare December 31, 2017 14:34

lafriks merged commit 8cd987a into go-gitea:master Dec 31, 2017

strk deleted the admin-repo-sync-release branch December 31, 2017 14:47

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add repo-sync-releases admin command #3254

Add repo-sync-releases admin command #3254

strk commented Dec 23, 2017

lafriks Dec 23, 2017

strk Dec 31, 2017

lafriks commented Dec 23, 2017

strk commented Dec 23, 2017 via email

ethantkoenig Dec 25, 2017

ethantkoenig Dec 25, 2017

ethantkoenig Dec 25, 2017 •

edited

Loading

strk commented Dec 30, 2017

codecov-io commented Dec 30, 2017 •

edited

Loading

strk commented Dec 30, 2017

lafriks Dec 31, 2017

strk Dec 31, 2017

strk Dec 31, 2017

strk commented Dec 31, 2017

lafriks commented Dec 31, 2017

Add repo-sync-releases admin command #3254

Add repo-sync-releases admin command #3254

Conversation

strk commented Dec 23, 2017

lafriks Dec 23, 2017

Choose a reason for hiding this comment

strk Dec 31, 2017

Choose a reason for hiding this comment

lafriks commented Dec 23, 2017

strk commented Dec 23, 2017 via email

ethantkoenig Dec 25, 2017

Choose a reason for hiding this comment

ethantkoenig Dec 25, 2017

Choose a reason for hiding this comment

ethantkoenig Dec 25, 2017 • edited Loading

Choose a reason for hiding this comment

strk commented Dec 30, 2017

codecov-io commented Dec 30, 2017 • edited Loading

Codecov Report

strk commented Dec 30, 2017

lafriks Dec 31, 2017

Choose a reason for hiding this comment

strk Dec 31, 2017

Choose a reason for hiding this comment

strk Dec 31, 2017

Choose a reason for hiding this comment

strk commented Dec 31, 2017

lafriks commented Dec 31, 2017

ethantkoenig Dec 25, 2017 •

edited

Loading

codecov-io commented Dec 30, 2017 •

edited

Loading