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.

Sign up for GitHub

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

auto-create repository on push #2478

Closed
wants to merge 1 commit into from
+55 −0
Closed

auto-create repository on push #2478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions conf/app.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ ALLOWED_TYPES =
FILE_MAX_SIZE = 3
; Max number of files per upload. Defaults to 5
MAX_FILES = 5
; Create repository on upload if it doesn't exists
AUTO_CREATE = false

[ui]
; Number of repositories that are showed in one explore page
Expand Down
43 changes: 43 additions & 0 deletions integrations/repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/sdk/gitea"

"github.com/stretchr/testify/assert"
)
Expand Down Expand Up @@ -74,3 +75,45 @@ func TestViewRepo1CloneLinkAuthorized(t *testing.T) {
sshURL := fmt.Sprintf("%s@%s:user2/repo1.git", setting.RunUser, setting.SSH.Domain)
assert.Equal(t, sshURL, link)
}

func TestAutoCreateRepo(t *testing.T) {
prepareTestEnv(t)

session := loginUser(t, "user2")

// repo should not exist
req := NewRequest(t, "GET", "/api/v1/repos/user2/autocreated")
MakeRequest(t, req, http.StatusNotFound)

//
// if 'AUTO_CREATE = false', the repo should not be created if it doesn't exist
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe don't use empty comment line.

setting.Repository.Upload.AutoCreate = false
req = NewRequest(t, "GET", "/user2/autocreated.git/info/refs?service=git-receive-pack")
session.MakeRequest(t, req, http.StatusNotFound)

req = NewRequest(t, "GET", "/api/v1/repos/user2/autocreated")
MakeRequest(t, req, http.StatusNotFound)

//
// if 'AUTO_CREATE = true', the repos should be created
//
setting.Repository.Upload.AutoCreate = true
req = NewRequest(t, "GET", "/user2/autocreated.git/info/refs?service=git-receive-pack")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if logged user is not same user as in url? You should check both options.

// we don't get a successful response code here, because we don't execute a real 'git push ...'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do real push? (I don't know how git requests look)

session.MakeRequest(t, req, NoExpectedStatus)

// repo should exist
req = NewRequest(t, "GET", "/api/v1/repos/user2/autocreated")
resp := MakeRequest(t, req, http.StatusOK)

var repo api.Repository
DecodeJSON(t, resp, &repo)
assert.Equal(t, repo.Name, "autocreated")

//
// cleanup
//
req = NewRequest(t, "DELETE", "/api/v1/repos/user2/autocreated")
session.MakeRequest(t, req, http.StatusNoContent)
}
3 changes: 3 additions & 0 deletions modules/setting/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ var (
AllowedTypes []string `delim:"|"`
FileMaxSize int64
MaxFiles int
AutoCreate bool
} `ini:"-"`

// Repository local settings
Expand Down Expand Up @@ -208,12 +209,14 @@ var (
AllowedTypes []string `delim:"|"`
FileMaxSize int64
MaxFiles int
AutoCreate bool
}{
Enabled: true,
TempPath: "data/tmp/uploads",
AllowedTypes: []string{},
FileMaxSize: 3,
MaxFiles: 5,
AutoCreate: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool is false by default, don't need to be set.

},

// Repository local settings
Expand Down
7 changes: 7 additions & 0 deletions routers/repo/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,13 @@ func HTTP(ctx *context.Context) {
}

repo, err := models.GetRepositoryByName(repoUser.ID, reponame)
if models.IsErrRepoNotExist(err) && setting.Repository.Upload.AutoCreate {
Copy link
Member

@Morlinest Morlinest Sep 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look good to me. You should check err for each function. Later err check block is expecting only GetRepositoryByName error, not CreateRepository error. I think something like this:

	repo, err := models.GetRepositoryByName(repoUser.ID, reponame)
	if err != nil {
		if models.IsErrRepoNotExist(err) {
			if accessMode == models.AccessModeWrite && setting.Repository.Upload.AutoCreate {
				repo, err = models.CreateRepository(repoUser, repoUser, models.CreateRepoOptions{
					Name: reponame,
				})
				if err != nil {
					ctx.Handle(http.StatusInternalServerError, "CreateRepository", err)
					return
				}
			} else {
				ctx.Handle(http.StatusNotFound, "GetRepositoryByName", nil)
				return
			}
		} else {
			ctx.Handle(http.StatusInternalServerError, "GetRepositoryByName", err)
			return
		}
	}

Edit: Updated if condition in example.

Copy link
Member

@Morlinest Morlinest Sep 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you are creating repository on any action, you should check if user is doing push or pull. I think you can use accessMode variable defined few lines above (changing if condition to accessMode == models.AccessModeWrite && setting.Repository.Upload.AutoCreate

Edit: Or use isPull variable.


// if 'CreateRepository' fails, the error are handled in the next 'if err != nil' block
repo, err = models.CreateRepository(repoUser, repoUser, models.CreateRepoOptions{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about access rights? Logged user don't need to be same as repoUser. You need user is logged and if he can create repository under repoUser (e.g. is admin or wants to add repository to his organization or organization he is in owner team).

Name: reponame,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about private/public repo? I think repository should be private by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it hard-coded private or with an extra bool setting - like 'AutoCreateAsPrivate'?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be Auto Create specific but probably more generic like 'NewRepositoryDefaultPrivate` and this setting should be reused to set private check box to this default when creating new repository also in UI

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point - i make a extra pr for this tomorrow

Copy link
Member

@Morlinest Morlinest Sep 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lafriks Actually I think it is more secure to set it private by default. In UI you can tell what you want, but if you autocreate repository, you don't have any option. Also if you use some general variable, you know about it as admin, but not as user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Morlinest yes but that does not mean that can't be configurable (as I need myself to to have such option so that I don't have remember to check that check box when creating new repository :) )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can add configurable variable, but we should not mix UI and some kind of auto create function.

})
}
if err != nil {
if models.IsErrRepoNotExist(err) {
ctx.Handle(http.StatusNotFound, "GetRepositoryByName", nil)
Expand Down