-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Mirror sync interval specified as duration string #1407
Conversation
Changing a table column type would require a migration, yes. I guess you'll want admins to be able to limit the minimum frequency of migrations (if allowed at all). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it should have a minimal duration, for example 1m
?
models/repo.go
Outdated
@@ -811,11 +811,15 @@ func MigrateRepository(u *User, opts MigrateRepoOptions) (*Repository, error) { | |||
} | |||
|
|||
if opts.IsMirror { | |||
duration, err := time.ParseDuration(setting.Mirror.DefaultInterval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compitable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just a note, time.ParseDuration accepts negative durations. This should be checked for.
I have taken your comments on board and done a few changes.
Ps. Haven't done anything with migration yet until settled on the other bits first. |
modules/setting/setting.go
Outdated
if Mirror.DefaultInterval <= 0 { | ||
Mirror.DefaultInterval = 24 | ||
sec = Cfg.Section("mirror") | ||
Mirror.MinInterval = sec.Key("MIN_INTERVAL").MustDuration(time.Second * 55) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is time.Secdon * 55
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
modules/setting/setting.go
Outdated
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm somethings not right with this. Doesn't read default from built in app.ini
models/migrations/v27.go
Outdated
} | ||
for _, mirror := range mirrors { | ||
mirror.Interval = mirror.Interval * time.Hour | ||
_, err := x.Id(mirror.ID).Cols("interval").Update(mirror) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may needs a transaction.
routers/repo/setting.go
Outdated
if form.Interval > 0 { | ||
interval, err := time.ParseDuration(form.Interval) | ||
if err != nil || interval < setting.Mirror.MinInterval { | ||
ctx.Handle(500, "UpdateMirror", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show a hint is better than give a 500.
Otherwise LGTM |
models/migrations/v27.go
Outdated
} | ||
} | ||
|
||
if err := sess.Commit(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return sess.Commit()
is more cleaner.
LGTM |
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?