Merge pull request #468 from thockin/master

Followup to #466 - small cleanups for one-time hooks

cmd/git-sync/main.go

@@ -544,10 +544,6 @@ func main() {
 	// Startup webhooks goroutine
 	var webhookRunner *hook.HookRunner
 	if *flWebhookURL != "" {
-		var webhookChannel chan bool
-		if *flOneTime {
-			webhookChannel = make(chan bool, 1)
-		}
 		webhook := hook.NewWebhook(
 			*flWebhookURL,
 			*flWebhookMethod,
@@ -560,7 +556,7 @@ func main() {
 			*flWebhookBackoff,
 			hook.NewHookData(),
 			log,
-			webhookChannel,
+			*flOneTime,
 		)
 		go webhookRunner.Run(context.Background())
 	}
@@ -568,10 +564,6 @@ func main() {
 	// Startup exechooks goroutine
 	var exechookRunner *hook.HookRunner
 	if *flExechookCommand != "" {
-		var exechookChannel chan bool
-		if *flOneTime {
-			exechookChannel = make(chan bool, 1)
-		}
 		exechook := hook.NewExechook(
 			cmd.NewRunner(log),
 			*flExechookCommand,
@@ -585,7 +577,7 @@ func main() {
 			*flExechookBackoff,
 			hook.NewHookData(),
 			log,
-			exechookChannel,
+			*flOneTime,
 		)
 		go exechookRunner.Run(context.Background())
 	}
@@ -639,14 +631,12 @@ func main() {
 				// 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 {
+					if err := exechookRunner.WaitForCompletion(); err != nil {
-						log.Error(err, "exechook completed with error")
 						exitCode = 1
 					}
 				}
 				if webhookRunner != nil {
-					if err = webhookRunner.WaitForCompletion(); err != nil {
+					if err := webhookRunner.WaitForCompletion(); err != nil {
-						log.Error(err, "webhook completed with error")
 						exitCode = 1
 					}
 				}

pkg/hook/hook.go

@@ -84,8 +84,12 @@ func (d *hookData) send(newHash string) {
 }
 
 // NewHookRunner returns a new HookRunner
-func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log *logging.Logger, hasSucceededOnce chan bool) *HookRunner {
+func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log *logging.Logger, oneTime bool) *HookRunner {
-	return &HookRunner{hook: hook, backoff: backoff, data: data, logger: log, hasCompletedOnce: hasSucceededOnce}
+	hr := &HookRunner{hook: hook, backoff: backoff, data: data, logger: log}
+	if oneTime {
+		hr.oneTimeResult = make(chan bool, 1)
+	}
+	return hr
 }
 
 // HookRunner struct
@@ -98,11 +102,9 @@ type HookRunner struct {
 	data *hookData
 	// Logger
 	logger *logging.Logger
-	// hasCompletedOnce is used to send true if and only if first run executed
+	// Used to send a status result when running in one-time mode.
-	// successfully and false otherwise to some receiver.  Should be
+	// Should be initialised to a buffered channel of size 1.
-	// initialised to a buffered channel of size 1.
+	oneTimeResult chan bool
-	// Is only meant for use within sendCompletedOnceMessageIfApplicable.
-	hasCompletedOnce chan bool
 }
 
 // Send sends hash to hookdata
@@ -131,27 +133,27 @@ func (r *HookRunner) Run(ctx context.Context) {
 				r.logger.Error(err, "hook failed")
 				updateHookRunCountMetric(r.hook.Name(), "error")
 				// don't want to sleep unnecessarily terminating anyways
-				r.sendCompletedOnceMessageIfApplicable(false)
+				r.sendOneTimeResultAndTerminate(false)
 				time.Sleep(r.backoff)
 			} else {
 				updateHookRunCountMetric(r.hook.Name(), "success")
 				lastHash = hash
-				r.sendCompletedOnceMessageIfApplicable(true)
+				r.sendOneTimeResultAndTerminate(true)
 				break
 			}
 		}
 	}
 }
 
-// If hasCompletedOnce is nil, does nothing. Otherwise, forwards the caller
+// If oneTimeResult is nil, does nothing. Otherwise, forwards the caller
 // provided success status (as a boolean) of HookRunner to receivers of
-// hasCompletedOnce, closes said chanel, and terminates this goroutine.
+// oneTimeResult, closes said chanel, and terminates this goroutine.
-// Using this function to write to hasCompletedOnce ensures it is only ever
+// Using this function to write to oneTimeResult ensures it is only ever
 // written to once.
-func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully bool) {
+func (r *HookRunner) sendOneTimeResultAndTerminate(completedSuccessfully bool) {
-	if r.hasCompletedOnce != nil {
+	if r.oneTimeResult != nil {
-		r.hasCompletedOnce <- completedSuccessfully
+		r.oneTimeResult <- completedSuccessfully
-		close(r.hasCompletedOnce)
+		close(r.oneTimeResult)
 		runtime.Goexit()
 	}
 }
@@ -159,15 +161,15 @@ func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully
 // WaitForCompletion waits for HookRunner to send completion message to
 // calling thread and returns either true if HookRunner executed successfully
 // and some error otherwise.
-// Assumes that r.hasCompletedOnce is not nil, but if it is, returns an error
+// Assumes that r.oneTimeResult is not nil, but if it is, returns an error
 func (r *HookRunner) WaitForCompletion() error {
 	// Make sure function should be called
-	if r.hasCompletedOnce == nil {
+	if r.oneTimeResult == nil {
 		return fmt.Errorf("HookRunner.WaitForCompletion called on async runner")
 	}
 
 	// Perform wait on HookRunner
-	hookRunnerFinishedSuccessfully := <-r.hasCompletedOnce
+	hookRunnerFinishedSuccessfully := <-r.oneTimeResult
 	if !hookRunnerFinishedSuccessfully {
 		return fmt.Errorf("hook completed with error")
 	}

test_e2e.sh

@@ -125,19 +125,6 @@ mkdir -p "$DOT_SSH"
 ssh-keygen -f "$DOT_SSH/id_test" -P "" >/dev/null
 cat "$DOT_SSH/id_test.pub" > "$DOT_SSH/authorized_keys"
 
-function finish() {
-  r=$?
-  trap "" INT EXIT
-  if [[ $r != 0 ]]; then
-    echo
-    echo "the directory $DIR was not removed as it contains"\
-         "log files useful for debugging"
-  fi
-  remove_containers
-  exit $r
-}
-trap finish INT EXIT
-
 SLOW_GIT_CLONE=/slow_git_clone.sh
 SLOW_GIT_FETCH=/slow_git_fetch.sh
 ASKPASS_GIT=/askpass_git.sh
@@ -1109,27 +1096,28 @@ function e2e::exechook_success_once() {
 ##############################################
 function e2e::exechook_fail_once() {
     cat /dev/null > "$RUNLOG"
-        # First sync - return a failure to ensure that we try again
-        echo "$FUNCNAME 1" > "$REPO"/file
-        git -C "$REPO" commit -qam "$FUNCNAME 1"
 
-        GIT_SYNC \
+    # First sync - return a failure to ensure that we try again
-            --period=100ms \
+    echo "$FUNCNAME 1" > "$REPO"/file
-            --one-time \
+    git -C "$REPO" commit -qam "$FUNCNAME 1"
-            --repo="file://$REPO" \
-            --branch="$MAIN_BRANCH" \
-            --root="$ROOT" \
-            --link="link" \
-            --exechook-command="$EXECHOOK_COMMAND_FAIL_SLEEPY" \
-            --exechook-backoff=1s \
-            >> "$1" 2>&1
 
-        # Check that exechook was called
+    GIT_SYNC \
-        sleep 2
+        --period=100ms \
-        RUNS=$(cat "$RUNLOG" | wc -l)
+        --one-time \
-        if [[ "$RUNS" < 1 ]]; then
+        --repo="file://$REPO" \
-            fail "exechook called $RUNS times, it should be at least 1"
+        --branch="$MAIN_BRANCH" \
-        fi
+        --root="$ROOT" \
+        --link="link" \
+        --exechook-command="$EXECHOOK_COMMAND_FAIL_SLEEPY" \
+        --exechook-backoff=1s \
+        >> "$1" 2>&1
+
+    # Check that exechook was called
+    sleep 2
+    RUNS=$(cat "$RUNLOG" | wc -l)
+    if [[ "$RUNS" != 1 ]]; then
+        fail "exechook called $RUNS times, it should be at exactly 1"
+    fi
 }
 
 ##############################################
@@ -1258,8 +1246,8 @@ function e2e::webhook_success_once() {
     # check that basic call works
     sleep 2
     HITS=$(cat "$HITLOG" | wc -l)
-    if [[ "$HITS" < 1 ]]; then
+    if [[ "$HITS" != 1 ]]; then
-        fail "webhook 1 called $HITS times"
+        fail "webhook called $HITS times"
     fi
 
     docker_kill "$CTR"
@@ -1295,8 +1283,8 @@ function e2e::webhook_fail_retry_once() {
     # Check that webhook was called
     sleep 2
     HITS=$(cat "$HITLOG" | wc -l)
-    if [[ "$HITS" < 1 ]]; then
+    if [[ "$HITS" != 1 ]]; then
-        fail "webhook 1 called $HITS times"
+        fail "webhook called $HITS times"
     fi
     docker_kill "$CTR"
 }
@@ -1807,14 +1795,29 @@ function list_tests() {
 # Figure out which, if any, tests to run.
 tests=($(list_tests))
 
-# Use -? to just list tests.
+function print_tests() {
-if [[ "$#" == 1 && "$1" == "-?" ]]; then
     echo "available tests:"
     for t in "${tests[@]}"; do
         echo "    $t"
     done
-    exit 0
+}
-fi
+
+for t; do
+    # Use -? to list known tests.
+    if [[ "${t}" == "-?" ]]; then
+        print_tests
+        exit 0
+    fi
+    # Make sure we know this test.
+    if [[ " ${tests[*]} " =~ " ${t} " ]]; then
+        continue
+    fi
+    # Not a known test or flag.
+    echo "ERROR: unknown test or flag: '${t}'"
+    echo
+    print_tests
+    exit 1
+done
 
 # If no tests specified, run them all.
 if [[ "$#" == 0 ]]; then
@@ -1825,6 +1828,19 @@ fi
 make container REGISTRY=e2e VERSION=$(make -s version)
 make test-tools REGISTRY=e2e
 
+function finish() {
+  r=$?
+  trap "" INT EXIT
+  if [[ $r != 0 ]]; then
+    echo
+    echo "the directory $DIR was not removed as it contains"\
+         "log files useful for debugging"
+  fi
+  remove_containers
+  exit $r
+}
+trap finish INT EXIT
+
 echo
 echo "test root is $DIR"
 echo