Take back control of hooks #1006

lunny · 2017-02-22T03:25:17Z

This PR depends on go-gitea/git#34. On this PR, we changed the hooks directory structure.

Now we put all the server-side hooks(pre-receive, update, post-receive) on hooks/pre-receive.d/, hooks/update.d/ and hooks/post-receive.d/.
Gitea's hook will be at hooks/pre-receive.d/gitea, hooks/update.d/gitea and hooks/post-receive.d/gitea.
And the old user hooks will be at hooks/pre-receive.d/pre-receive and hooks/post-receive.d/post-receive. Of course, we will support multiple hooks on the UI sometime, it's will be easy if this PR is merged.

bkcsoft

Otherwise LGTM

bkcsoft · 2017-02-22T03:50:45Z

cmd/hook.go

+ CmdHook = cli.Command{
+ Name: "hook",
+ Usage: "Delegate commands to corresponding Git hooks",
+ Description: "All sub-commands should only be called by Git",


"This should only be called by Git"

bkcsoft · 2017-02-22T04:03:46Z

models/repo.go

+ return x.Where("id > 0").Iterate(new(Repository),
+ func(idx int, bean interface{}) error {
+ if err := createDelegateHooks(bean.(*Repository).RepoPath()); err != nil {
+ return err


Maybe prefix the error with SyncRepositoryHook: %v ?

bkcsoft · 2017-02-22T04:05:41Z

Of course, we will support multiple hooks on the UI sometime, it's will be easy if this PR is merged.

Agreed :)

bkcsoft · 2017-02-22T05:34:09Z

LGTM

typeless · 2017-02-22T08:22:36Z

cmd/hook.go

+ if len(os.Getenv("SSH_ORIGINAL_COMMAND")) == 0 {
+ return nil
+ }
+ if err := setup("hooks/pre-receive.log"); err != nil {


The function name 'setup' is a bit of vague as to what it actually does. I guess it's about setting up a logger?
But I don't understand why it has anything to do with the database engine.

Setup is to initialize the command, this function is for serval commands which the different is the log place.

Looks like it's resulted from factoring out the common 'preparing' part of all the commands. Maybe the Before callback is a suitable place for it.

https://sourcegraph.com/github.com/lunny/gitea@f132edf80f2de3b9335e99eec7d23abdfcc6670f/-/blob/vendor/github.com/urfave/cli/command.go#L32:2-32:8

Before should be the same actions, but this time we have a parameter - log name.

The Before callback has the context parameter which has the needed (I guess) information like the command name to construct the path of the log file, if I wasn't misunderstanding something

Actually, I am not sure if putting it in Before is better or not. But there is the possibility. 😉

typeless · 2017-02-22T08:27:20Z

cmd/hook.go

+ args := c.Args()
+ if len(args) != 3 {
+ fail("Arguments received are not equal to three", "Arguments received are not equal to three")
+ } else if len(args[0]) == 0 {


I suppose the two conditions are not mutually exclusive? Not sure if I understand it correctly, but the use of else if seems suspicious.

Yes. Here I just move codes from update.go to hook.go. I think it's no problem. Suppose the command hook update "" a b. The refName is empty.

typeless · 2017-02-22T08:32:12Z

cmd/hook.go

+}
+
+func runHookUpdate(c *cli.Context) error {
+ if len(os.Getenv("SSH_ORIGINAL_COMMAND")) == 0 {


I don't understand why the check is needed, with the nearby context.

If the SSH_ORIGINAL_COMMAND is not set, that's because it is called by shell on manually.

typeless · 2017-02-22T08:54:00Z

models/migrations/v19.go

+ }
+ }
+
+ if err = ioutil.WriteFile(oldHookPath, []byte(hookTpls[i]), 0777); err != nil {


Is 0777 necessary? Least privilege is prefered in general.

Yes. It seems 0755 is enough.

If 0700 is enough then 0700 is better. I cannot think of a scenario where 'others' need the permissions to R/W it.

Edit: Okay, it would be much convenient when debugging 😆

You are right. But I ls -l the repository's root dir, all the permission is 755. Don't know why.

typeless · 2017-02-22T09:15:14Z

Otherwise LGTM and good job 👍

lunny · 2017-02-22T11:59:20Z

will fix #343

appleboy · 2017-02-22T14:34:21Z

cmd/hook.go

+
+ "github.com/urfave/cli"
+
+ "code.gitea.io/gitea/models"


coding style?

import ( // stdlib "encoding/json" "fmt" // local packages "code.gitea.io/gitea/models" "code.gitea.io/sdk/gitea" // external packages "github.com/foo/bar" "gopkg.io/baz.v1" )

appleboy · 2017-02-22T14:36:33Z

models/migrations/v19.go

+ "github.com/Unknwon/com"
+ "github.com/go-xorm/xorm"
+
+ "code.gitea.io/gitea/modules/setting"


Same above coding style.

appleboy · 2017-02-22T14:53:38Z

build fails

appleboy · 2017-02-23T01:08:54Z

@lunny rebase base on master can fix build errors?

lunny · 2017-02-23T01:12:59Z

Yes. let me try.

appleboy · 2017-02-23T02:00:23Z

LGTM

lunny · 2017-02-23T03:39:59Z

let L-G-T-M work

tboerger · 2017-02-25T22:05:16Z

models/migrations/v19.go

+ var (
+ hookNames = []string{"pre-receive", "update", "post-receive"}
+ hookTpls = []string{
+ fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/pre-receive.d\"`; do\n sh \"$SHELL_FOLDER/pre-receive.d/$i\"\ndone", setting.ScriptType),


It's late but still want to comment it. You don't need the quotes for the ls and you don't need to call ls. Just something like for I in folder/*; do done. Beside that I would use source instead of sh to execute the scripts, than you can also define env variables in scripts that can be used in another script later on

Maybe you are right since I'm not familiar with shell script. We also have runServe could define env variables before these script to be executed. So it's no need here to put another one.

lunny added type/enhancement An improvement of existing functionality type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Feb 22, 2017

lunny added this to the 1.1.0 milestone Feb 22, 2017

lunny mentioned this pull request Feb 22, 2017

Use a new hooks dir struct go-gitea/git#34

Merged

bkcsoft suggested changes Feb 22, 2017

View reviewed changes

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 22, 2017

lunny added the pr/wip This PR is not ready for review label Feb 22, 2017

lunny changed the title ~~Take control hooks back~~ [WIP] Take control hooks back Feb 22, 2017

bkcsoft changed the title ~~[WIP] Take control hooks back~~ [WIP] Take back control of hooks Feb 22, 2017

lunny removed the pr/wip This PR is not ready for review label Feb 22, 2017

lunny changed the title ~~[WIP] Take back control of hooks~~ Take back control of hooks Feb 22, 2017

bkcsoft approved these changes Feb 22, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 22, 2017

typeless reviewed Feb 22, 2017

View reviewed changes

lunny force-pushed the lunny/tack_control_hooks_back branch from f132edf to 39387ec Compare February 22, 2017 08:41

typeless reviewed Feb 22, 2017

View reviewed changes

appleboy suggested changes Feb 22, 2017

View reviewed changes

appleboy reviewed Feb 22, 2017

View reviewed changes

unknwon and others added 2 commits February 23, 2017 09:13

git: delegate all server-side Git hooks (go-gitea#1623)

ec82565

create hooks directories

24be2d7

lunny added 4 commits February 23, 2017 09:13

take control hooks back

6a98cc0

fix lint

a3ae540

bug fixed and minor changes

a62eb0f

fix imports style

9a9f14d

lunny force-pushed the lunny/tack_control_hooks_back branch from f42099d to 9a9f14d Compare February 23, 2017 01:17

appleboy approved these changes Feb 23, 2017

View reviewed changes

tboerger 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 Feb 23, 2017

fix migration scripts

2c33815

lunny merged commit 0e6b9ea into go-gitea:master Feb 23, 2017

lunny added a commit to lunny/gitea that referenced this pull request Feb 23, 2017

added back unmerged update command on go-gitea#1006

a3813a0

This was referenced Feb 23, 2017

Added back unmerged update command on #1006 #1028

Closed

Take back control of git-hooks (aka The Great Migration) #343

Closed

tboerger reviewed Feb 25, 2017

View reviewed changes

lunny mentioned this pull request Mar 8, 2017

Git hooks are not being executed on Windows when running as a service #1139

Closed

lunny deleted the lunny/tack_control_hooks_back branch March 8, 2017 15:11

zeripath mentioned this pull request Feb 3, 2020

Any events on webhook send twice #9183

Closed

7 tasks

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take back control of hooks #1006

Take back control of hooks #1006

lunny commented Feb 22, 2017

bkcsoft left a comment

bkcsoft Feb 22, 2017

lunny Feb 22, 2017

bkcsoft Feb 22, 2017

lunny Feb 22, 2017

bkcsoft commented Feb 22, 2017

bkcsoft commented Feb 22, 2017

typeless Feb 22, 2017

lunny Feb 22, 2017

typeless Feb 22, 2017

lunny Feb 22, 2017

typeless Feb 22, 2017 •

edited

Loading

typeless Feb 22, 2017

lunny Feb 22, 2017

typeless Feb 22, 2017 •

edited

Loading

lunny Feb 22, 2017

typeless Feb 22, 2017

lunny Feb 22, 2017

typeless Feb 22, 2017 •

edited

Loading

lunny Feb 22, 2017

typeless commented Feb 22, 2017

lunny commented Feb 22, 2017

appleboy Feb 22, 2017

lunny Feb 22, 2017

appleboy Feb 22, 2017

lunny Feb 22, 2017

appleboy commented Feb 22, 2017

appleboy commented Feb 23, 2017

lunny commented Feb 23, 2017

appleboy commented Feb 23, 2017

lunny commented Feb 23, 2017

tboerger Feb 25, 2017

lunny Feb 26, 2017

Take back control of hooks #1006

Take back control of hooks #1006

Conversation

lunny commented Feb 22, 2017

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Feb 22, 2017

bkcsoft commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless commented Feb 22, 2017

lunny commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleboy commented Feb 22, 2017

appleboy commented Feb 23, 2017

lunny commented Feb 23, 2017

appleboy commented Feb 23, 2017

lunny commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless Feb 22, 2017 •

edited

Loading

typeless Feb 22, 2017 •

edited

Loading

typeless Feb 22, 2017 •

edited

Loading