Implement sync push mirror on commit by harryzcy · Pull Request #19411 · go-gitea/gitea

harryzcy · 2022-04-15T22:15:09Z

Support synchronizing with the push mirrors whenever new commits are pushed or synced from pull mirror.

Related Issues: #18220

Breaking changes:

field SyncOnCommit is added to PushMirror struct, thus a database migration is necessary.

TODO:

update UI to add "sync on push" option
database migration

Screenshot: (the checkbox "Sync when new commit is pushed")

lunny · 2022-04-17T10:08:37Z

Since services/mirror is not too big, Is it necessary to split it as two packages?

harryzcy · 2022-04-17T21:49:54Z

In services/mirror, functions dealing with pull mirrors would send new notifications. Then services/mirror imports modules/notification, which then imports modules/notification/mirror, which then must call functions in services/mirror again. It causes a circular imports.

To avoid it, push mirror related functions must be in a new package, so that:

services/mirror
- import modules/notification
  - import modules/notification/mirror
    - import services/pushmirror (can't be services/mirror again)

lunny · 2022-04-18T11:49:57Z

In services/mirror, functions dealing with pull mirrors would send new notifications. Then services/mirror imports modules/notification, which then imports modules/notification/mirror, which then must call functions in services/mirror again. It causes a circular imports.

To avoid it, push mirror related functions must be in a new package, so that:

services/mirror

import modules/notification

import modules/notification/mirror

import services/pushmirror (can't be services/mirror again)

To avoid import cycle, we can move some into modules/mirror.

harryzcy · 2022-04-21T05:26:08Z

In services/mirror, functions dealing with pull mirrors would send new notifications. Then services/mirror imports modules/notification, which then imports modules/notification/mirror, which then must call functions in services/mirror again. It causes a circular imports.
To avoid it, push mirror related functions must be in a new package, so that:

services/mirror

import modules/notification

import modules/notification/mirror

import services/pushmirror (can't be services/mirror again)

To avoid import cycle, we can move some into modules/mirror.

What exactly differentiates packages in modules and services? I find the contribution guideline a bit vague on this. And the guideline says "Uses models and modules to handle the request," but there's no requests involved as well. I'm not too sure which part fits modules/mirror.

Sorry that I'm new to gitea contributions. Any more suggestions would be much appreciated.

harryzcy · 2022-04-21T06:02:57Z

I still think push mirror and pull mirror have to be separated. That will allow the event pipelines to flow from pull sync to push sync, through notifications.

graph LR;
    A["pull sync"] --> C["new commits"];
    B["git push"] --> C;
    C --> D["push sync"];

Separation of two mirror types would be the most natural fit for this event flow. Again, any other ideas welcomed.

harryzcy · 2022-06-03T05:27:55Z

Now it's using a modules/mirror package and the separate pushmirror package has been removed.

The functions related push mirrors will be called by notification module, when sync-on-push is supported. So relevant functions are moved to pushmirror service package to avoid circular imports.

Some functions in services/mirror are moved to modules/mirror package. This approach solves the circular imports problem without introducting a seperate pushmirror package.

codecov-commenter · 2022-06-29T05:42:11Z

Codecov Report

❗ No coverage uploaded for pull request base (main@c174bdc). Click here to learn what that means.
The diff coverage is 35.29%.

@@           Coverage Diff           @@
##             main   #19411   +/-   ##
=======================================
  Coverage        ?   46.91%           
=======================================
  Files           ?      975           
  Lines           ?   134972           
  Branches        ?        0           
=======================================
  Hits            ?    63320           
  Misses          ?    63893           
  Partials        ?     7759

Impacted Files	Coverage Δ
routers/api/v1/repo/mirror.go	`71.42% <0.00%> (ø)`
services/forms/repo_form.go	`40.76% <ø> (ø)`
services/mirror/mirror.go	`11.95% <7.14%> (ø)`
modules/mirror/mirror.go	`16.00% <16.00%> (ø)`
modules/notification/mirror/mirror.go	`53.33% <53.33%> (ø)`
routers/web/repo/setting.go	`16.92% <71.42%> (ø)`
models/repo/pushmirror.go	`78.00% <100.00%> (ø)`
modules/notification/notification.go	`93.02% <100.00%> (ø)`

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 c174bdc...bc1afc1. Read the comment docs.

Co-authored-by: delvh <dev.lh@web.de>

zeripath · 2022-07-06T08:10:41Z

Assuming this is not backported to v1.17 - once we merge this we will not be able to merge any more migrations - including fixups.

Are we certain that all of our 1.17 migrations are correct?

Co-authored-by: zeripath <art27@cantab.net>

Co-authored-by: delvh <dev.lh@web.de>

zeripath · 2022-07-08T19:42:47Z

make lgtm work

* giteaoffical/main: Implement sync push mirror on commit (go-gitea#19411) Use git.HOME_PATH for Git HOME directory (go-gitea#20114)

Support synchronizing with the push mirrors whenever new commits are pushed or synced from pull mirror. Related Issues: go-gitea#18220 Co-authored-by: delvh <dev.lh@web.de> Co-authored-by: zeripath <art27@cantab.net> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

Elara6331 · 2022-09-30T23:43:51Z

I'm currently testing this feature using 1.18.0+dev-478-g08609d439. There seem to be a few issues. First of all, looking at the database, it seems to have set all my existing push mirrors to sync on commit automatically when I updated Gitea. Second, checking the "Sync when commits are pushed" checkbox seems to do nothing. I tried with it checked and unchecked, and both times, the new mirror had sync_on_commit set to false in the database. The syncing only started working once I manually set it to true in the database.

techknowlogick · 2022-10-01T02:55:43Z

@Arsen6331 please open a new issue for this, as otherwise things get lost in closed threads.

lunny reviewed Apr 17, 2022

View reviewed changes

Comment thread modules/notification/mirror/mirror.go Outdated

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 17, 2022

Gusted added this to the 1.17.0 milestone May 3, 2022

lunny modified the milestones: 1.17.0, 1.18.0 Jun 3, 2022

harryzcy force-pushed the sync-on-push branch from 632c7e4 to e671074 Compare June 3, 2022 04:58

harryzcy requested a review from lunny June 9, 2022 21:03

harryzcy added 9 commits June 29, 2022 01:11

Update pushmirror model to allow sync-on-push

ac61c17

Move mirror push functions to a pushmirror service

04d7bd5

The functions related push mirrors will be called by notification module, when sync-on-push is supported. So relevant functions are moved to pushmirror service package to avoid circular imports.

Implement sync-on-push via a mirrorNotifier

bd55411

Fix lint-backend issues

8f87595

Add migration for PushMirror struct

eb25993

Improve filtering for push mirrors

bcabe51

Refactor to use modules/mirror

c6a4f30

Some functions in services/mirror are moved to modules/mirror package. This approach solves the circular imports problem without introducting a seperate pushmirror package.

Update tests after moving code to modules/mirror

2978942

Include copyright

6c0e721

harryzcy force-pushed the sync-on-push branch from 4c4211f to 6c0e721 Compare June 29, 2022 05:17

harryzcy mentioned this pull request Jun 29, 2022

Plans for Gitea v1.18 #20022

Closed

harryzcy added 4 commits June 29, 2022 03:31

Add 'sync on commit' checkbox in web UI

0836473

Update checkbox input

5a8d2b5

Store sync on commit setting

88f50e9

Use more generic name SyncOnCommit

c9d6393

harryzcy changed the title ~~[WIP] Implement sync push mirror on push~~ [WIP] Implement sync push mirror on commit Jun 30, 2022

delvh reviewed Jul 5, 2022

View reviewed changes

harryzcy and others added 4 commits July 5, 2022 19:41

Apply suggestions from code review

92603fc

Co-authored-by: delvh <dev.lh@web.de>

Fix minor errors from code review

67fa209

Enable sync on commit by default

740408e

Set SyncOnCommit's default to true in models

2210235

zeripath reviewed Jul 6, 2022

View reviewed changes

Comment thread models/repo/pushmirror.go Outdated

Update comment to match function name

bc1afc1

Co-authored-by: zeripath <art27@cantab.net>

zeripath approved these changes Jul 7, 2022

View reviewed changes

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 Jul 7, 2022

zeripath and others added 2 commits July 7, 2022 18:53

Merge branch 'main' into sync-on-push

6320a5d

Merge branch 'main' into sync-on-push

6aab911

delvh approved these changes Jul 8, 2022

View reviewed changes

Comment thread modules/mirror/mirror.go Outdated

Apply suggestions from code review

b7331e9

Co-authored-by: delvh <dev.lh@web.de>

zeripath merged commit 49f9d43 into go-gitea:main Jul 8, 2022

harryzcy deleted the sync-on-push branch July 8, 2022 21:03

zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 9, 2022

Merge remote-tracking branch 'giteaoffical/main'

62c4c96

* giteaoffical/main: Implement sync push mirror on commit (go-gitea#19411) Use git.HOME_PATH for Git HOME directory (go-gitea#20114)

harryzcy mentioned this pull request Jul 14, 2022

Synchronize push mirror on push #18220

Closed

6543 added a commit to 6543-forks/gitea that referenced this pull request Jul 30, 2022

backport migration part of go-gitea#19411 only

72170c8

go-gitea locked and limited conversation to collaborators Oct 1, 2022

lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed type/enhancement An improvement of existing functionality labels Nov 24, 2022

Uh oh!

Conversation

harryzcy commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lunny commented Apr 17, 2022

Uh oh!

harryzcy commented Apr 17, 2022

Uh oh!

lunny commented Apr 18, 2022

Uh oh!

harryzcy commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harryzcy commented Apr 21, 2022

Uh oh!

harryzcy commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeripath commented Jul 6, 2022

Uh oh!

Uh oh!

Uh oh!

zeripath commented Jul 8, 2022

Uh oh!

Elara6331 commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

techknowlogick commented Oct 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

harryzcy commented Apr 15, 2022 •

edited

Loading

harryzcy commented Apr 21, 2022 •

edited

Loading

harryzcy commented Jun 3, 2022 •

edited

Loading

codecov-commenter commented Jun 29, 2022 •

edited

Loading

Elara6331 commented Sep 30, 2022 •

edited

Loading