Drop db operations from hook commands #1514
Conversation
…tead of internal routes
| ) | ||
|
|
||
| // GetProtectedBranchBy get protected branch information | ||
| func GetProtectedBranchBy(repoID int64, branchName string) (*models.ProtectedBranch, error) { |
There was a problem hiding this comment.
This should be moved to go-gitea/sdk later on... I'm not gonna ask you to do it now , but we're gonna have to move it at some point.
There was a problem hiding this comment.
Why should we move it to go-gitea/sdk later? It is an internal SDK.
There was a problem hiding this comment.
Should still be in the SDK. That way other implementations of gitea serv could re-use this :)
There was a problem hiding this comment.
I don't think that the sdk contain any internal route but we could maybe add it in an other time.
There was a problem hiding this comment.
In fact it add /api/v1 to url request (https://github.com/go-gitea/go-sdk/blob/master/gitea/gitea.go#L42) so no internal route.
routers/private/push_update.go
Outdated
| "strings" | ||
|
|
||
| macaron "gopkg.in/macaron.v1" | ||
|
|
There was a problem hiding this comment.
local package -> public package.
|
related #1023 |
tboerger
added
the
lgtm/need 2
cmd/hook.go
Outdated
| log.NewGitLogger(filepath.Join(setting.LogRootPath, logPath)) | ||
| models.LoadConfigs() | ||
|
|
||
| return nil |
There was a problem hiding this comment.
No error return. Maybe changed to
func hookSetup(logPath string) {
|
LGTM |
|
@lunny Why do you think this is a good idea? Just trying to get the intention... Doing GET and POST from hooks seems unstable to me. Will they work when HTTPS is being used? |
tboerger
added
lgtm/need 1
@andreynering They will use |
| // GetProtectedBranchBy get protected branch information | ||
| func GetProtectedBranchBy(repoID int64, branchName string) (*models.ProtectedBranch, error) { | ||
| // Ask for running deliver hook and test pull request tasks. | ||
| reqURL := setting.LocalURL + fmt.Sprintf("api/internal/branch/%d/%s", repoID, branchName) |
There was a problem hiding this comment.
@andreynering As you can see here it uses LocalURL which is fine 🙂
|
@andreynering As far as "is this a good idea?", this is actually how |
|
@andreynering It will let us avoid open db on |
|
LGTM |
tboerger
added
lgtm/done
No description provided.