-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
auto-create repository on push #2478
Conversation
Maybe add some integration tests? |
@lunny how can i run the integration test local? if i run it per
can i setup the test env local? my code so far in
Thanks. |
if the repository doesn't exists at push time, and the setting 'Repository.Upload.AutoCreate' is 'true' (default is 'false') create the repository with the given name. Signed-off-by: Jürgen Keck <[email protected]>
19f0768
to
7d5fe39
Compare
I have included a test case. Rebased on 'master' and squash the impl + test in one commit. |
|
||
// | ||
// if 'AUTO_CREATE = false', the repo should not be created if it doesn't exist | ||
// |
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.
Maybe don't use empty comment line.
@@ -95,6 +95,13 @@ func HTTP(ctx *context.Context) { | |||
} | |||
|
|||
repo, err := models.GetRepositoryByName(repoUser.ID, reponame) | |||
if models.IsErrRepoNotExist(err) && setting.Repository.Upload.AutoCreate { |
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 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.
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 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{ | ||
Name: reponame, |
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.
What about private/public repo? I think repository should be private by default.
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.
should it hard-coded private or with an extra bool setting - like 'AutoCreateAsPrivate'?
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.
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
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.
good point - i make a extra pr for this tomorrow
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.
@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.
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.
@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 :) )
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.
Yes, we can add configurable variable, but we should not mix UI and some kind of auto create function.
}{ | ||
Enabled: true, | ||
TempPath: "data/tmp/uploads", | ||
AllowedTypes: []string{}, | ||
FileMaxSize: 3, | ||
MaxFiles: 5, | ||
AutoCreate: false, |
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.
bool
is false
by default, don't need to be set.
// 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") |
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.
What if logged user is not same user as in url? You should check both options.
// | ||
setting.Repository.Upload.AutoCreate = true | ||
req = NewRequest(t, "GET", "/user2/autocreated.git/info/refs?service=git-receive-pack") | ||
// we don't get a successful response code here, because we don't execute a real 'git push ...' |
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.
Can you do real push? (I don't know how git requests look)
if models.IsErrRepoNotExist(err) && setting.Repository.Upload.AutoCreate { | ||
|
||
// if 'CreateRepository' fails, the error are handled in the next 'if err != nil' block | ||
repo, err = models.CreateRepository(repoUser, repoUser, models.CreateRepoOptions{ |
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.
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).
my changes was only a quick hack for my local setup - not ready for a pull-request. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
This pull request has been automatically closed because of inactivity. You can re-open it if needed. |
if the repository doesn't exists at push time,
and the setting 'Repository.Upload.AutoCreate' is 'true'
(default is 'false') create the repository with the given name.
Signed-off-by: Jürgen Keck [email protected]