Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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 _test_tools/exechook_command_with_sleep.sh
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@

sleep 3
cat file > exechook
cat exechook
Copy link
Member

Choose a reason for hiding this comment

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

why?

if [[ "$(pwd)" != "$(pwd -P)" ]]; then echo "true" > delaycheck; fi
echo "ENVKEY=$ENVKEY" > exechook-env
58 changes: 44 additions & 14 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ func main() {
flHooksAsync := pflag.Bool("hooks-async",
envBool(true, "GITSYNC_HOOKS_ASYNC", "GIT_SYNC_HOOKS_ASYNC"),
Copy link
Member

Choose a reason for hiding this comment

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

You don't need "GIT_SYNC_HOOKS_ASYNC" - some flags have old names for back-compat, but this is net-new :)

"run hooks asynchronously")
flHooksBeforeSymlink := pflag.Bool("hooks-before-symlink",
envBool(false, "GITSYNC_HOOKS_BEFORE_SYMLINK", "GIT_SYNC_HOOKS_BEFORE_SYMLINK"),
Copy link
Member

Choose a reason for hiding this comment

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

as with the other, you only need the "GITSYNC_" name

"run hooks before creating the symlink (defaults to false)")

flUsername := pflag.String("username",
envString("", "GITSYNC_USERNAME", "GIT_SYNC_USERNAME"),
Expand Down Expand Up @@ -879,6 +882,25 @@ func main() {
go exechookRunner.Run(context.Background())
}

runHooks := func(hash string) error {
var err error
if exechookRunner != nil {
log.V(3).Info("sending exechook")
err = exechookRunner.Send(hash)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to Send() on both, even if an error happens.

}
}
if webhookRunner != nil {
log.V(3).Info("sending webhook")
err = webhookRunner.Send(hash)
}
if err != nil {
return err
}
return nil
}

// Setup signal notify channel
sigChan := make(chan os.Signal, 1)
if syncSig != 0 {
Expand Down Expand Up @@ -919,11 +941,12 @@ func main() {

failCount := 0
syncCount := uint64(0)

for {
start := time.Now()
ctx, cancel := context.WithTimeout(context.Background(), *flSyncTimeout)

if changed, hash, err := git.SyncRepo(ctx, refreshCreds); err != nil {
if changed, hash, err := git.SyncRepo(ctx, refreshCreds, runHooks, *flHooksBeforeSymlink); err != nil {
failCount++
updateSyncMetrics(metricKeyError, start)
if *flMaxFailures >= 0 && failCount >= *flMaxFailures {
Expand All @@ -944,11 +967,10 @@ func main() {
log.V(3).Info("touched touch-file", "path", absTouchFile)
}
}
if webhookRunner != nil {
webhookRunner.Send(hash)
}
if exechookRunner != nil {
exechookRunner.Send(hash)
// if --hooks-before-symlink is set, these will have already been sent and completed.
Copy link
Member

Choose a reason for hiding this comment

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

since we already pass runhooks to SyncRepo(), why not let it be part of the impl there - call it from inside SyncRepo, either before or after the symlink.

// otherwise, we send them now.
if !*flHooksBeforeSymlink {
runHooks(hash)
}
updateSyncMetrics(metricKeySuccess, start)
} else {
Expand All @@ -968,14 +990,17 @@ func main() {
// Assumes that if hook channels are not nil, they will have at
// least one value before getting closed
exitCode := 0 // is 0 if all hooks succeed, else is 1
if exechookRunner != nil {
if err := exechookRunner.WaitForCompletion(); err != nil {
exitCode = 1
// This will not be needed if async == false, because the Send func for the hookRunners will wait
if *flHooksAsync {
Copy link
Member

Choose a reason for hiding this comment

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

This smells funny.

Why not validate early (with all the other flag validation) that flOneTime and flHooksAsync are mutually exclusive - you can't ask for both one-time and synchrous?

Or maybe it's better to decompose a bit more. Instead of having the HookRunner do the Wait internally, have main() do the wait when it's in synchronous mode. Then the anonymous function you build here can check the flag.

That also means that Send() doesn't need to return an error, which simplifies things when you have both web and exec hooks.

Does that make sense? I am a bit jetlagged, let me know if I am not being clear :)

if exechookRunner != nil {
if err := exechookRunner.WaitForCompletion(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Looking at it more, this is a bit tricky - you might get a result on the channel that indicates that the hook failed, but it will be retried (async). For one-time that's fine, but is it OK in non-one-time mode? If you ask for synchronous hooks, and the hook fails -- do we think that's "good enough" or do we wait for a success? What if success never comes?

exitCode = 1
}
}
}
if webhookRunner != nil {
if err := webhookRunner.WaitForCompletion(); err != nil {
exitCode = 1
if webhookRunner != nil {
if err := webhookRunner.WaitForCompletion(); err != nil {
exitCode = 1
}
}
}
log.DeleteErrorFile()
Expand Down Expand Up @@ -1723,7 +1748,7 @@ func (git *repoSync) currentWorktree() (worktree, error) {
// SyncRepo syncs the repository to the desired ref, publishes it via the link,
// and tries to clean up any detritus. This function returns whether the
// current hash has changed and what the new hash is.
func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Context) error) (bool, string, error) {
func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Context) error, runHooks func(hash string) error, flHooksBeforeSymlink bool) (bool, string, error) {
git.log.V(3).Info("syncing", "repo", redactURL(git.repo))

if err := refreshCreds(ctx); err != nil {
Expand Down Expand Up @@ -1778,6 +1803,11 @@ func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Con
// path was different.
changed := (currentHash != remoteHash) || (currentWorktree != git.worktreeFor(currentHash))

// Fire hooks if needed.
if flHooksBeforeSymlink {
Copy link
Member

Choose a reason for hiding this comment

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

This is running before the new hash is even checked out?

runHooks(remoteHash)
}

// We have to do at least one fetch, to ensure that parameters like depth
// are set properly. This is cheap when we already have the target hash.
if changed || git.syncCount == 0 {
Expand Down
28 changes: 27 additions & 1 deletion test_e2e.sh
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ function assert_file_eq() {
if [[ $(cat "$1") == "$2" ]]; then
return
fi
fail "$1 does not contain '$2': $(cat "$1")"
fail "$1 does not equal '$2': $(cat "$1")"
}

function assert_file_contains() {
Expand Down Expand Up @@ -2509,6 +2509,32 @@ function e2e::exechook_success_hooks_non_async() {
assert_file_eq "$ROOT/link/exechook-env" "$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL"
}

##############################################
# Test exechook-success with --hooks-async=false and --hooks-before-symlink
##############################################
function e2e::exechook_success_hooks_before_symlink_non_async() {
cat /dev/null > "$RUNLOG"

GIT_SYNC \
--hooks-async=false \
Copy link
Member

Choose a reason for hiding this comment

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

args-under-test go at the end of the list

--hooks-before-symlink \
--period=100ms \
--repo="file://$REPO" \
--root="$ROOT" \
--link="link" \
--exechook-command="/$EXECHOOK_COMMAND_SLEEPY" \
--exechook-backoff=1s \
&
sleep 5 # give it time to run
assert_link_exists "$ROOT/link"
assert_file_exists "$ROOT/link/file"
assert_file_exists "$ROOT/link/exechook"
assert_file_eq "$ROOT/link/file" "${FUNCNAME[0]}"
assert_file_eq "$ROOT/link/exechook" "${FUNCNAME[0]}"
assert_file_eq "$ROOT/link/exechook-env" "$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL"
assert_file_absent "$ROOT/link/delaycheck"
Copy link
Member

Choose a reason for hiding this comment

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

I am unclear on what this is actually proving? Can you explain?

}

##############################################
# Test webhook success
##############################################
Expand Down