fix: Backoff retry should not swallow errors. Fixes argoproj#1166 (ar…

…goproj#1167)

Signed-off-by: Derek Wang <[email protected]>

common/retry.go

@@ -17,6 +17,7 @@ limitations under the License.
 package common
 
 import (
+	"fmt"
 	"time"
 
 	"github.com/pkg/errors"
@@ -94,10 +95,18 @@ func Connect(backoff *apicommon.Backoff, conn func() error) error {
 	if err != nil {
 		return errors.Wrap(err, "invalid backoff configuration")
 	}
-	return wait.ExponentialBackoff(*b, func() (bool, error) {
-		if err := conn(); err != nil {
+	if waitErr := wait.ExponentialBackoff(*b, func() (bool, error) {
+		if err = conn(); err != nil {
+			// return "false, err" will cover waitErr
 			return false, nil
 		}
 		return true, nil
-	})
+	}); waitErr != nil {
+		if err != nil {
+			return fmt.Errorf("%v: %v", waitErr, err)
+		} else {
+			return waitErr
+		}
+	}
+	return nil
 }

common/retry_test.go

@@ -16,6 +16,8 @@ limitations under the License.
 package common
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/argoproj/argo-events/pkg/apis/sensor/v1alpha1"
@@ -36,3 +38,16 @@ func TestRetryableKubeAPIError(t *testing.T) {
 	assert.False(t, IsRetryableKubeAPIError(errInvalid))
 	assert.False(t, IsRetryableKubeAPIError(errMethodNotSupported))
 }
+
+func TestConnect(t *testing.T) {
+	err := Connect(nil, func() error {
+		return fmt.Errorf("new error")
+	})
+	assert.NotNil(t, err)
+	assert.True(t, strings.Contains(err.Error(), "new error"))
+
+	err = Connect(nil, func() error {
+		return nil
+	})
+	assert.Nil(t, err)
+}