Mirror sync interval specified as duration string #1407

cez81 · 2017-03-29T10:32:49Z

My attempt on solution for #1140. Would love to hear your comments if I'mon the right track?

Mirror.Interval is changed from int to string so that would require a migration script?

strk · 2017-03-29T10:39:06Z

Changing a table column type would require a migration, yes.
Keeping the integer but changing semantic (for example: have them interpreted as minutes) would also require a migration.

I guess you'll want admins to be able to limit the minimum frequency of migrations (if allowed at all).

lunny

And it should have a minimal duration, for example 1m ?

lunny · 2017-03-29T11:36:32Z

models/repo.go

@@ -811,11 +811,15 @@ func MigrateRepository(u *User, opts MigrateRepoOptions) (*Repository, error) {
 	}

 	if opts.IsMirror {
+		duration, err := time.ParseDuration(setting.Mirror.DefaultInterval)


compitable?

Also, just a note, time.ParseDuration accepts negative durations. This should be checked for.

cez81 · 2017-03-29T19:26:45Z

I have taken your comments on board and done a few changes.

Added a new setting mirror.MinInterval with 10 minutes as default
MinInterval must be >1 minute or defaults to 1 minute
DefaultInterval must be > MinInterval or defaults to 8h
Set Interval must be >MinInterval
Changed back to storing the interval as int in the database (store as time.Duration ie nanoseconds)
I use duration more to save me from parsing the duration string all the time

Ps. Haven't done anything with migration yet until settled on the other bits first.

lunny · 2017-03-30T00:59:28Z

modules/setting/setting.go

-	if Mirror.DefaultInterval <= 0 {
-		Mirror.DefaultInterval = 24
+	sec = Cfg.Section("mirror")
+	Mirror.MinInterval = sec.Key("MIN_INTERVAL").MustDuration(time.Second * 55)


why is time.Secdon * 55?

Oops they shouldn't be there. Removed as MustDuration will return 0 on error and that will be picked up with the following if cases.

cez81 · 2017-03-30T10:07:34Z

modules/setting/setting.go

@@ -903,8 +903,8 @@ please consider changing to GITEA_CUSTOM`)
 	}

 	sec = Cfg.Section("mirror")
-	Mirror.MinInterval = sec.Key("MIN_INTERVAL").MustDuration(time.Second * 55)
-	Mirror.DefaultInterval = sec.Key("DEFAULT_INTERVAL").MustDuration(time.Hour * 55)
+	Mirror.MinInterval = sec.Key("MIN_INTERVAL").MustDuration()


Hmm somethings not right with this. Doesn't read default from built in app.ini

lunny · 2017-04-06T03:19:56Z

models/migrations/v27.go

+	}
+	for _, mirror := range mirrors {
+		mirror.Interval = mirror.Interval * time.Hour
+		_, err := x.Id(mirror.ID).Cols("interval").Update(mirror)


This may needs a transaction.

lunny · 2017-04-06T03:22:33Z

routers/repo/setting.go

-		if form.Interval > 0 {
+		interval, err := time.ParseDuration(form.Interval)
+		if err != nil || interval < setting.Mirror.MinInterval {
+			ctx.Handle(500, "UpdateMirror", err)


Show a hint is better than give a 500.

lunny · 2017-04-06T03:23:07Z

Otherwise LGTM

lunny · 2017-04-06T08:04:55Z

models/migrations/v27.go

+		}
+	}
+
+	if err := sess.Commit(); err != nil {


return sess.Commit()

is more cleaner.

pgaskin · 2017-04-08T14:17:32Z

LGTM

Sync interval specifed as duration string

3372c69

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

cez81 changed the title ~~[WIP] Sync interval specifed as duration string~~ [WIP] Mirror sync interval specified as duration string Mar 29, 2017

lunny requested changes Mar 29, 2017

View reviewed changes

cez81 added 7 commits March 29, 2017 14:23

Changed mirror interval text

29376da

make fmt

91c3aaf

Add MinInterval for mirror sync

5e7c691

Use duration internally

f807596

Changed min default to 10m

266fb0f

make fmt

9b0df2d

Incorrect default

c1c0f8e

lunny reviewed Mar 30, 2017

View reviewed changes

cez81 added 2 commits March 30, 2017 08:05

Removed defaults in MustDuration()

c4c60b7

Add Mirror interval migration

245c1a3

cez81 commented Mar 30, 2017

View reviewed changes

Default values corrected

2e85b22

cez81 changed the title ~~[WIP] Mirror sync interval specified as duration string~~ Mirror sync interval specified as duration string Mar 31, 2017

lunny added this to the 1.2.0 milestone Apr 6, 2017

lunny added the type/enhancement An improvement of existing functionality label Apr 6, 2017

lunny requested changes Apr 6, 2017

View reviewed changes

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 Apr 6, 2017

cez81 added 2 commits April 6, 2017 09:50

Use transaction during migration

27c1ec9

Change http 500 to page with error message

6ea0e06

lunny reviewed Apr 6, 2017

View reviewed changes

lunny approved these changes Apr 6, 2017

View reviewed changes

Cleanup session.commit()

ee651c1

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 Apr 8, 2017

lunny merged commit 54f0293 into go-gitea:master Apr 8, 2017

lunny mentioned this pull request Apr 8, 2017

Allow less than an hour mirror interval #1140

Closed

cez81 deleted the mirror_interval branch April 8, 2017 16:12

sapk mentioned this pull request Apr 16, 2017

My new problem after updating Ubuntu to 17.04 #1496

Closed

cez81 mentioned this pull request Apr 17, 2017

Fix broken v27 migration - change mirror interval from int to bigint #1504

Merged

bwenrich mentioned this pull request Sep 22, 2019

Mirror sync interval time < 10min results in an error "Das Spiegel-Intervall ist ungültig." #8252

Closed

5 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mirror sync interval specified as duration string #1407

Mirror sync interval specified as duration string #1407

cez81 commented Mar 29, 2017

strk commented Mar 29, 2017

lunny left a comment

lunny Mar 29, 2017

pgaskin Mar 29, 2017

cez81 commented Mar 29, 2017 •

edited

Loading

lunny Mar 30, 2017

cez81 Mar 30, 2017

cez81 Mar 30, 2017

lunny Apr 6, 2017

lunny Apr 6, 2017 •

edited

Loading

lunny commented Apr 6, 2017

lunny Apr 6, 2017

pgaskin commented Apr 8, 2017

Mirror sync interval specified as duration string #1407

Mirror sync interval specified as duration string #1407

Conversation

cez81 commented Mar 29, 2017

strk commented Mar 29, 2017

lunny left a comment

Choose a reason for hiding this comment

lunny Mar 29, 2017

Choose a reason for hiding this comment

pgaskin Mar 29, 2017

Choose a reason for hiding this comment

cez81 commented Mar 29, 2017 • edited Loading

lunny Mar 30, 2017

Choose a reason for hiding this comment

cez81 Mar 30, 2017

Choose a reason for hiding this comment

cez81 Mar 30, 2017

Choose a reason for hiding this comment

lunny Apr 6, 2017

Choose a reason for hiding this comment

lunny Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

lunny commented Apr 6, 2017

lunny Apr 6, 2017

Choose a reason for hiding this comment

pgaskin commented Apr 8, 2017

cez81 commented Mar 29, 2017 •

edited

Loading

lunny Apr 6, 2017 •

edited

Loading