Sync releases table with tags on push and for mirrors #2459

lafriks · 2017-09-04T02:21:15Z

~~Not fully tested but ready for review~~

ethantkoenig · 2017-09-04T05:14:40Z

models/release.go

+
+// SyncReleasesWithTags synchronizes release table with repository tags
+func SyncReleasesWithTags(repo *Repository, gitRepo *git.Repository) error {
+	checked := make([]string, 100)


Could we use a map[string]struct{} instead? It will be more efficient to check if tags have been checked below with a map.

ethantkoenig · 2017-09-04T05:20:50Z

routers/api/v1/repo/release.go

-		ctx.Error(500, "Commit", err)
+
+	if !rel.IsTag {
+		ctx.Status(409)


If it is typo than it was also previously as I kept previous behaviour, it was returning status 409 when release already existed

I think it is not a typo:

gitea/routers/api/v1/repo/release.go

Line 97 in 93a1de4

ctx.Status(409)

lunny · 2017-09-04T05:23:35Z

models/migrations/v39.go

+func releaseAddColumnIsTagAndSyncTags(x *xorm.Engine) error {
+	if err := x.Sync2(new(ReleaseV39)); err != nil {
+		return fmt.Errorf("Sync2: %v", err)
+	} else if _, err = x.Cols("is_tag").Update(&ReleaseV39{IsTag: false}); err != nil {


Why update all are false ?

default, probably not needed

lunny · 2017-09-04T05:24:23Z

models/migrations/v39.go

+		if len(repos) == 0 {
+			break
+		}
+		offset += 10


offset += len(repos)

lunny · 2017-09-04T05:29:44Z

models/release.go


-	if _, err = x.Id(rel.ID).Delete(new(Release)); err != nil {
-		return fmt.Errorf("Delete: %v", err)
+		if _, err = x.Id(rel.ID).Delete(new(Release)); err != nil {


update after delete?

forgot to remove update, good catch, thx

actually it is just in diff looks like this, if you look at the full file code there is no update before delete, update is from other function :)

bkcsoft · 2017-09-04T11:57:50Z

So, why do we need to do this? Tags and Releases are not the same thing, they should not be treated as the same thing... If we need this list it should be fetched from the GitRepo directly 🤔

lafriks · 2017-09-04T12:18:43Z

@bkcsoft this is only way to efficiently implement release page paging and to have release page match github behavior. Releases are actually just extenended tags so can say they are quite the same

lunny · 2017-09-04T12:40:54Z

@lafriks I think maybe @bkcsoft is right. Since user will input username and password, we can retrieves release information via github API and inserted to gitea.( maybe an checkbox option: sync releases ). And this sync function also could be a button on tags UI to publish all the tags.

lafriks · 2017-09-04T14:00:33Z

@bkcsoft @lunny it's not about just migration from github it's about displaying tags that are not linked to releases in release page like github is doing:

And it does not matter if repository is migrated or tags are created from git client. Not everyone wants or need to create releases for each tag but it is useful when creating tag to see kind of release list and only if needed create release with additional info and attachments. This is especially important for mirrored repositories and they can also be mirrored from github/gitlab etc

lafriks · 2017-09-04T14:06:48Z

And also this was the functionality gitea had before release page was rewritten to show only data from database (I'm not against it)

lunny · 2017-09-05T00:44:03Z

@lafriks OK. I see.

lunny · 2017-09-05T03:22:46Z

LGTM

bkcsoft · 2017-09-05T04:31:56Z

we can retrieves release information via github API

@lunny I thought that any non-repo migration/sync would be left to third-party tools?

lafriks · 2017-09-05T05:13:16Z

@bkcsoft but this PR is not related to it in any way but otherwise I agree 3rd-party sync/migration should be external tool

ethantkoenig · 2017-09-07T18:39:32Z

models/release.go

@@ -246,6 +248,9 @@ func (opts *FindReleasesOptions) toConds(repoID int64) builder.Cond {
 	if !opts.IncludeDrafts {
 		cond = cond.And(builder.Eq{"is_draft": false})
 	}
+	if !opts.IncludeTags {
+		cond = cond.And(builder.Eq{"is_release": false})


Should this be "is_tag"?

ethantkoenig · 2017-09-07T18:48:58Z

routers/api/v1/repo/release.go

 	if err != nil {
-		ctx.Error(500, "GetTag", err)
+		if models.IsErrReleaseNotExist(err) {


I'm confused here. If we are creating a release, why do we expect there to already be a release with the name form.TagName?

because release table does now contains also tags that are not full releases with attachments etc and when creating release it is needed to update release table flag (is_tag to false) and adding additional release information to that row

bkcsoft · 2017-09-07T20:57:07Z

@lafriks Mainly just thinking out loud 🙂

lafriks · 2017-09-08T07:40:16Z

@ethantkoenig fixed and also refactored code to minimize chance of possible race conditions

lunny · 2017-09-13T06:12:32Z

@lafriks and this one.

Minimize posibility of race conditions

codecov-io · 2017-09-16T21:57:37Z

Codecov Report

Merging #2459 into master will decrease coverage by 0.22%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2459      +/-   ##
==========================================
- Coverage   27.21%   26.99%   -0.23%     
==========================================
  Files          85       85              
  Lines       16955    17097     +142     
==========================================
  Hits         4615     4615              
- Misses      11665    11807     +142     
  Partials      675      675

Impacted Files	Coverage Δ
models/repo_mirror.go	`0.61% <0%> (-0.04%)`	⬇️
models/update.go	`10.69% <0%> (-8.18%)`	⬇️
models/repo.go	`13.5% <0%> (-0.03%)`	⬇️
models/release.go	`0% <0%> (ø)`	⬆️

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 8b6236d...3927a9a. Read the comment docs.

lafriks · 2017-09-17T16:41:11Z

@lunny I think I found bug but have to still reproduce that, do not merge yet. Would be great if someone could test it on his dev environment

ethantkoenig · 2017-09-17T16:54:19Z

models/release.go

+	existingRelTags := make(map[string]struct{})
+	opts := FindReleasesOptions{IncludeDrafts: true, IncludeTags: true}
+	page := 0
+	for {


nit: for page := 1; ; page++ {

lafriks · 2017-09-17T22:20:46Z

@ethantkoenig fixed your suggestion
@lunny I think I fixed the bug but would appreciate if someone could test it more. At least for me it seems to work correctly now

daviian · 2017-09-17T23:33:52Z

@lafriks Found two issues.

git treats tags case sensitive, therefore using toLowerCase could drop a tag
if a tag is removed and added on a new commit sync doesn't update tag reference

But maybe there are more.

daviian · 2017-09-18T15:25:28Z

@lafriks Ok, I have finished my examination. Last commit definitely fixes bugs regarding release creation.
So above mentioned issues are still a problem.

As toLowerCase isn't an issue created with this PR it should be addressed separately.
But IMO tag reference update should be fixed before merging.

ethantkoenig · 2017-09-20T03:02:57Z

~~@lafriks I believe I found a bug.~~ Never mind, was running an old version.

Steps to reproduce:

Create a mirror repository of a Github repo with releases (for example, I used https://github.com/ethantkoenig/run_lambda)
Make the repository a regular, non-mirror repository
Clone the repo, and try deleting a tag (e.g. git push origin :v1.0.4)
The release count remains unchanged, and the deleted tag still shows up in the list of release. When you try to view the deleted tag, you get a 404.

ethantkoenig

Never mind

ethantkoenig · 2017-09-20T04:55:25Z

LGTM

ethantkoenig · 2017-09-20T04:55:57Z

Make L-G-T-M work

lunny · 2017-09-20T05:09:25Z

I made a mistake. @lafriks

* Sync releases table with tags on push and for mirrors * Code style fixes * Fix api to return only releases * Optimize release creation and update Minimize posibility of race conditions * Fix release lower tag name updating * handle tag reference update by addionally comparing commit id

lafriks added the issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change label Sep 4, 2017

lafriks added this to the 1.x.x milestone Sep 4, 2017

lafriks force-pushed the sync_releases_tags branch from 31c1b9e to 5de49c8 Compare September 4, 2017 03:27

lafriks modified the milestones: 1.3.0, 1.x.x Sep 4, 2017

lafriks added the backport/v1.2 label Sep 4, 2017

ethantkoenig suggested changes Sep 4, 2017

View reviewed changes

lunny reviewed Sep 4, 2017

View reviewed changes

models/migrations/v39.go Outdated

if len(repos) == 0 {

break

}

offset += 10

Copy link

Member

lunny Sep 4, 2017

Choose a reason for hiding this comment

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

offset += len(repos)

lafriks reacted with thumbs up emoji

lunny reviewed Sep 4, 2017

View reviewed changes

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 4, 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 Sep 5, 2017

ethantkoenig suggested changes Sep 7, 2017

View reviewed changes

lafriks force-pushed the sync_releases_tags branch from f2d2992 to 6e160c3 Compare September 8, 2017 05:39

lafriks added 3 commits September 17, 2017 00:35

Sync releases table with tags on push and for mirrors

1924c25

Code style fixes

7a4e467

Fix api to return only releases

8b78ce7

Optimize release creation and update

b2a9ea0

Minimize posibility of race conditions

lafriks force-pushed the sync_releases_tags branch from cd6235b to b2a9ea0 Compare September 16, 2017 21:37

daviian mentioned this pull request Sep 17, 2017

Complete push webhooks #2530

Merged

ethantkoenig approved these changes Sep 17, 2017

View reviewed changes

Fix release lower tag name updating

93f8157

handle tag reference update by addionally comparing commit id

15171ac

daviian mentioned this pull request Sep 18, 2017

Tags of releases are not case sensitive while git tags are #2543

Open

7 tasks

ethantkoenig suggested changes Sep 20, 2017

View reviewed changes

ethantkoenig approved these changes Sep 20, 2017

View reviewed changes

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 Sep 20, 2017

Merge branch 'master' into sync_releases_tags

3927a9a

lunny merged commit 7a02978 into go-gitea:master Sep 20, 2017

lafriks added the backport/done All backports for this PR have been created label Sep 20, 2017

lunny mentioned this pull request Sep 20, 2017

Backport: Sync releases table with tags on push and for mirrors (#2459) #2554

Merged

strk mentioned this pull request Dec 21, 2017

Releases page don't show anything if git version less than 1.8 #1133

Closed

ethantkoenig mentioned this pull request Dec 30, 2017

All releases lost upon upgrading from Gogs to Gitea-1.3.2 #3247

Closed

7 tasks

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

Sync releases table with tags on push and for mirrors #2459

Sync releases table with tags on push and for mirrors #2459

Conversation

lafriks commented Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Sep 4, 2017

lafriks commented Sep 4, 2017 • edited Loading

lunny commented Sep 4, 2017

lafriks commented Sep 4, 2017

lafriks commented Sep 4, 2017

lunny commented Sep 5, 2017

lunny commented Sep 5, 2017

bkcsoft commented Sep 5, 2017

lafriks commented Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Sep 7, 2017

lafriks commented Sep 8, 2017

lunny commented Sep 13, 2017

codecov-io commented Sep 16, 2017 • edited Loading

Codecov Report

lafriks commented Sep 17, 2017

Choose a reason for hiding this comment

lafriks commented Sep 17, 2017

daviian commented Sep 17, 2017

daviian commented Sep 18, 2017

ethantkoenig commented Sep 20, 2017 • edited Loading

ethantkoenig left a comment • edited Loading

Choose a reason for hiding this comment

ethantkoenig commented Sep 20, 2017

ethantkoenig commented Sep 20, 2017

lunny commented Sep 20, 2017

lafriks commented Sep 4, 2017 •

edited

Loading

lafriks Sep 4, 2017 •

edited

Loading

lafriks commented Sep 4, 2017 •

edited

Loading

lafriks commented Sep 5, 2017 •

edited

Loading

codecov-io commented Sep 16, 2017 •

edited

Loading

ethantkoenig commented Sep 20, 2017 •

edited

Loading

ethantkoenig left a comment •

edited

Loading