From 9eed6946b71832d40b4e1a9bf98660ac5dd91e1e Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 19 Jan 2022 11:37:54 -0800 Subject: [PATCH 1/2] e2e cleanup Fix some whitespace and names. Make command-line and flag handling a bit cleaner. --- test_e2e.sh | 96 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 40 deletions(-) diff --git a/test_e2e.sh b/test_e2e.sh index 40bb137..9538363 100755 --- a/test_e2e.sh +++ b/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 \ - --period=100ms \ - --one-time \ - --repo="file://$REPO" \ - --branch="$MAIN_BRANCH" \ - --root="$ROOT" \ - --link="link" \ - --exechook-command="$EXECHOOK_COMMAND_FAIL_SLEEPY" \ - --exechook-backoff=1s \ - >> "$1" 2>&1 + # First sync - return a failure to ensure that we try again + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 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 least 1" - fi + GIT_SYNC \ + --period=100ms \ + --one-time \ + --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 + 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 - fail "webhook 1 called $HITS times" + if [[ "$HITS" != 1 ]]; then + 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 - fail "webhook 1 called $HITS times" + if [[ "$HITS" != 1 ]]; then + 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. -if [[ "$#" == 1 && "$1" == "-?" ]]; then +function print_tests() { 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 From c143bfd31a528b86d68970d19a58108d7b79b98e Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 19 Jan 2022 11:37:22 -0800 Subject: [PATCH 2/2] Small cleanup on one-time x hooks --- cmd/git-sync/main.go | 18 ++++-------------- pkg/hook/hook.go | 40 +++++++++++++++++++++------------------- 2 files changed, 25 insertions(+), 33 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index d0d1d3b..8b80b40 100644 --- a/cmd/git-sync/main.go +++ b/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 { - log.Error(err, "exechook completed with error") + if err := exechookRunner.WaitForCompletion(); err != nil { exitCode = 1 } } if webhookRunner != nil { - if err = webhookRunner.WaitForCompletion(); err != nil { - log.Error(err, "webhook completed with error") + if err := webhookRunner.WaitForCompletion(); err != nil { exitCode = 1 } } diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index 13f92e3..326c209 100644 --- a/pkg/hook/hook.go +++ b/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 { - return &HookRunner{hook: hook, backoff: backoff, data: data, logger: log, hasCompletedOnce: hasSucceededOnce} +func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log *logging.Logger, oneTime bool) *HookRunner { + 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 - // successfully and false otherwise to some receiver. Should be - // initialised to a buffered channel of size 1. - // Is only meant for use within sendCompletedOnceMessageIfApplicable. - hasCompletedOnce chan bool + // Used to send a status result when running in one-time mode. + // Should be initialised to a buffered channel of size 1. + oneTimeResult 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. -// Using this function to write to hasCompletedOnce ensures it is only ever +// oneTimeResult, closes said chanel, and terminates this goroutine. +// Using this function to write to oneTimeResult ensures it is only ever // written to once. -func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully bool) { - if r.hasCompletedOnce != nil { - r.hasCompletedOnce <- completedSuccessfully - close(r.hasCompletedOnce) +func (r *HookRunner) sendOneTimeResultAndTerminate(completedSuccessfully bool) { + if r.oneTimeResult != nil { + r.oneTimeResult <- completedSuccessfully + 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") }