Add push to remote mirror repository #15157

lunny · 2021-03-25T15:59:23Z

KN4CK3R commented

2021-03-25 15:59:23 +00:00

(Migrated from github.com)

Adds the ability to push-mirror a repository to a remote destination.

ToDo:

Models
integrate into sync queue
UI
Tests
LFS integration (client implementation is proposed in #14726)

close #3480
maybe #2280

Adds the ability to push-mirror a repository to a remote destination. ToDo: - [x] Models - [x] integrate into sync queue - [x] UI - [x] Tests - [x] LFS integration (client implementation is proposed in #14726) ![grafik](https://user-images.githubusercontent.com/1666336/112722941-7b8ab700-8f0c-11eb-85fd-63b7e4af9f35.png) close #3480 maybe #2280

👍 16 👀 2 ❤️ 10 🎉 10

lunny reviewed 2021-03-25 16:23:43 +00:00

services/mirror/mirror_pull.go

lunny commented

2021-03-25 16:23:43 +00:00

2021

KN4CK3R (Migrated from github.com) reviewed 2021-03-26 06:59:14 +00:00

services/mirror/mirror_pull.go

KN4CK3R (Migrated from github.com) commented

2021-03-26 06:59:13 +00:00

Even if the code in the file was just moved from the other file?

lunny reviewed 2021-04-19 12:00:10 +00:00

models/repo_pushmirror.go

lunny commented

2021-04-19 12:00:10 +00:00

A created column will help us sometimes.

A `created` column will help us sometimes.

kdumontnu (Migrated from github.com) requested changes 2021-05-04 20:49:24 +00:00

kdumontnu (Migrated from github.com) left a comment

Tested this locally and it's looking really good! Are you still adding more tests? Asking because the checkbox is unchecked in the description.

Some initial findings:

Looks like user credentials are currently included in the log (at info level) - should be sanitized:

[git-module] stdout:
https://<username>:<password>@<url>.git

When I added a mirror repo with no credentials, gitea tried to use my local keychain credentials. Not sure if this is normal operation, but seems like a security risk.
We'll need to add a tooltip for "Push" mirrors. Something like:

Push mirrors are external repositories used to add a redundancy. Data from this repository will be force pushed to the remote. **Note: this will overwrite any commits in the remote repository.**

Probably also a link to the docs - I can work on this
4. We should add some information about Auth to the right of the field - ex. password should probably be an OAuth token (this is actually required for 2FA logins).
5. Can we hide the "Direction" and "Last update" columns if there are no Mirrors?

6. Errors might be more consistent formatted as a message (this isn't a big deal though):

7. We should add the option to initialize a push mirror in the "New Repository" form. This can be a separate PR.

Tested this locally and it's looking really good! Are you still adding more tests? Asking because the checkbox is unchecked in the description. Some initial findings: 1. Looks like user credentials are currently included in the log (at info level) - should be sanitized: ``` [git-module] stdout: https://<username>:<password>@<url>.git ``` 2. When I added a mirror repo with no credentials, gitea tried to use my local keychain credentials. Not sure if this is normal operation, but seems like a security risk. 3. We'll need to add a tooltip for "Push" mirrors. Something like: ``` Push mirrors are external repositories used to add a redundancy. Data from this repository will be force pushed to the remote. **Note: this will overwrite any commits in the remote repository.** ``` Probably also a link to the docs - I can work on this 4. We should add some information about Auth to the right of the field - ex. password should probably be an OAuth token (this is actually required for 2FA logins). 5. Can we hide the "Direction" and "Last update" columns if there are no Mirrors? ![image](https://user-images.githubusercontent.com/12700993/117065911-49c3f780-ace5-11eb-91a2-c105071ba3ef.png) 6. Errors might be more consistent formatted as a message (this isn't a big deal though): ![image](https://user-images.githubusercontent.com/12700993/117067120-d9b67100-ace6-11eb-9d81-30074c194bff.png) 7. We should add the option to initialize a push mirror in the "New Repository" form. This can be a separate PR.

6543 (Migrated from github.com) reviewed 2021-05-18 16:21:17 +00:00

models/migrations/v180.go

6543 (Migrated from github.com) commented

2021-05-18 16:21:17 +00:00

if we have Interval do we need NextUpdateUnix ?

6543 (Migrated from github.com) reviewed 2021-06-01 19:18:09 +00:00

models/repo_pushmirror.go

6543 (Migrated from github.com) commented

2021-06-01 19:18:08 +00:00

I'm still not sure why we should store NextUpdateUnix in database. Since it can be calculated from LastUpdateUnix & Interval ?

KN4CK3R (Migrated from github.com) reviewed 2021-06-02 15:01:45 +00:00

models/repo_pushmirror.go

KN4CK3R (Migrated from github.com) commented

2021-06-02 15:01:45 +00:00

Was just the same interface as the pull mirror. I removed the field and it should be removed in the pull mirror table too in another PR.

6543 (Migrated from github.com) reviewed 2021-06-03 16:57:19 +00:00

models/repo_pushmirror.go

						
				@ -0,0 +16,4 @@

				var (

					// ErrPushMirrorNotExist mirror does not exist error

					ErrPushMirrorNotExist = errors.New("PushMirror does not exist")

6543 (Migrated from github.com) commented

2021-06-03 16:57:19 +00:00

NOTE: other error types for models package are declared in models/error.go ... throu I don't know if we should declare them in there used files instead ...

this is just a smal code inconsistency

NOTE: other error types for models package are declared in models/error.go ... throu I don't know if we should declare them in there used files instead ... this is just a smal code inconsistency

6543 (Migrated from github.com) reviewed 2021-06-03 16:59:28 +00:00

models/repo_pushmirror.go

						
				@ -0,0 +33,4 @@

				}

				// AfterLoad is invoked from XORM after setting the values of all fields of this object.

				func (m *PushMirror) AfterLoad(session *xorm.Session) {

6543 (Migrated from github.com) commented

2021-06-03 16:59:28 +00:00

I'm not a fan of load it each time intransparent, we should create a "LoadRepo" func for PushMirror struct
And exec it where needed, this way we can also pass the error upwards and not just drop it into the logs

I'm not a fan of load it each time intransparent, we should create a "LoadRepo" func for *PushMirror* struct And exec it where needed, this way we can also pass the error upwards and not just drop it into the logs

KN4CK3R (Migrated from github.com) reviewed 2021-06-05 14:18:46 +00:00

models/repo_pushmirror.go

						
				@ -0,0 +16,4 @@

				var (

					// ErrPushMirrorNotExist mirror does not exist error

					ErrPushMirrorNotExist = errors.New("PushMirror does not exist")

KN4CK3R (Migrated from github.com) commented

2021-06-05 14:18:46 +00:00

I would vote for a location near/in the used file and not a god-file with all errors. Anyway in this case it's not a new error "type". If you search for = errors.New in the code they appear all in the file which uses them.

I would vote for a location near/in the used file and not a god-file with all errors. Anyway in this case it's not a new error "type". If you search for `= errors.New` in the code they appear all in the file which uses them.

KN4CK3R (Migrated from github.com) reviewed 2021-06-05 14:32:23 +00:00

models/repo_pushmirror.go

						
				@ -0,0 +33,4 @@

				}

				// AfterLoad is invoked from XORM after setting the values of all fields of this object.

				func (m *PushMirror) AfterLoad(session *xorm.Session) {

KN4CK3R (Migrated from github.com) commented

2021-06-05 14:32:22 +00:00

That was copied from the pull mirror. While I agree with your intention there is a fine line between being intransparent and cluttering the code every time the object is used. For the mirror structs for example the load of the repository is needed all the time. Maybe it would be useful if AfterLoad could return an error which would be propagated to the caller of Get, Find and Iterate.

That was copied from the pull mirror. While I agree with your intention there is a fine line between being intransparent and cluttering the code every time the object is used. For the mirror structs for example the load of the repository is needed all the time. Maybe it would be useful if `AfterLoad` could return an error which would be propagated to the caller of `Get`, `Find` and `Iterate`.

6543 (Migrated from github.com) reviewed 2021-06-05 14:52:11 +00:00

models/repo_pushmirror.go

						
				@ -0,0 +33,4 @@

				}

				// AfterLoad is invoked from XORM after setting the values of all fields of this object.

				func (m *PushMirror) AfterLoad(session *xorm.Session) {

6543 (Migrated from github.com) commented

2021-06-05 14:52:11 +00:00

hmm an xorm feature request ...

and it looks like the "pull mirror" code need some tuning ...

hmm an xorm feature request ... and it looks like the "pull mirror" code need some tuning ...

6543 (Migrated from github.com) reviewed 2021-06-11 12:17:11 +00:00

models/repo_pushmirror.go

6543 (Migrated from github.com) commented

2021-06-11 12:16:09 +00:00

	m := &PushMirror{}
	has, err := x.ID(ID).Get(m)

```suggestion m := &PushMirror{} has, err := x.ID(ID).Get(m) ```

6543 (Migrated from github.com) commented

2021-06-11 12:16:50 +00:00

	_, err := x.ID(ID).Delete(&PushMirror{})

```suggestion _, err := x.ID(ID).Delete(&PushMirror{}) ```

6543 (Migrated from github.com) reviewed 2021-06-11 13:54:32 +00:00

models/repo_pushmirror.go

6543 (Migrated from github.com) commented

2021-06-11 13:54:32 +00:00

I think you should use xorm.Builder so xorm take care of different sql dialects

6543 (Migrated from github.com) reviewed 2021-06-14 10:52:15 +00:00

models/migrations/v182.go

6543 (Migrated from github.com) commented

2021-06-14 10:52:14 +00:00

	defer sess.Close()

```suggestion defer sess.Close() ```

6543 (Migrated from github.com) approved these changes 2021-06-14 10:53:28 +00:00

lunny requested changes 2021-06-14 11:42:27 +00:00

models/repo_pushmirror.go

lunny commented

2021-06-14 11:42:21 +00:00

LastError should use xorm:"text"
default it's varchar(255).

LastError should use `xorm:"text"` default it's `varchar(255)`.

lunny approved these changes 2021-06-14 14:02:03 +00:00

This repo is archived. You cannot comment on pull requests.

No reviewers

kdumontnu

6543

No Label

hacktoberfest-accepted

kind/upstream-related

status/needs-feedback

No Milestone

No project

No Assignees

1 Participants

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: lunny/gitea#15157