Skip to content
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

Implement sync push mirror on commit #19411

Merged
merged 34 commits into from
Jul 8, 2022
Merged

Conversation

harryzcy
Copy link
Contributor

@harryzcy harryzcy commented Apr 15, 2022

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")

image

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

lunny commented Apr 17, 2022

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

@harryzcy
Copy link
Contributor Author

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
Copy link
Member

lunny commented Apr 18, 2022

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
Copy link
Contributor Author

harryzcy commented Apr 21, 2022

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
Copy link
Contributor Author

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"];
Loading

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

@Gusted Gusted added this to the 1.17.0 milestone May 3, 2022
@lunny lunny modified the milestones: 1.17.0, 1.18.0 Jun 3, 2022
@harryzcy
Copy link
Contributor Author

harryzcy commented Jun 3, 2022

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

@harryzcy harryzcy requested a review from lunny June 9, 2022 21:03
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
Copy link

codecov-commenter commented Jun 29, 2022

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.

@harryzcy harryzcy mentioned this pull request Jun 29, 2022
@harryzcy harryzcy changed the title [WIP] Implement sync push mirror on push [WIP] Implement sync push mirror on commit Jun 30, 2022
models/migrations/v219.go Outdated Show resolved Hide resolved
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
templates/repo/settings/options.tmpl Outdated Show resolved Hide resolved
modules/notification/mirror/mirror.go Show resolved Hide resolved
models/repo/pushmirror.go Outdated Show resolved Hide resolved
models/migrations/v219.go Outdated Show resolved Hide resolved
modules/notification/mirror/mirror.go Outdated Show resolved Hide resolved
modules/notification/mirror/mirror.go Outdated Show resolved Hide resolved
modules/mirror/mirror.go Outdated Show resolved Hide resolved
modules/mirror/mirror.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor

zeripath commented Jul 6, 2022

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?

models/repo/pushmirror.go Outdated Show resolved Hide resolved
Co-authored-by: zeripath <art27@cantab.net>
@GiteaBot 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
modules/mirror/mirror.go Outdated Show resolved Hide resolved
Co-authored-by: delvh <dev.lh@web.de>
@zeripath
Copy link
Contributor

zeripath commented Jul 8, 2022

make lgtm work

@zeripath zeripath merged commit 49f9d43 into go-gitea:main Jul 8, 2022
@harryzcy 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
* giteaoffical/main:
  Implement sync push mirror on commit (go-gitea#19411)
  Use git.HOME_PATH for Git HOME directory (go-gitea#20114)
dineshsalunke pushed a commit to dineshsalunke/gitea that referenced this pull request Jul 9, 2022
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>
dineshsalunke pushed a commit to dineshsalunke/gitea that referenced this pull request Jul 15, 2022
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>
6543 added a commit to 6543-forks/gitea that referenced this pull request Jul 30, 2022
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
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
Copy link

Elara6331 commented Sep 30, 2022

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
Copy link
Member

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

@go-gitea go-gitea locked and limited conversation to collaborators Oct 1, 2022
@lunny 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants